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] base_tier_validation: migration to 16 #540

Merged
merged 108 commits into from
Dec 30, 2022

Conversation

bizzappdev
Copy link
Contributor

No description provided.

@celm1990
Copy link
Contributor

Any plan to improve performance??
please see #496

@bizzappdev
Copy link
Contributor Author

Any plan to improve performance?? please see #496

Not in this MR.
Prio is the Migration first and then might be new MR for improvement of performance.

@bizzappdev bizzappdev force-pushed the 16.0-mig-base_tier_validation branch from 525a7a9 to ae420e4 Compare November 1, 2022 09:16
LoisRForgeFlow and others added 27 commits November 1, 2022 14:52
* able to restart validation
* sudo() not needed anymore
and reject can be hidden according to this computed field.
… and who asks for reviews in new fields 'done_by' and 'requested_by'.
fixup and extend tests

[ADD] systray icon for pending reviews

[FIX] Remove python safe_eval

[ADD] base_tier_validation_formula and migration scripts

[ADD] widget domain and python expression to define reviewer in tier definition

[ADD] auto updating of systray icon counter

[ADD] validation date field

[ADD] review widget dropdown menu
…reviews' name and state correctly translated.
* using similar approach to activities has already benn addressed.
* add a new point explaining review tooltip improvement possibilities.
Currently translated at 100.0% (59 of 59 strings)

Translation: server-ux-12.0/server-ux-12.0-base_tier_validation
Translate-URL: https://translation.odoo-community.org/projects/server-ux-12-0/server-ux-12-0-base_tier_validation/es/
@StefanRijnhart
Copy link
Member

Please check the js linting

 error  Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

@bizzappdev
Copy link
Contributor Author

bizzappdev commented Nov 23, 2022

Please check the js linting

 error  Parsing error: 'import' and 'export' may appear only with 'sourceType: module'

I really do not have much idea about how to fix this. as in the entire odoo also they have imported like this only. as some suggestions are changing .eslintrc file. but that will be needed at 16.0 level.

@StefanRijnhart
Copy link
Member

I really do not have much idea about how to fix this. as in the entire odoo also they have imported like this only. as some suggestions are changing .eslintrc file. but that will be needed at 16.0 level.

You can fix the errors by renaming the .js files to .esm.js files as mentioned in the 15.0 migration issue (https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-15.0#tasks-to-do-in-the-migration). This will trigger further eslint messages that need to be followed up.

Copy link
Member

@StefanRijnhart StefanRijnhart left a comment

Choose a reason for hiding this comment

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

Thanks for the huge work migrating this module and all its javascript! I tested the module successfully. The code still needs some work on the JS linting.

@rven
Copy link

rven commented Nov 29, 2022

``> Thanks for the huge work migrating this module and all its javascript! I tested the module successfully. The code still needs some work on the JS linting.

@StefanRijnhart
We discovered the same problem with the linting

I think the solution for the JS linting is the following:

parserOptions:
  ecmaVersion: 2019

overrides:
  - files:
      - "**/*.esm.js"
    parserOptions:
      sourceType: module

Should be adapted to

parserOptions:
  ecmaVersion: 2019
  sourceType: module

overrides:
  - files:
      - "**/*.esm.js"
    parserOptions:
      sourceType: module

After this change, the linting error will dissapear.

@pedrobaeza
Copy link
Member

JS files should be renamed to .esm.js if they use the new ES6 syntax.

@bizzappdev bizzappdev force-pushed the 16.0-mig-base_tier_validation branch from 8c6ed64 to 1347ee4 Compare December 6, 2022 09:28
@bizzappdev
Copy link
Contributor Author

Pre-commits are fixed. you can review it again.
code coverage is already more than 90%. should we increase the coverage?
@StefanRijnhart

@StefanRijnhart
Copy link
Member

Thanks for the js updates! Looking good. Please check my version number comment. It's always good to improve coverage, but it's going to be hard to make that 96% mark. I had a look at what's still missing coverage, and I don't see anything critical.

@bizzappdev bizzappdev force-pushed the 16.0-mig-base_tier_validation branch from 1347ee4 to 145c570 Compare December 6, 2022 10:39
Copy link
Member

@StefanRijnhart StefanRijnhart left a comment

Choose a reason for hiding this comment

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

Thanks! Tested + code reviewed.

@StefanRijnhart
Copy link
Member

/ocabot migration base_tier_validation

Copy link
Contributor

@alan196 alan196 left a comment

Choose a reason for hiding this comment

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

Tested functionally 👍

@alan196
Copy link
Contributor

alan196 commented Dec 23, 2022

I tested the module in a custom model, and I get an issue when I try to click on the notification in the tray menu.

It looks that is an issue of Odoo, I made the following PR odoo/odoo#108609 to fix this issue.

I post this in case you face the same issue 😄

@dreispt
Copy link
Member

dreispt commented Dec 30, 2022

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-540-by-dreispt-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit fae2da3 into OCA:16.0 Dec 30, 2022
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 0b6c4e4. 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.