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

[15.0][IMP]base_tier_validation: only post notifications to reciepients #732

Closed
wants to merge 1 commit into from

Conversation

bosd
Copy link
Contributor

@bosd bosd commented Oct 22, 2023

Before this PR there where double tier review notifications when used in combination with tier_validation_waiting.

image

One of the messages has no recepients:
image

After this PR:
Only one notification gets posted.
(The one adressed to the requested reviewer)
image

@OCA-git-bot
Copy link
Contributor

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

@LoisRForgeFlow
Copy link
Contributor

@bosd something is wrong in the tests, can you have a look?

@bosd
Copy link
Contributor Author

bosd commented Oct 23, 2023

@bosd something is wrong in the tests, can you have a look?

@LoisRForgeFlow I noticed.
I'm working on it right now: #733

Yet it is a bit tricky. As I cannot replicate it on my local system.
Do you have any ideas?

@LoisRForgeFlow
Copy link
Contributor

@bosd something is wrong in the tests, can you have a look?

@LoisRForgeFlow I noticed. I'm working on it right now: #733

Yet it is a bit tricky. As I cannot replicate it on my local system. Do you have any ideas?

Not really, I cannot reproduce it locally either...

@bosd bosd force-pushed the 15.0-fix-base_tier_validation branch from 4be2934 to 8097e59 Compare October 25, 2023 20:06
@bosd bosd changed the title [IMP]base_tier_validation: only post notifications to reciepients [15.0][IMP]base_tier_validation: only post notifications to reciepients Nov 2, 2023
Copy link

@Dranyel-Bosd Dranyel-Bosd left a comment

Choose a reason for hiding this comment

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

functionally review 👍

Copy link

@bofiltd bofiltd left a comment

Choose a reason for hiding this comment

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

LGTM

@bosd
Copy link
Contributor Author

bosd commented Dec 28, 2023

Why does the @OCA-git-bot not attach the ready to merge label?

@bosd bosd force-pushed the 15.0-fix-base_tier_validation branch from 7579207 to c8e6643 Compare December 30, 2023 12:15
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.

Hi

I have just reviewed this PR, sorry for the delay.

I do not agree with this fix:

  1. When you restart a validation, and request again, you will get no message now in the chatter.

image

  1. You are trying to fix a problem in the interaction with base_tier_validation_waiting, however this module is not even available in version 15.0.

@bosd
Copy link
Contributor Author

bosd commented Jan 4, 2024

Thanks for the review..

  1. When you restart a validation, and request again, you will get no message now in the chatter.

Can you check if the new message after restarting the validation is actually sent out to someone?
(Assuming that it is a message without a recepeint) Maybe we can solve this in another way.

2. You are trying to fix a problem in the interaction with base_tier_validation_waiting, however this module is not even available in version 15.0.

It is not yet merged in V15, but there is a PR in #638

The tier validation waiting module really adds some usefull features for sequential review.
But it also changes the base module drastically which makes it hard to test/maintain.
I've proposed to merge it into the base base_tier_validation module.
tracked in: #787 (comment)
Can you please provide some feedback on that idea there?

Copy link

github-actions bot commented May 5, 2024

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label May 5, 2024
@github-actions github-actions bot closed this Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants