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: add login user to driver if available for notifications #39638

Merged
merged 5 commits into from
Jul 18, 2024

Conversation

blaggacao
Copy link
Collaborator

@blaggacao blaggacao commented Jan 30, 2024

Use Case

  1. Driver is internal user (for example when employee)
  2. Send notifications to driver based on data that is only available or maintained on internal users

Proposed Solution

  • add internal user to driver document
  • take it from employee, if employee is set and make the field read-only

no-docs

@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Jan 30, 2024
Copy link

codecov bot commented Jan 31, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 60.13%. Comparing base (8e52218) to head (b39f7b1).
Report is 398 commits behind head on develop.

Current head b39f7b1 differs from pull request most recent head 7032a10

Please upload reports for the commit 7032a10 to get more accurate results.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #39638      +/-   ##
===========================================
- Coverage    60.76%   60.13%   -0.64%     
===========================================
  Files          760      758       -2     
  Lines        71881    70955     -926     
===========================================
- Hits         43682    42669    -1013     
- Misses       28199    28286      +87     
Files Coverage Δ
erpnext/setup/doctype/driver/driver.py 33.33% <75.00%> (+5.55%) ⬆️

... and 334 files with indirect coverage changes

@blaggacao
Copy link
Collaborator Author

image

Copy link

stale bot commented Mar 2, 2024

This pull request has been automatically marked as inactive 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 Mar 2, 2024
@stale stale bot removed the inactive label Mar 2, 2024
@ruthra-kumar ruthra-kumar self-assigned this Mar 29, 2024
@ruthra-kumar
Copy link
Member

Send notifications to driver based on data that is only available or maintained on internal users

Any example of an Internal User specific data?

@blaggacao
Copy link
Collaborator Author

blaggacao commented Mar 29, 2024

Any example of an Internal User specific data?

Yes, in our case it's Matrix ID (for matrix chat). But it could be email, phone or pager, too.

I should also note, that not every driver is also an employee. Hence, if employee isn't set, then the user field can be set freely.

However, any driver that should be incorporated into the company's notification infrastructure can be required, if not an employee, to be(come) a user.

Thus external service providers, indeed are users so they get notifications, itinerary, maps links, etc via Matrix Chat.

@blaggacao
Copy link
Collaborator Author

blaggacao commented Mar 29, 2024

Maybe I should also add, that in our deployment, this works together with: frappe/frappe#24725

That PR implements the selection of a user field as a notification target.

@ruthra-kumar
Copy link
Member

Looks like a simple addition. Once the Framework changes are merged, then we can merge this as well.

Copy link

stale bot commented Apr 13, 2024

This pull request has been automatically marked as inactive 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 13, 2024
@blaggacao
Copy link
Collaborator Author

blaggacao commented Apr 14, 2024

@ruthra-kumar can you maybe help recommend the framework changes with a trusted colleague responsible for the framework? It would be nice if we where to avoid the endless and nasty stale bot loop on this one. Thx, that'd be much appreciated!

@stale stale bot removed the inactive label Apr 14, 2024
@blaggacao
Copy link
Collaborator Author

ping

Copy link

stale bot commented May 18, 2024

This pull request has been automatically marked as inactive 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 18, 2024
@blaggacao
Copy link
Collaborator Author

@ruthra-kumar Could you please help to push for frappe/frappe#24725 Thanks!

@stale stale bot removed the inactive label May 22, 2024
@blaggacao
Copy link
Collaborator Author

ping

@blaggacao
Copy link
Collaborator 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!

@blaggacao
Copy link
Collaborator 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
Collaborator 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!

@blaggacao
Copy link
Collaborator 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!

@blaggacao
Copy link
Collaborator Author

Looks like a simple addition. Once the Framework changes are merged, then we can merge this as well.

@ruthra-kumar fyi, the framework changes have just been merged.

@ruthra-kumar ruthra-kumar merged commit a912e07 into frappe:develop Jul 18, 2024
13 checks passed
@blaggacao blaggacao deleted the feat/add-login-user-to-driver branch July 18, 2024 12:36
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
depends-on-frappe needs-tests This PR needs automated unit-tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants