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

[17.0][MIG] account_analytic_distribution_manual: Migration to 17.0 #700

Open
wants to merge 18 commits into
base: 17.0
Choose a base branch
from

Conversation

BernatObrador
Copy link
Contributor

@BernatObrador BernatObrador commented Oct 10, 2024

Module migrated to version 17.0

cc https://github.com/APSL 161694

@miquelalzanillas @lbarry-apsl @mpascuall @peluko00 @javierobcn @ppyczko please review

@BernatObrador BernatObrador force-pushed the 17.0-mig-account_analytic_distribution_manual branch from 5febd1f to 5ddf248 Compare October 11, 2024 11:40
@BernatObrador BernatObrador mentioned this pull request Oct 15, 2024
18 tasks
@BernatObrador BernatObrador force-pushed the 17.0-mig-account_analytic_distribution_manual branch 6 times, most recently from 527842e to 4a11424 Compare October 18, 2024 06:11
@BernatObrador
Copy link
Contributor Author

Hi @pedrobaeza!

I hope you're doing well! I’m facing some issues with the tests as they’re failing due to the tour, and I'm not sure how to resolve it. If you could lend a hand, I would really appreciate it!

Thank you in advance!

@pedrobaeza
Copy link
Member

Not sure. We had a similar problem some weeks ago that was solved with this: OCA/oca-ci#78

@sbidoul @chienandalu do you have any clue?

@BernatObrador
Copy link
Contributor Author

Thanks, @pedrobaeza! If there's anything I can assist with, just let me know.

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

You need to change the tour first

@BernatObrador BernatObrador force-pushed the 17.0-mig-account_analytic_distribution_manual branch 2 times, most recently from be2a906 to 1c2324c Compare November 7, 2024 09:06
@BernatObrador
Copy link
Contributor Author

Hi @chienandalu! Thank you very much for your contribution! The changes have been applied, but now the tour fails at the step "Create new invoice" with the trigger .o_list_button_add.

I'll take a closer look at this and see if I can resolve it. Thanks again for your help!

@chienandalu
Copy link
Member

Yes, now it's just a matter of debugging the steps 😉

@BernatObrador BernatObrador force-pushed the 17.0-mig-account_analytic_distribution_manual branch from 1c2324c to f87beaa Compare November 7, 2024 10:29
@BernatObrador BernatObrador marked this pull request as draft November 7, 2024 10:29
@BernatObrador BernatObrador force-pushed the 17.0-mig-account_analytic_distribution_manual branch from f87beaa to 379c043 Compare November 7, 2024 10:37
@BernatObrador BernatObrador force-pushed the 17.0-mig-account_analytic_distribution_manual branch 2 times, most recently from 158bf67 to e90815d Compare November 22, 2024 09:29
@BernatObrador BernatObrador force-pushed the 17.0-mig-account_analytic_distribution_manual branch from e90815d to 6e9004d Compare December 10, 2024 12:19
@BernatObrador
Copy link
Contributor Author

The tour tests are now working correctly. The issue lies with the tests themselves. I’m not sure why, but when running the tour directly through Odoo, everything works fine: analytic entries are generated as expected, and the other tests also pass without issues. However, when running Odoo tests that include the tour, it fails because it's not generating the analytic as intended. Do yo have any clue about why this can happen? Thanks in advance! @pedrobaeza @chienandalu

@pedrobaeza
Copy link
Member

Maybe @carlos-lopez-tecnativa can say.

@BernatObrador BernatObrador force-pushed the 17.0-mig-account_analytic_distribution_manual branch from 6e9004d to 91623fe Compare December 17, 2024 12:08
@BernatObrador BernatObrador marked this pull request as ready for review December 17, 2024 12:14
Copy link

@ppyczko ppyczko left a comment

Choose a reason for hiding this comment

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

LGTM, tested in runboat and code review 👍

@BernatObrador BernatObrador force-pushed the 17.0-mig-account_analytic_distribution_manual branch 2 times, most recently from 501221c to f020273 Compare December 19, 2024 18:54
@BernatObrador
Copy link
Contributor Author

@carlos-lopez-tecnativa Thanks a lot for your time and valuable feedback! The bugs have been fixed. Could you please take another look and review the changes?

@carlos-lopez-tecnativa
Copy link

@carlos-lopez-tecnativa Thanks a lot for your time and valuable feedback! The bugs have been fixed. Could you please take another look and review the changes?

I tested again, but the issues are still present. Could you review it once more? I’m not sure if network latency on Runboat could be related.

2024-12-23.15-02-42.mp4

@BernatObrador
Copy link
Contributor Author

BernatObrador commented Dec 24, 2024

Grabación de pantalla desde 2024-12-24 07-43-43.webm
It's working fine when I try it on Runboat, and my team tested it as well without any issues. One person did encounter a similar problem, but they resolved it by using an incognito tab. Could you please try using an incognito tab as well? It might be related to cookies or cached data causing the issue.

@BernatObrador BernatObrador force-pushed the 17.0-mig-account_analytic_distribution_manual branch from f020273 to 9e8c2aa Compare December 27, 2024 07:36
@BernatObrador
Copy link
Contributor Author

@carlos-lopez-tecnativa Can you review again? Thanks for your time!

Copy link

@carlos-lopez-tecnativa carlos-lopez-tecnativa left a comment

Choose a reason for hiding this comment

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

Tested and working fine. Please squash administrative commits(see https://github.com/OCA/maintainer-tools/wiki/Merge-commits-in-pull-requests). Thanks for the effort in the migration.

carlos-lopez-tecnativa and others added 18 commits January 8, 2025 08:09
…complete analytic accounts on any model that has a field for analytic accounts.
Currently translated at 95.6% (22 of 23 strings)

Translation: account-analytic-16.0/account-analytic-16.0-account_analytic_distribution_manual
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-16-0/account-analytic-16-0-account_analytic_distribution_manual/es/
…tics account from manual distribution

- Show the current distribution selected at the top of the tags
- When unselecting a distribution, all related analytic accounts are also unselected
Currently translated at 100.0% (26 of 26 strings)

Translation: account-analytic-16.0/account-analytic-16.0-account_analytic_distribution_manual
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-16-0/account-analytic-16-0-account_analytic_distribution_manual/it/
…teger to many2one

- If the user needs to group by this field, it was impossible when it was an integer.
- Added a script to populate the new field in models related to account_analytic_tags.
Note: When multiple tags are present, only the last record is populated.
Currently translated at 100.0% (26 of 26 strings)

Translation: account-analytic-16.0/account-analytic-16.0-account_analytic_distribution_manual
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-16-0/account-analytic-16-0-account_analytic_distribution_manual/it/
Currently translated at 100.0% (27 of 27 strings)

Translation: account-analytic-16.0/account-analytic-16.0-account_analytic_distribution_manual
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-16-0/account-analytic-16-0-account_analytic_distribution_manual/it/
…export/import of analytical distributions

TT50235
Currently translated at 100.0% (29 of 29 strings)

Translation: account-analytic-16.0/account-analytic-16.0-account_analytic_distribution_manual
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-16-0/account-analytic-16-0-account_analytic_distribution_manual/it/
Currently translated at 75.0% (24 of 32 strings)

Translation: account-analytic-16.0/account-analytic-16.0-account_analytic_distribution_manual
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-16-0/account-analytic-16-0-account_analytic_distribution_manual/es/
Currently translated at 100.0% (32 of 32 strings)

Translation: account-analytic-16.0/account-analytic-16.0-account_analytic_distribution_manual
Translate-URL: https://translation.odoo-community.org/projects/account-analytic-16-0/account-analytic-16-0-account_analytic_distribution_manual/it/
…export/import to analytic.mixin

Move the functionality to the analytic.mixin model so that it can also be used in account.move.line records

TT50944
@BernatObrador BernatObrador force-pushed the 17.0-mig-account_analytic_distribution_manual branch from 9e8c2aa to 60f773d Compare January 8, 2025 07:09
@BernatObrador
Copy link
Contributor Author

@carlos-lopez-tecnativa Done!

Copy link

@carlos-lopez-tecnativa carlos-lopez-tecnativa left a comment

Choose a reason for hiding this comment

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

Could you review this use case? It’s not working as expected

Comment on lines +38 to +46
// When record is created, and manual_distribution_id is cleared
// but user discard changes, we need to refresh the manual distribution
const force_refresh_discart = new_manual_distribution_id === 0 && record_id > 0;
if (
manual_distribution_Changed &&
(new_manual_distribution_id > 0 || force_refresh_discart)
) {
await this.refreshManualDistribution(new_manual_distribution_id);
}

Choose a reason for hiding this comment

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

  • Create an invoice, add a manual distribution, and save.
  • Edit the invoice and remove the manual distribution.
  • Focus on another element.
  • Discard the changes.

Issue: The manual distribution is not displayed; it’s lost after discarding the changes

V17

V17.account_analytic_distribution_manual.mp4

V16

V16.account_analytic_distribution_manual.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue lies in the fact that the method willUpdate(nextProps) has been replaced with willUpdateRecord(record).

The challenge is that I can no longer determine whether force_refresh_discart should be applied, as this.props no longer includes this.props.record.data.id. This makes it impossible to track whether the user is discarding changes.

Do you have any suggestions on how to resolve this? I specifically need a reliable way to detect when the user discards changes so that I can apply the correct distribution.

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

Successfully merging this pull request may close these issues.

9 participants