2604 preparation reminder for internal users - #2688
Conversation
| if internal_only: | ||
| responsibles = responsibles.exclude(pk__in=[r.pk for r in responsibles if r.is_external]) |
There was a problem hiding this comment.
is_external is implemented in python logic, so django cannot convert it into a SQL query. That means we don't "win" anything by adding this filter to the query early. Note that the code here currently fetches responsibles, computes is_external for each of them, and then amends the original query with "where id is not in [list of external IDs]" -- this is just one extra database round trip without real benefit.
Above, you access request.POST, but outside the if request.method == 'POST' block below.
I think for maintainability, it makes sense to put this check inside this block. That way, we keep the queries simple, save one database roundtrip, don't build large SQL, and the world is greener.
The code change would then just be
if request.POST.get("internal_only") == "true" and responsible.is_external:
continue| email_template_mock.objects.get.return_value = email_template_mock | ||
| email_template_mock.EDITOR_REVIEW_REMINDER = EmailTemplate.EDITOR_REVIEW_REMINDER | ||
|
|
||
| self.app.post(self.url, params={"internal_only": "true"}, user=self.manager, status=200) | ||
|
|
||
| recipients = [call[0][0] for call in email_template_mock.send_to_user.call_args_list] | ||
| self.assertIn(self.user, recipients) | ||
| self.assertNotIn(external_responsible, recipients) |
There was a problem hiding this comment.
Would it be less code to not patch the email templates and just check mail.outbox afterwards? If so, let's do that (otherwise, ignore).
| <span slot="action-text">{% translate 'Remind all internal users' %}</span> | ||
| <span slot="question">{% translate 'Do you really want to remind all internal users?' %}</span> | ||
|
|
||
| <button slot="show-button" type="button" id="remindAllInternalUsersButton" class="btn btn-sm btn-light">{% translate 'Remind all internal users' %}</button> |
There was a problem hiding this comment.
The id here, remindAllInternalUsersButton, is never used. Let's remove it.
While you're at it, the same is true for remindAllButton above.
| messages.success(request, _("Successfully sent reminders to everyone.")) | ||
| messages.success(request, _("Successfully sent reminders.")) |
There was a problem hiding this comment.
two different success messages for the two cases would be nice
richardebeling
left a comment
richardebeling
left a comment
There was a problem hiding this comment.
Marking as "changes requested" to clean-up our "to review" lists
3 participants
closes #2604: On staff:preparation_reminder, next to "Remind all", adds a "Remind all internal" button.