Skip to content

[16.0][FIX] base_tier_validation: Proper notifications on reviews #1083

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 1, 2025
Merged

[16.0][FIX] base_tier_validation: Proper notifications on reviews #1083

merged 1 commit into from
Jul 1, 2025

Conversation

victoralmau
Copy link
Member

Proper notifications on reviews

Changes done:

  • The subtypes should not be defined as default=True to avoid that a follower receives notifications of everything (even if he/she doesn't really need it).
  • When adding followers they are added to the corresponding subtype so that the notification arrives to whom it corresponds.
  • Improve the code so that the reviews to notify are the "following ones according to the sequence".

@Tecnativa TT56354

@OCA-git-bot
Copy link
Contributor

Hi @LoisRForgeFlow,
some modules you are maintaining are being modified, check this out!

@victoralmau victoralmau force-pushed the 16.0-fix-base_tier_validation-TT56354 branch from 5ae9897 to 7a791cf Compare May 20, 2025 10:15
LoisRForgeFlow
LoisRForgeFlow requested changes May 20, 2025
View reviewed changes
Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

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

Just signaling that I would like to review this when ready

@victoralmau victoralmau marked this pull request as ready for review May 20, 2025 15:05
@victoralmau
Copy link
Member Author

You can review it and indicate what you think about it, although I still leave the WIP tag in the PR title until it is clear that this is the right approach.

Ping @pedrobaeza and @sergio-teruel what do you think about it?

@pedrobaeza pedrobaeza added this to the 16.0 milestone May 20, 2025
@pedrobaeza
Copy link
Member

I suppose this is to reduce the noise when you chain approvers so only the approver in the proper moment is notified, isn't it? It's a change of behavior, but it sounds reasonable.

@victoralmau victoralmau force-pushed the 16.0-fix-base_tier_validation-TT56354 branch 4 times, most recently from d556493 to 9929d4e Compare June 18, 2025 11:57
@victoralmau victoralmau force-pushed the 16.0-fix-base_tier_validation-TT56354 branch from 9929d4e to bd6e62d Compare June 30, 2025 07:51
@LoisRForgeFlow
Copy link
Contributor

Hi @victoralmau

I have checked your proposal, in general it makes sense and it is cleaner. However I'm not sure if pushing this change in v16 can disturb some installations. What do you think? Maybe it is best to do it only in 18?

@pedrobaeza
Copy link
Member

We can remove the migration scripts for not affecting existing installations and minimizing the impact.

@LoisRForgeFlow
Copy link
Contributor

We can remove the migration scripts for not affecting existing installations and minimizing the impact.

OK, let's do that, there is still impact with the rest but at the end of the day this behavior makes more sense.

@victoralmau
[FIX] base_tier_validation: Proper notifications on reviews
19e0337 
Changes done:
- The subtypes should not be defined as default=True to avoid that a follower receives notifications of everything (even if he/she doesn't really need it).
- When adding followers they are added to the corresponding subtype so that the notification arrives to whom it corresponds.
- Improve the code so that the reviews to notify are the "following ones according to the sequence".

TT56354
@victoralmau victoralmau force-pushed the 16.0-fix-base_tier_validation-TT56354 branch from bd6e62d to 19e0337 Compare July 1, 2025 07:27
@victoralmau victoralmau changed the title [16.0][WIP] base_tier_validation: Proper notifications on reviews [16.0][FIX] base_tier_validation: Proper notifications on reviews Jul 1, 2025
LoisRForgeFlow
LoisRForgeFlow approved these changes Jul 1, 2025
View reviewed changes
Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

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

👍

pedrobaeza
pedrobaeza approved these changes Jul 1, 2025
View reviewed changes
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

/ocabot merge major

@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-1083-by-pedrobaeza-bump-major, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 75740b9 into OCA:16.0 Jul 1, 2025
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@pedrobaeza pedrobaeza deleted the 16.0-fix-base_tier_validation-TT56354 branch July 1, 2025 09:47
@victoralmau victoralmau mentioned this pull request Jul 1, 2025
Merged
@LoisRForgeFlow LoisRForgeFlow mentioned this pull request Jul 2, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedrobaeza pedrobaeza pedrobaeza approved these changes

@LoisRForgeFlow LoisRForgeFlow LoisRForgeFlow approved these changes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
16.0
Development

Successfully merging this pull request may close these issues.
4 participants
@victoralmau @OCA-git-bot @pedrobaeza @LoisRForgeFlow