[16.0][MIG] purchase_order_secondary_unit #2216
Conversation
|
👍 great, let me know when CI works. |
dalonsod
force-pushed
the
16.0-mig-purchase_order_secondary_unit
branch
from
4325613 to
03c0de5
Compare
|
@SirAionTech done!!! |
There was a problem hiding this comment.
Thanks for the PR!
I only looked at the code.
It would be nice if all the migration changes that do not have to be backported could go in the same [MIG] commit, maybe you could set yourself as co-author of the [MIG] commit, if the original author @yajo agrees?
Could you please also squash the bots commits as explained in https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests#mergesquash-the-commits-generated-by-bots-or-weblate?
| self.assertEqual(line.secondary_uom_qty, 3.5) | ||
| self.assertEqual(line.product_qty, 2450000.0) | ||
| self.assertEqual(line.secondary_uom_qty, 3500.0) |
There was a problem hiding this comment.
I am concerned about this change: the test was expecting line.secondary_uom_qty == 3.5, now it happens that line.secondary_uom_qty == 3500.
Usually when this happens it means that there might be something to be fixed in the code, or some cache value to be reset in the test, but the test's assertions shouldn't be changed in a migration.
Or maybe are you saying that the test was asserting something wrong already in 15.0?
There was a problem hiding this comment.
I'm not sure why these changes were made, maybe @yajo could help us, as original change (at least partially) was made by him, if I'm not wrong
purchase_order_secondary_unit/tests/test_purchase_order_secondary_unit.py
Outdated
Show resolved
Hide resolved
| if hasattr(super(), "_compute_product_qty"): | ||
| return super()._compute_product_qty() | ||
| self._compute_helper_target_field_qty() | ||
| return super()._compute_product_qty() |
There was a problem hiding this comment.
Could you please clarify what the bug was?
There was a problem hiding this comment.
This is explained by original author in commit description
Fix a bug when calling super's_compute_product_qty()was skipping the compute helper from the mixin.
There was a problem hiding this comment.
I had read that already, but saying that the bug was "the helper was not called" does not help me understand what the bug was.
Why do we need to call the helper? What is the error or issue happening without this change?
|
hello @SirAionTech thanks for the comments. Most of them are related to changes made by other contributors, I've only superseded them, prorizing commits authorship and thus, last changes made by other people, so I'm not able to answer some of that questions you've made. On my own, I solved your last questions about separating commits and fixed a pending test, supposing other changes in core were right, indeed (maybe I did a bad choice). I could rollback every change that seems to be unnecessary or inadequate (or simply start from scratch), but I'd wait for @acsonefho and @anddago78 feedback |
|
I think it's good to respect others' authorship. But if you need to squash some things because some commits won't have sense in the final consolidated history, at least tag them as co-authored. |
dalonsod
force-pushed
the
16.0-mig-purchase_order_secondary_unit
branch
2 times, most recently
from
2429a86 to
db5887e
Compare
I've preserved changes in separated commits because one of them was marked as fix by original author, so it can be backported. And merged bots commits as required |
|
Hello, @SirAionTech could you review again? Thanks! |
There was a problem hiding this comment.
Thanks for squashing the commits! I have noticed that 4b1b2f8 is missing, maybe it has been squashed by mistake, could you please check?
Also some threads from previous review #2216 (review) still have to be resolved.
| if hasattr(super(), "_compute_product_qty"): | ||
| return super()._compute_product_qty() | ||
| self._compute_helper_target_field_qty() | ||
| return super()._compute_product_qty() |
There was a problem hiding this comment.
I had read that already, but saying that the bug was "the helper was not called" does not help me understand what the bug was.
Why do we need to call the helper? What is the error or issue happening without this change?
purchase_order_secondary_unit/tests/test_purchase_order_secondary_unit.py
Outdated
Show resolved
Hide resolved
|
please include #2311 |
…_unit mixin model from product-attribute repository.
…condary_uom_qty is filled
Fix a bug when calling super's `_compute_product_qty()` was skipping the compute helper from the mixin. @moduon MT-3483 Co-authored-by: Andrea Cattalani <22261939+anddago78@users.noreply.github.com>
… qty set to 1 Co-authored-by: David Alonso // Solvos <david.alonso@solvos.es>
…ndary quantity fields optional TT49686
…secondary uom without product in sale order line TT51683
dalonsod
force-pushed
the
16.0-mig-purchase_order_secondary_unit
branch
from
db5887e to
ee8f610
Compare
@pilarvargas-tecnativa added
@sergio-teruel added |
@sergio-teruel this PR has been a little headache for me, because I've superseded a PR that had been superseded another one as well. I've tried to preserve some improvements/contributions added by previous authors, some of them technically complex for my current knowledge of recent Odoo versions, or difficult to understand. Most of pending tasks are finished IMO, but some of them are still pending. Not sure if I can resume this in the next weeks, so feel free to supersede it, if you're ready for it... |
|
/ocabot migration purchase_order_secondary_unit |
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at 0b4e9d7. Thanks a lot for contributing to OCA. ❤️ |
@pedrobaeza some things I mentioned in #2216 (review) are still missing, for instance commit 4b1b2f8 has not been ported: it has probably been squashed by mistake in some other commit. |
|
Sorry, didn't know. Can any of you do the corresponding followup PR? |
For what I see, the open comments cannot be fixed afterwards. I wouldn't know how to do a PR to restore a commit that has been squashed in some other commit, even if I added an empty commit (just to mention that the commit's author helped with the translations) the history would be wrong. Another unresolved comment is for understanding why a change has been made, for instance:
(from #2216 (comment)). I was just waiting for some answers and being asked to update the review. Just please have a better look at |
|
Well, the usual flow for blocking a PR is to put "Changes requested". Sorry anyway for not looking with enough care. |
That is what I did in #2216 (review) Please let me know if I had to do something different: if I'm reviewing things wrong I'd like to know before creating more confusion in other pull requests.
Appreciate it, thanks :) |
|
No, you didn't. You pressed on "Comment", not on "Request changes". You did code suggestions, but that's something different.
|
Supersedes #2068
@SirAionTech tried to make every change suggested at that PR (removed unnecessary changes and fix splitted in a standalone commit). One test fails (at least locally), still need work.
cc @acsonefho @anddago78