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

feat: generalize receiver logic in notifications #24725

Conversation

blaggacao
Copy link
Contributor

@blaggacao blaggacao commented Feb 4, 2024

towards: #23700

Context

Prior to this PR, the receiver logic was complicated but not generalized resulting in significant
code duplication as soon as anoter channel had been implemented (e.g. Matrix Chat).

However, the principled logic of discovering receivers is generally the same:

  • Is there a User which has a specific field representing the reciver id
  • Or is there a Data field with a specific option setting representing an ad-hoc receiver id (doc-specifc room/channel, ad-hoc user, etc)

Proposed Solution

  • generalize the involved functions to be more reusable by other implementations
  • expand the SMS/Whatsapp receiver logic to include User link fields (as is already the case with emails)
  • allow options == Mobile and (for backward compat) options == Phone interchangeably

Risk / Reward

  • This is designed to be fully backwards compatible
  • This code is being used for a Matrix Chat implementation which currently lives on a private feature branch

no-docs

@blaggacao blaggacao requested review from a team and ankush and removed request for a team February 4, 2024 11:26
@blaggacao blaggacao force-pushed the chore/generalize-notification-receiver-fields branch from f14ccab to 8fd9550 Compare February 4, 2024 11:34
Copy link

codecov bot commented Feb 4, 2024

Codecov Report

Attention: Patch coverage is 37.14286% with 22 lines in your changes missing coverage. Please review.

Project coverage is 61.96%. Comparing base (9a809f1) to head (8fd9550).
Report is 99 commits behind head on develop.

Current head 8fd9550 differs from pull request most recent head 75dcf88

Please upload reports for the commit 75dcf88 to get more accurate results.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #24725      +/-   ##
===========================================
- Coverage    62.05%   61.96%   -0.09%     
===========================================
  Files          801      787      -14     
  Lines        77104    75230    -1874     
  Branches      6465     6430      -35     
===========================================
- Hits         47847    46619    -1228     
+ Misses       25510    24909     -601     
+ Partials      3747     3702      -45     
Flag Coverage Δ
server 70.94% <37.14%> (-0.49%) ⬇️
server-ui 27.09% <5.71%> (+1.27%) ⬆️
ui-tests 50.79% <ø> (+0.51%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link

stale bot commented Mar 2, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added inactive and removed inactive labels Mar 2, 2024
@blaggacao
Copy link
Contributor Author

@ankush / @NagariaHussain Is that something that could be of interest in upstream? I needed this for Matrix integration, but it's entirely possible that this is useful for Raven Chat, too.

Copy link

stale bot commented Mar 23, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@blaggacao
Copy link
Contributor Author

In our deployment, this PR works together with frappe/erpnext#39638

@blaggacao
Copy link
Contributor Author

@ankush since this is now "soft blocking" frappe/erpnext#39638 could you maybe help or help to find help with this? That would be much appreciated. 😄

Copy link

stale bot commented Apr 27, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Apr 27, 2024
@blaggacao
Copy link
Contributor Author

ping

@stale stale bot removed the inactive label Apr 28, 2024
@blaggacao
Copy link
Contributor Author

@ankush this continues to soft-block frappe/erpnext#39638

Copy link

stale bot commented May 16, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label May 16, 2024
@stale stale bot closed this May 19, 2024
@blaggacao
Copy link
Contributor Author

@ankush @akhilnarang can you please reopen and remove the stale bot?

@akhilnarang akhilnarang reopened this May 22, 2024
@stale stale bot removed the inactive label May 22, 2024
@blaggacao
Copy link
Contributor Author

blaggacao commented Jun 17, 2024

@akhilnarang Thank you! Would you mind giving it a review, too? 😄 I already tended to @barredterra 's feedback above.

@blaggacao
Copy link
Contributor Author

blaggacao commented Jun 17, 2024

I can't capture a screenshot, because the drop down doesn't persist when triggering a capture :-/

But now you can select customer (Customer) (i.e. <field> (<linked doctype>) as a target for SMS (& on private branch: Whatsapp, Matrix), because for SMS, from customer can be extracted a receiver.

@blaggacao blaggacao force-pushed the chore/generalize-notification-receiver-fields branch from 57624ee to 3b80b2e Compare June 17, 2024 08:17
@blaggacao
Copy link
Contributor Author

This is now really battle tested since ~6 months in production and it's doing critical service every day.

@blaggacao
Copy link
Contributor Author

ping

@blaggacao
Copy link
Contributor Author

Go for it now. The future is promised to no one. - Wayne Dyer


Kindly help move this PR forward. Many thanks from Yours Sincerely!

@NagariaHussain
Copy link
Member

@akhilnarang Can you please review this? 😅

Let's either review-merge this or close with reason if we can't.

Copy link
Member

@akhilnarang akhilnarang left a comment

Choose a reason for hiding this comment

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

Seems fine mostly, will test it out a bit morelocally

frappe/email/doctype/notification/notification.py Outdated Show resolved Hide resolved
frappe/email/doctype/notification/notification.py Outdated Show resolved Hide resolved
frappe/email/doctype/notification/notification.py Outdated Show resolved Hide resolved
@blaggacao
Copy link
Contributor Author

@akhilnarang I attended all your feedback accordingly, thanks for the review!

@blaggacao
Copy link
Contributor Author

Fears are nothing more than a state of mind. - Napoleon Hill


Kindly help move this PR forward. Many thanks from Yours Sincerely!

@blaggacao
Copy link
Contributor Author

What the caterpillar calls the end of the world, the master calls a butterfly. - Richard Bach


Kindly help move this PR forward. Many thanks from Yours Sincerely!

@akhilnarang
Copy link
Member

@blaggacao could you rebase this PR, seems like there's a conflict now.

@blaggacao blaggacao force-pushed the chore/generalize-notification-receiver-fields branch from 75dcf88 to 7c1abdf Compare July 12, 2024 09:38
@blaggacao
Copy link
Contributor Author

@akhilnarang alright, thanks for the pointer! Should be good now.

@blaggacao
Copy link
Contributor Author

Watch your thoughts, they become your words Watch your words, they become your actions Watch your actions, they become your habits Watch your habits, they become your character Watch your character, it becomes your destiny. -


Kindly help move this PR forward. Many thanks from Yours Sincerely!

@akhilnarang
Copy link
Member

@blaggacao just run pre-commit once, small formatting issue

@blaggacao
Copy link
Contributor Author

@akhilnarang done 🤦

@akhilnarang akhilnarang merged commit 53b054e into frappe:develop Jul 18, 2024
24 checks passed
@blaggacao blaggacao deleted the chore/generalize-notification-receiver-fields branch July 18, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add-test-cases Add test case to validate fix or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants