Skip to content
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

[MIG] account_operating_unit: Migration to 17.0 #670

Merged
merged 110 commits into from
Aug 2, 2024

Conversation

jdidderen-nsi
Copy link
Contributor

@jdidderen-nsi jdidderen-nsi commented May 29, 2024

@jdidderen-nsi jdidderen-nsi force-pushed the 17.0-mig-account_operating_unit branch 3 times, most recently from 10bad65 to 981c6b1 Compare May 31, 2024 17:38
@@ -22,16 +22,6 @@ def create(self, vals_list):
vals["operating_unit_id"] = move.operating_unit_id.id
return super().create(vals_list)

@api.model

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is is just removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was removed from OC in v16 (odoo/odoo#96134) and it seems that it's not used anymore outside of reports like account.invoice.report

def _compute_operating_unit(self):
for record in self:
if record.journal_id.operating_unit_id:
record.operating_unit_id = record.journal_id.operating_unit_id.id

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need to explicitly assign an id to a many2one record, just record.journal_id.operating_unit_id

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. Thanks for the review 👍

@@ -19,10 +19,3 @@ def _select(self):
,line.operating_unit_id
"""
return select_str

def _group_by(self):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is it just removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was removed from OC in v16 (odoo/odoo#96134) and it seems that it's not used anymore outside of reports like account.invoice.report

<field name="line_ids" position="attributes">
<attribute name="context">
<attribute name="context" operation="update">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operation="update" will be only applied if you have a dependency on base_view_inheritance_extension I suppose, so probably you want to overwrite the original context like it was before

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's exactly that

@yankinmax
Copy link

As soon as #667 and #669 is merged, everything should be good to go for this one.

why don't simply add tets-requirements.txt file including dependencies references?
just name that commit with prefix DON'T MERGE

@jdidderen-nsi jdidderen-nsi force-pushed the 17.0-mig-account_operating_unit branch from f48771c to a9cc4f5 Compare June 11, 2024 07:28
@jdidderen-nsi
Copy link
Contributor Author

As soon as #667 and #669 is merged, everything should be good to go for this one.

why don't simply add tets-requirements.txt file including dependencies references? just name that commit with prefix DON'T MERGE

Thanks for the tip :)

@jdidderen-nsi jdidderen-nsi force-pushed the 17.0-mig-account_operating_unit branch from a9cc4f5 to ba21f64 Compare June 11, 2024 08:04
Copy link

@Camille0907 Camille0907 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migration technically LGTM

@@ -76,7 +76,7 @@ def test_payment_from_two_invoices(self):
for payment in payments:
# Validate that inter OU balance move lines are created
self.assertEqual(len(payment.move_id.line_ids), 2)
self.assertAlmostEqual(payment.amount, 115000)
self.assertEqual(payment.amount, invoices[0].amount_total)

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. Since the decimal precision of the amount field is based on the currency, we should not have any difference between the amount of the invoice and the amount of the payment.

if amls:
move.with_context(wip=False).write(
move.with_context(check_move_validity=True).write(

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a key added in Odoo Community a few version ago (https://github.com/odoo/odoo/blob/96223d322c0f59b35ccda01c82e99a1ac6e2fe0c/addons/account/models/account_move.py#L1890).
The purpose is to avoid the check on the balance.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright; thank you :)

@yankinmax
Copy link

yankinmax commented Jul 8, 2024

@jdidderen-nsi can you please remove dependency commit and rebase?
operating_unit is merged:

@jdidderen-nsi jdidderen-nsi force-pushed the 17.0-mig-account_operating_unit branch from 95d72a4 to cd6a0d2 Compare July 9, 2024 14:45
@jdidderen-nsi
Copy link
Contributor Author

@yankinmax And it's rebased 👍

@hbrunn
Copy link
Member

hbrunn commented Jul 18, 2024

I was curious how you solved adding the operating unit to the analytic distribution widget, as I mentioned in the v16 migration, but it seems you didn't, correct? In my opinion this needs to be done for the migration to be complete

@jdidderen-nsi
Copy link
Contributor Author

@hbrunn It was solved directly in the analytic_operating_unit module - #669

@hbrunn
Copy link
Member

hbrunn commented Jul 18, 2024

ah nice, thanks. That would then also be a good solution for v16, will backport that eventually

Copy link
Member

@hbrunn hbrunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also cherry pick #685 so that we have feature parity

@hbrunn
Copy link
Member

hbrunn commented Jul 18, 2024

please also add d328870 for higher test coverage

@yankinmax
Copy link

@jdidderen-nsi can you pls apply @hbrunn last suggestions?

alan196 and others added 2 commits August 2, 2024 12:02
…rpart base line

This commit adds the operating unit to the cash basis counterpart base line
when cash basis accounting is enabled.
@jdidderen-nsi
Copy link
Contributor Author

please also cherry pick #685 so that we have feature parity

please also add d328870 for higher test coverage

@jdidderen-nsi can you pls apply @hbrunn last suggestions?

@hbrunn @yankinmax And it's done 👍

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@hbrunn
Copy link
Member

hbrunn commented Aug 2, 2024

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 17.0-ocabot-merge-pr-670-by-hbrunn-bump-nobump, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Aug 2, 2024
Signed-off-by hbrunn
@OCA-git-bot
Copy link
Contributor

@hbrunn your merge command was aborted due to failed check(s), which you can inspect on this commit of 17.0-ocabot-merge-pr-670-by-hbrunn-bump-nobump.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@AaronHForgeFlow
Copy link
Contributor

Issue with pre-commit on build, trying it again

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 17.0-ocabot-merge-pr-670-by-AaronHForgeFlow-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 3381cb8 into OCA:17.0 Aug 2, 2024
5 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 3583849. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.