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][ADD]mute_notification_user_autosubscribe: Do not send notification to users autosubscribed through user_id field #1460

Conversation

manuelregidor
Copy link
Contributor

@manuelregidor manuelregidor commented Oct 9, 2024

@HaraldPanten @ValentinVinagre

@luis-ron Could you review, please?

T-6499

@pedrobaeza pedrobaeza added this to the 17.0 milestone Oct 9, 2024
@pedrobaeza
Copy link
Member

Please avoid plurals in module names, and more in the middle of the name. That's food for constant typos.

@manuelregidor manuelregidor force-pushed the 17.0-add-mute_notifications_user_autosubscribe branch from 6a9a3d4 to 17dde9e Compare October 9, 2024 10:12
@manuelregidor
Copy link
Contributor Author

@pedrobaeza done

@manuelregidor manuelregidor changed the title [17.0][ADD]mute_notifications_user_autosubscribe: Do not send notification to users autosubscribed trhough user_id field [17.0][ADD]mute_notifications_user_autosubscribe: Do not send notification to users autosubscribed through user_id field Oct 10, 2024
@HaraldPanten
Copy link

@manuelregidor Could you adapt the commit and the PR names, considering the correct module name, please? mute_notifications_user_autosubscribe --> mute_notification_user_autosubscribe

@manuelregidor manuelregidor changed the title [17.0][ADD]mute_notifications_user_autosubscribe: Do not send notification to users autosubscribed through user_id field [17.0][ADD]mute_notification_user_autosubscribe: Do not send notification to users autosubscribed through user_id field Oct 11, 2024
@manuelregidor manuelregidor force-pushed the 17.0-add-mute_notifications_user_autosubscribe branch from 17dde9e to 05d89e0 Compare October 11, 2024 05:34
@manuelregidor
Copy link
Contributor Author

@HaraldPanten done

@HaraldPanten
Copy link

Let's wait for @luis-ron 's functional review, then.

Copy link

@luis-ron luis-ron left a comment

Choose a reason for hiding this comment

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

Functional review: LGTM 👍🏻

@manuelregidor
Copy link
Contributor Author

@Tisho99 Could you review, please?

Copy link

@Tisho99 Tisho99 left a comment

Choose a reason for hiding this comment

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

LGTM. Technical review

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

Copy link
Contributor

@ValentinVinagre ValentinVinagre left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@HaraldPanten
Copy link

@pedrobaeza @etobella your point of view here would be nice.

Could you give us a hand?

THX

@manuelregidor manuelregidor force-pushed the 17.0-add-mute_notifications_user_autosubscribe branch from 05d89e0 to b7d0a97 Compare October 23, 2024 07:15
@manuelregidor
Copy link
Contributor Author

@pedrobaeza Changes applied

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.

Well, this is replacement one table (the previous one) by another (the followers one). The question is why can't you determine dynamically in the moment of sending the notification if it should be sent or not.

@manuelregidor
Copy link
Contributor Author

@pedrobaeza We decided to use subtypes to avoid notificactions, so the muting subtype is applied only once, when the user in field user_id is autosubscribed, and after that, notifications will not be sent to this user unless subtypes in the document are changed, with no need of further checks. By doing it this way:

  • It is easy to keep a user subscribed to the document while not receiving notifications.
  • In case the user needs to receive notifications, the subtypes can be easily changed.
  • We can check which contacts are being sent notifications and which are not in each document.

I hope that answers your question.

@pedrobaeza
Copy link
Member

As said, I don't think this is the way to go, as if you change any mute rule, all the existing documents won't have such configuration, but if you want to go this way, put it on the ROADMAP and I won't block it.

@HaraldPanten
Copy link

HaraldPanten commented Oct 26, 2024

As said, I don't think this is the way to go, as if you change any mute rule, all the existing documents won't have such configuration, but if you want to go this way, put it on the ROADMAP and I won't block it.

@pedrobaeza

The use case is:

  • We have a Sales team that prepare lots of quotations and sales orders (after confirmation).
  • Once these sales orders are processed (materials are delivered, etc.) we create the corresponding invoices.
  • When the Accounting team creates the invoice, the Salesperson assigned to the invoice musn't receive notifications when the invoices are sent by email.
  • We need to keep the Salesperson as a follower, but they should not receive notifications coming from the invoices, as it is an Accounting issue, not Sales.

Do you know what I mean?

I'm not sure about what you mean. Could you give us more details and which would be a better approach for this situation?

THX

@pedrobaeza
Copy link
Member

As said, if you change any mute rule, all the existing documents won't have such configuration, but it's just a corner case. You can continue putting that case in the ROADMAP.

…o users autosubscribed through user_id field
@manuelregidor manuelregidor force-pushed the 17.0-add-mute_notifications_user_autosubscribe branch from b7d0a97 to 8b31c5e Compare October 28, 2024 08:33
@manuelregidor
Copy link
Contributor Author

@pedrobaeza Case included in ROADMAP.

@pedrobaeza
Copy link
Member

OK, you can go ahead then.

@HaraldPanten
Copy link

@pedrobaeza Could you merge it, please? I'm not PSC of this repository.

THX!

@etobella
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 17.0-ocabot-merge-pr-1460-by-etobella-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit a782294 into OCA:17.0 Oct 28, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@HaraldPanten HaraldPanten deleted the 17.0-add-mute_notifications_user_autosubscribe branch October 28, 2024 10:00
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.

8 participants