-
-
Notifications
You must be signed in to change notification settings - Fork 549
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
[15.0][MIG] base_tier_validation_waiting #638
Conversation
05371d2
to
1084cad
Compare
/ocabot migration base_tier_validation_waiting |
Sorry @bosd you are not allowed to mark the addon tobe migrated. To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons. If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the |
/ocabot migration base_tier_validation_waiting |
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.
LG minor comments
097e6d1
to
26c2caa
Compare
@cwjoalder Can you please review? |
@simahawk Can you review the changes? 🙏 |
81eca93
to
26c2caa
Compare
Force pushed to recreate runboat |
@Dranyel-Bosd Can you please review? |
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 so good to me 👍 🎇
if record.status != "waiting": | ||
continue | ||
# if approve by sequence, check sequence has been reached | ||
if record.approve_sequence: |
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'm not sure I get this IF block: in any case you set "pending" -> can't you have 1 single check?
7d8edf5
to
3c5d1ce
Compare
@simahawk Thanks for your review! I'm working my way towards more coverage and green tests 😁 I've watched your talks about the unittests. This module is altering the behaviour by adding the state waiting, which results in breaking the tests of server action.
|
9b63c33
to
2f5281f
Compare
2f5281f
to
f68e73c
Compare
Maybe this helps to work with the other module |
Co-authored-by: Simone Orsi <simahawk@users.noreply.github.com>
2ee9c17
to
cd6ccb0
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.
LGTM 👍
902d294
to
4a4b26f
Compare
without this the tests of some other modules might fail, e.g. base_tier_validation_server_action. The introduced context prevents writing/patching unit tests of other base_tier_validation_* modules.
4a4b26f
to
174337e
Compare
Dropped the dependency on 732. Now we finally have green tests ✔️ |
|
||
|
||
class TierReview(models.Model): | ||
_inherit = "tier.review" | ||
|
||
def _default_status(self): | ||
if ( | ||
not config["test_enable"] |
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.
do we really need this specific check for tests?
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.
Yes, without this commit, it would mean that we need to patch a lot of depending modules, to fix their tests.
Another solution I've evaluated is using seperate tests. That might solve the problem with the CI tests here on github.
But it would simply transfer the problem to the persons running an custom CI like odoo.sh.
The method here is not really clean. But it's the least dirty solution.
(Discussed this test issue with a couple of oca contributors at the OXP)
Maybe you have another insight to solve this problem?
status = fields.Selection( | ||
selection_add=[("waiting", "Waiting")], | ||
default="waiting", | ||
default=_default_status, |
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.
use lambda to not reference the method object directly
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
Depends on:
#732 Do not sent double messages on the first validation sequence