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

fix!: allow system managers to toggle email queue #17466

Merged
merged 2 commits into from Jul 19, 2022

Conversation

sagarvora
Copy link
Member

@sagarvora sagarvora commented Jul 11, 2022

Closes #14218

Why did the Resume / Suspend Sending button only work for Administrator?

The button set the default hold_queue using the frappe.defaults.set_default JS API which called the frappe.client.set_default API without explicitly passing a parent. In absence of a parent, the set_default API chose frappe.session.user as one (why?). When processing the queue in EmailQueue.process, we're using frappe.defaults.get_defaults which gets defaults from the __default + frappe.session.user parents. When called from redis queue, the frappe.session.user is Administrator. Hence, get_defaults gives the correct value of hold_queue only if set by Administrator, but not other users.

Changes Made

Screenshots

Patch Test

image

UI

email queue after

@sagarvora sagarvora requested a review from a team as a code owner July 11, 2022 17:05
@sagarvora sagarvora requested review from shariquerik and removed request for a team July 11, 2022 17:05
@sagarvora sagarvora force-pushed the remove-client.set_default branch 3 times, most recently from b0aa24e to 861947d Compare July 11, 2022 17:38
@codecov
Copy link

codecov bot commented Jul 18, 2022

Codecov Report

Merging #17466 (4b0a9da) into develop (04f4fd8) will increase coverage by 0.37%.
The diff coverage is 50.00%.

@@             Coverage Diff             @@
##           develop   #17466      +/-   ##
===========================================
+ Coverage    56.62%   57.00%   +0.37%     
===========================================
  Files          760      768       +8     
  Lines        68219    69016     +797     
  Branches      5835     6003     +168     
===========================================
+ Hits         38632    39343     +711     
- Misses       25946    26192     +246     
+ Partials      3641     3481     -160     
Flag Coverage Δ
server 61.14% <50.00%> (-1.76%) ⬇️
ui-tests 49.76% <ø> (+4.45%) ⬆️

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

@mergify mergify bot merged commit 7a7693e into frappe:develop Jul 19, 2022
@sagarvora sagarvora deleted the remove-client.set_default branch July 19, 2022 05:10
@sagarvora
Copy link
Member Author

@Mergifyio backport version-13-hotfix

@mergify
Copy link
Contributor

mergify bot commented Jul 19, 2022

backport version-13-hotfix

✅ Backports have been created

@sagarvora sagarvora restored the remove-client.set_default branch July 20, 2022 06:35
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Email is sent from queue despite Suspend Sending is active.
2 participants