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

[16.0][mig] sale_timesheet_invoice_description: Migration to 16.0 #1475

Merged

Conversation

dsolanki-initos
Copy link

No description provided.

@dsolanki-initos dsolanki-initos force-pushed the 16.0-mig-sale_timesheet_invoice_description branch from e4cc071 to a880cd7 Compare June 9, 2023 10:42
Copy link
Member

@victor-champonnois victor-champonnois left a comment

Choose a reason for hiding this comment

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

Functionnal review OK


timesheets = self.env["account.analytic.line"].sudo().search(domain)
timesheets.write({"timesheet_invoice_line_id": aml.id})

def _check_balanced(self):
@contextmanager
Copy link
Member

Choose a reason for hiding this comment

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

Why adding this? And why we need to override this check in first place? Maybe the reason has gone or it should be implemented in other way.

Choose a reason for hiding this comment

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

It comes from this commit 98c253e . It looks like splitting the account move line by timesheet breaks the debit/credit balance by line.

Choose a reason for hiding this comment

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

I'm afraid I don't know account.move well enough to suggest a better way.

Copy link
Member

@azoellner azoellner Oct 24, 2023

Choose a reason for hiding this comment

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

The new (in comparison to v15) @contextmanager here is needed, because the super method in the Odoo v16 core got one in the reworked accounting code, in commit odoo/odoo@d8d47f9.

However, the super call is implemented incorrectly here (a couple of lines below). It should read something line with super()._check_balanced(container): yield. The flake8-bugbear warning B901, suppressed here with # noqa, points to that issue.

👉 I am fixing this. [Update:] Fixed in 4e2d0ce.

* `Akretion <https://www.akretion.com>`_:

* Clément Mombereau <clement.mombereau@akretion.com.br>
* Dhara Solanki <dhara.solanki@initos.com>
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect indentation. This way, it's inside Akretion section.

Copy link
Member

@azoellner azoellner Oct 24, 2023

Choose a reason for hiding this comment

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

👉 I am fixing this. [Update:] Fixed in 4e2d0ce.

Thank you for your advice.

odooNextev pushed a commit to odooNextev/account-invoicing that referenced this pull request Sep 29, 2023
Signed-off-by rafaelbn
@tbaden
Copy link
Member

tbaden commented Oct 11, 2023

@dsolanki-initos can you have a look into the requested changes

carlosdauden and others added 25 commits October 24, 2023 16:55
[ADD] sale_timesheet_invoice_description: New module
There's a constraint that limits the quantity over received one. Although it seems invalid
on services, we bypass it now with this.
Currently translated at 100,0% (10 of 10 strings)

Translation: account-invoicing-11.0/account-invoicing-11.0-sale_timesheet_invoice_description
Translate-URL: https://translation.odoo-community.org/projects/account-invoicing-11-0/account-invoicing-11-0-sale_timesheet_invoice_description/de/
Keep the previous behavior showing only date
Remove order parameter in sear method
Updated by Update PO files to match POT (msgmerge) hook in Weblate.
Currently translated at 100.0% (10 of 10 strings)

Translation: account-invoicing-12.0/account-invoicing-12.0-sale_timesheet_invoice_description
Translate-URL: https://translation.odoo-community.org/projects/account-invoicing-12-0/account-invoicing-12-0-sale_timesheet_invoice_description/es/
Currently translated at 100.0% (10 of 10 strings)

Translation: account-invoicing-12.0/account-invoicing-12.0-sale_timesheet_invoice_description
Translate-URL: https://translation.odoo-community.org/projects/account-invoicing-12-0/account-invoicing-12-0-sale_timesheet_invoice_description/de/
Currently translated at 100.0% (10 of 10 strings)

Translation: account-invoicing-12.0/account-invoicing-12.0-sale_timesheet_invoice_description
Translate-URL: https://translation.odoo-community.org/projects/account-invoicing-12-0/account-invoicing-12-0-sale_timesheet_invoice_description/pt_BR/
@azoellner azoellner force-pushed the 16.0-mig-sale_timesheet_invoice_description branch from a880cd7 to 4e2d0ce Compare October 25, 2023 07:47
@azoellner
Copy link
Member

@pedrobaeza (@robinkeunen @tbaden) I now fixed the issues that you mentioned above, did run pre-commit afterwards, and finally rebased this feature branch onto the current head of its target "16.0".

@pedrobaeza
Copy link
Member

pedrobaeza commented Oct 25, 2023

Great, now it's available for reviewers. Check https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#review for knowing the process. You can review other PRs and ask in exchange that they review yours.

/ocabot migration sale_timesheet_invoice_description

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Oct 25, 2023
@OCA-git-bot OCA-git-bot mentioned this pull request Oct 25, 2023
60 tasks
@azoellner
Copy link
Member

azoellner commented Oct 26, 2023

@pedrobaeza While working on migrating the feature "split-by-task" (see PR #1501 for version 15.0, which is still on my agenda), I found and fixed some issues related to version 16.0 core changes (to timesheets and accounting), pushed in fafcaef.

P.S. Note that the aforementioned PR #1501 does contain further fixes, unrelated to the migration itself. I did not include them here, as imho this migration branch should only be concerned with migrating the version 15.0 code, not fixing it.

Copy link

@robinkeunen robinkeunen left a comment

Choose a reason for hiding this comment

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

👍 Last 3 commits sould be squashed IMO.

Thx !

@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). 🤖

* adapted code to version 16.0 changes; for details see below
* added contributors
* updated i18n: sale_timesheet_invoice_description.pot, de.po
* run `pre-commit`

Linking timesheets to invoice lines: Respecting both the specified (in
context) start and end time, the same way as it is done for linking them
to the invoice itself.

Reworked preventing unbalanced invoice: Invoices must not and cannot be
unbalanced. This is now (in 16.0) done automatically. I.e., the
intermediate invoices, created while splitting lines, will no longer be
possibly unbalanced.
@azoellner azoellner force-pushed the 16.0-mig-sale_timesheet_invoice_description branch from fafcaef to b419341 Compare October 26, 2023 12:34
@azoellner
Copy link
Member

@robinkeunen I squashed the last three commits into one, as you suggested. Thx.

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit 7e7b7fa into OCA:16.0 Oct 26, 2023
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@azoellner azoellner deleted the 16.0-mig-sale_timesheet_invoice_description branch October 26, 2023 21:57
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.

None yet