set invoice status automatically after creation

[abdellani]

What? Why?

• Closes [Invoices] Set an invoice status after creating the first invoice #11044

What should we test?

Possible scenarios:
Scenario 1:

• Create an invoice for an order
• the status must be 'active'

Scenario 2:

• edit order note
• try to create a new invoice
• it should only update the existing invoice ( you'll not see a new invoice)

Scenario 3:

• edit line items
• try to create a new invoice
• a new invoice should be created and set to active, the old invoice should be updated to cancelled.

Release notes

Changelog Category: Technical changes

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

[jibees]

I've one comment and one question. Thanks @abdellani ! 🙏

[jibees]

This one was strongly needed? I'd love to avoid as much as possible this kind of sleep in the code...

[abdellani]

what do you think about replacing it with page.driver.wait_for_network_idle? ( thanks @filipefurtad0 for sharing)

[jibees]

I think it's better, even if I tend to avoid this as well. But if it's strongly needed, let's do this!

[rioug]

I would try to check something UI related instead of waiting, if possible (I am not familiar with this page)
Fixed in this commit : 7196baf

[abdellani]

rspec ./spec/lib/reports/orders_and_fulfillment/orders_cycle_supplier_totals_report_spec.rb:85
is passing locally! I don't see why it's not passing on the pipeline

[mkllnk]

Nice code.

Zooming out to the big picture, I'm wondering if we need the status flag. If the newest invoice is always the active one, can't we just use that? Otherwise we always have the challenge that the data in the database is inconsistent for a tiny moment and we could end up with two active invoices if they are created at the same time (DB transactions not seeing each other).

But thinking about the implementation, it would be complicated and less efficient to query the status by comparing the timestamp to all other invoices belonging to the same order. So I agree that the status flag is good to have.

I would still suggest the following changes:

• Define a default value for status on the database level. This needs a migration but then we don't need the before_validation callback.
• Remove set_active.

[mkllnk]

This will set the flag every time the invoice is saved.

[mkllnk]

This would override an existing value as well. It will be impossible to set the status to another value, even for test purposes.

[rioug]

Looks good 👍

[rioug]

I think we need an extra review to double checked @mkllnk changes, @abdellani could you have a look ?

[s33dco]

Hiya, forgive me jumping in.
Has changing the :status from a string -> integer been considered?, this could be fixed up in one migration and the values reset in a rake task, a default could be then set at the db level, for active and new instances would be initialized in that status? 0: Active. Would have assumed an invoice could only have a handful of state.

[rioug]

Hi @s33dco What would be the benefit of having an integer instead of a string ? To me it's a downgrade, with a string you would need to figure out what the status number mean.

[mkllnk]

Instead of an integer, we could change status, :string to :active, :boolean. That would make more sense to me. Or maybe even better in this case: :cancelled, :boolean. That fits better into the workflow. New invoices are always uncancelled and the creation process cancels previous invoices.

I think I would prefer that unless there are plans for more states, @abdellani?

[s33dco]

Hi, Just figured that an invoice would have a finite number of states which would suit being an Enum in the class/ integer in the DB. Could easily be I18n’d across different locales aswell and maybe it would be beneficial to be able to call invoice.active? (or other methods from enum) in the codebase. From: Gaetan Craig-Riou ***@***.***> Date: Monday, 3 July 2023 at 00:29 To: openfoodfoundation/openfoodnetwork ***@***.***> Subject: Re: [openfoodfoundation/openfoodnetwork] set invoice status automatically after creation (PR #11084) Hi @s33dco<https://github.com/s33dco> What would be the benefit of having an integer instead of a string ? To me it's a downgrade, with a string you would need to figure out what the status number mean. — Reply to this email directly, view it on GitHub<#11084 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AC5ZHJIGGCDRBLLSSR7HULDXOH763ANCNFSM6AAAAAAZNARI6A>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[rioug]

Indeed using an Enum for the status here would work, it wasn't clear to me that's what you were suggesting in your original comment. Thanks!

[s33dco]

i figured an invoice would have more statuses than active / inactive, for example. paid, cancelled, overdue etc.

[abdellani]

i figured an invoice would have more statuses than active / inactive, for example. paid, cancelled, overdue etc.

According to the specifications, we only have two statuses: active/canceled (+ only the latest invoice will be active).

I'm not against the idea of using Enum (which will take less space on the storage compared to a string), but I think that it'll not be consistent with the other models Spree::Order, Payment, Shipment, and Adjustment.
On all those models, we're using the state_machines-activerecord Gem which expects a string column to store the state (not sure if it can be configured to use an integer).

For the other statuses like paid, and canceled... we're tracking those on the Order model (+ there are other state machines on payments and shipment). An invoice can be generated only when an order is on the state completed or resumed.

I like the idea of using :cancelled, :boolean from @mkllnk

[abdellani]

@mkllnk
I squashed all the commits because I needed to edit all the previous changes.

[rioug]

Looks good 👍 ! I learnt about squish today.

[mkllnk]

Cool, nice work!

[filipefurtad0]

Hey @abdellani ,

Scenario 1

- Create an invoice for an order
- the status must be 'active'

Before this PR, an error 500 was triggered, on this scenario:

https://app.bugsnag.com/yaycode/openfoodnetwork-uk/errors/64ba9aa8525cb30007b1caf3?event_id=64ba9aa800bee437b4360000&i=sk&m=nw

After staging, we can see the status is now attributed:

Scenario 2

_edit order note
try to create a new invoice
it should only update the existing invoice ( you'll not see a new invoice)_

Creating a new invoice (this requires clicking New Invoice) updates the exiting invoice and adds the Additional Information section, with the note contents:

Scenario 3

_edit line items
try to create a new invoice
a new invoice should be created and set to active, the old invoice should be updated to cancelled._

Also here, it is necessary to click clicking New Invoice, then a new invoice is generated and the previous one set to cancelled:

This looks good 👌

It also adds some missing translations - so it closes #11262.
I see there might be some overlap with #11252...
I'll merge and see it it needs some reconciliation.

Thanks @abdellani !! This improves things quite a lot. Merging.