-
-
Notifications
You must be signed in to change notification settings - Fork 719
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
set invoice status automatically after creation #11084
set invoice status automatically after creation #11084
Conversation
0436221
to
d5bb178
Compare
06b85cd
to
00d4731
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've one comment and one question. Thanks @abdellani ! 🙏
|
||
expect { | ||
click_link 'New Invoice' | ||
sleep 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one was strongly needed? I'd love to avoid as much as possible this kind of sleep
in the code...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about replacing it with page.driver.wait_for_network_idle? ( thanks @filipefurtad0 for sharing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better, even if I tend to avoid this as well. But if it's strongly needed, let's do this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
00d4731
to
19fcf2a
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
app/models/invoice.rb
Outdated
@@ -3,7 +3,8 @@ | |||
class Invoice < ApplicationRecord | |||
belongs_to :order, class_name: 'Spree::Order' | |||
serialize :data, Hash | |||
before_validation :serialize_order | |||
before_validation :serialize_order, :set_active |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will set the flag every time the invoice is saved.
app/models/invoice.rb
Outdated
def set_active | ||
self.status = 'active' | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would override an existing value as well. It will be impossible to set the status to another value, even for test purposes.
6f95e3b
to
63dd780
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
I think we need an extra review to double checked @mkllnk changes, @abdellani could you have a look ? |
Hiya, forgive me jumping in. |
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. |
Instead of an integer, we could change I think I would prefer that unless there are plans for more states, @abdellani? |
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: ***@***.***>
|
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! |
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 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 I like the idea of using |
ea72573
to
6066afb
Compare
@mkllnk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍 ! I learnt about squish
today.
6066afb
to
6131343
Compare
6131343
to
4b003aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, nice work!
Hey @abdellani , Scenario 1
Before this PR, an error 500 was triggered, on this scenario: After staging, we can see the Scenario 2
Creating a new invoice (this requires clicking Scenario 3
Also here, it is necessary to click clicking This looks good 👌 It also adds some missing translations - so it closes #11262. Thanks @abdellani !! This improves things quite a lot. Merging. |
What? Why?
What should we test?
Possible scenarios:
Scenario 1:
Scenario 2:
Scenario 3:
Release notes
Changelog Category: Technical changes
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates