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] base_tier_validation: Migration to 17.0 #787

Merged
merged 157 commits into from
Jan 10, 2024

Conversation

celm1990
Copy link
Contributor

superseded by #755

LoisRForgeFlow and others added 22 commits December 5, 2023 07:42
* 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.
@celm1990
Copy link
Contributor Author

@bosd @moitabenfdz @TonyMasciI @Duranio @pedrobaeza @gurneyalex
The PR is ready for review. I'll squash the commits before merging. For now, it is keep for an easy technical review.

@bosd
Copy link
Contributor

bosd commented Dec 28, 2023

@celm1990 Thanks so much for this PR.
Can you please include: 127ec31

In the other PR, I made a comment with the propossal to integrate the module base_tier_validation_waiting into this base module.
There was no feedback yet...
What is your opinion on this?

Motivation:

  1. The base_tier_validation_waiting module might be "too simple" to be an independent module.
  2. Mainting it separatly is kinda "costly" it needs some patches to make the unit tests pass. (by either separating the test of the module, or patching all other tier_validation modules)
  3. Usability, when making (sequential) tier reviews it makes sense to have an clearoverview on the approval flow.
    (Sequence approval is implemented in this base module as well)

@celm1990 celm1990 force-pushed the 17.0-mig-base_tier_validation branch from 9a299fb to be4ecae Compare December 28, 2023 20:37
@celm1990
Copy link
Contributor Author

@celm1990 Thanks so much for this PR. Can you please include: 127ec31

cherry-pick done

In the other PR, I made a comment with the propossal to integrate the module base_tier_validation_waiting into this base module. There was no feedback yet... What is your opinion on this?

I apologize, but I haven't worked with that module before, and I'm not familiar with its functionality. Additionally, I couldn't find available in versions 16 or 15. Introducing it might require additional migration efforts. I suggest focusing on the current migration for now. In a subsequent pull request, you could propose the integration of that module directly.

@bosd
Copy link
Contributor

bosd commented Dec 28, 2023

I apologize, but I haven't worked with that module before, and I'm not familiar with its functionality.

Sorry, forgot to mention a little bit of background info...

What the module does is quite simple. It adds another validation status waiting.
When reaching that status a notification can be sent out.
Use case: In sequential approval flow, you only want to sent out a notification to the approver when it is 'his' turn.

It's really small change, but because it is adding a status. It's kinda problematic and breaking tests.

Currently I have PR's open for base_tier_validation_waiting for version 15 and 16. (for quite some while now)
As per discussion in the pr of V15 #638 it become clear that unit tests of that module is kinda problematic.

I couldn't find available in versions 16 or 15. Introducing it might require additional migration efforts.

The testing problem
Could likely be the cause for not yet having this module merged into 15 and 16 branches.
There is no clean, neat and "oca" way to test it. I've discussed this with some dev's at the OXP 2023.
The options:

  1. Change functionality when in test context (Dirty but works, current implementation)
  2. Create an isolated testing runboat (very dirty hack. and not a real integration test)

The added actual migration effort is a good thing. 🤓
The best time to refactor is in a migration. As all the code for the depending modules need to be migrated anyway.
If we include it now, we take the pain once, and we know that all depending modules for V17 and onwards are compatible.

In a subsequent pull request, you could propose the integration of that module directly.

Doing the refactor in the "middle" of a release will break tests, CI/CD.
(Not only here on the OCA repo's but also on other partners). We will encounter issues when we have to versions of the base_tier_validation module.

Sorry, for thinking out loud a bit 😉
The more I think of it, I'm convinced it should be done in the migration to V17.

Preferrably, I would like to have an opinion on this matter from of one the more OCA expeirenced members / heavyweights. With a deeper understanding of these modules.
Is that where the PSC's are for??

We could also drag this "issue" along to next versions.
(But as V17 is not used in production yet, we might have a chance here.)

@celm1990 celm1990 force-pushed the 17.0-mig-base_tier_validation branch from 285ade5 to bb5723c Compare January 9, 2024 05:06
@celm1990
Copy link
Contributor Author

celm1990 commented Jan 9, 2024

@celm1990 You might need to squash all migration commits into one commit.

Done

@bosd
Copy link
Contributor

bosd commented Jan 9, 2024

Please include #724

@@ -0,0 +1,593 @@
<?xml version="1.0" encoding="utf-8"?>

Choose a reason for hiding this comment

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

I faced this warning, should remove this line


2024-01-09 08:42:30,778 12362 INFO v17c_base_tier_validation odoo.modules.registry: module base_tier_validation_formula: creating or updating database tables 
2024-01-09 08:42:30,872 12362 WARNING v17c_base_tier_validation py.warnings: /home/chien/code/odoo/odoo/17.0/odoo/addons/base/models/ir_module.py:170: DeprecationWarning: XML declarations in HTML module descriptions are deprecated since Odoo 17, base_tier_validation_formula can just have a UTF8 description with not need for a declaration.

Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be fixed when repo update in #798 is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a warning, @pedrobaeza do you have any plans to update the global OCA update copier after merging this PR OCA/oca-addons-repo-template#239?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I didn't remember that it was fixed at maintainer-tools level. @sbidoul has the token to update with the bot all the branches.

@celm1990 celm1990 force-pushed the 17.0-mig-base_tier_validation branch from bb5723c to 5be3bb2 Compare January 9, 2024 22:12
@celm1990
Copy link
Contributor Author

celm1990 commented Jan 9, 2024

@nguyenminhchien could you please update your review

@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 17.0-ocabot-merge-pr-787-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit f01e2ac into OCA:17.0 Jan 10, 2024
@OCA-git-bot
Copy link
Contributor

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

@bosd
Copy link
Contributor

bosd commented Jan 10, 2024

Oops, this merge went quick.
The V17 comments where not yet adressed.
I was about to open the merge pr based on this one this evening.

Well, we can discuss that merge later in that pr 😉

Anyway big thanks @celm1990 for all your work on this pr!

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.