[ENG-10083] Add type and is_digest "relation" and update subscription #11558
Conversation
|
LGTM! |
cslzchen
left a comment
cslzchen
left a comment
There was a problem hiding this comment.
Have a few questions on the unit test fixes. Let's take a look together tomorrow.
| is_published=True, | ||
| set_doi=False | ||
| ) | ||
| with capture_notifications(): |
There was a problem hiding this comment.
hmm, so creating a new version of preprint did not send notifications before our digest change?
There was a problem hiding this comment.
Yes, this notification was sent as a digest before.
|
|
||
| messages = get_mailhog_messages() | ||
| assert messages['count'] == len(notifications['emits']) | ||
| assert_emails(messages, notifications) |
There was a problem hiding this comment.
Curious why assert_emails(messages, notifications) fix the test failure?
And I see send_users_instant_digest_email.delay() no longer being called and tested?
There was a problem hiding this comment.
assert_emails does not fix the test, it is more suitable here because it compares emails rather than just counting them, and it can distinguish between digest and non-digest emails.
The send_users_instant_digest_email was removed because PROVIDER_REVIEWS_SUBMISSION_CONFIRMATION emails are not sent as a digest, so we don't need to trigger it to receive them.
| assert not messages['items'] | ||
| with capture_notifications(passthrough=True) as notifications: | ||
| send_users_instant_digest_email.delay() | ||
| massages = get_mailhog_messages() | ||
| assert massages['count'] == len(notifications['emits']) |
There was a problem hiding this comment.
Similarly, what was the issue that leads us to remove this part of the test.
| messages = get_mailhog_messages() | ||
| assert messages['count'] == 1 | ||
| assert messages['items'][0]['Content']['Headers']['To'][0] == registration.creator.username | ||
| assert_emails(messages, notifications) |
| def test_save_published_called(self, mock_share_responses, preprint, user, auth): | ||
| with expect_preprint_ingest_request(mock_share_responses, preprint): | ||
| preprint.set_published(True, auth=auth, save=True) | ||
| with capture_notifications(): |
There was a problem hiding this comment.
Also curious on why our _is_digest change has triggered extra notifications to be sent in many tests here. Should we verify what notifications are sent and are they sent correctly?
There was a problem hiding this comment.
Now, the PROVIDER_REVIEWS_SUBMISSION_CONFIRMATION email is sent immediately, rather than as part of a digest. This change caused the tests to fail.
| user=user, | ||
| event_context=context | ||
| event_context=context, | ||
| is_digest=True, |
There was a problem hiding this comment.
With our change, I would think whether having is_digest=True or not shouldn't make a difference.
There was a problem hiding this comment.
Yes, but if we don't set the correct value, Sentry will log it each time an email is sent.
| 'document_type': self.provider.preprint_word, | ||
| 'notify_comment': not self.provider.reviews_comments_private | ||
| }, | ||
| is_digest=True |
There was a problem hiding this comment.
Oh, I see. So this one was digest but is actually non-digest now. Is this (part of) the reason why we have so many changes in the test files?
There was a problem hiding this comment.
Correct
3 participants
Ticket
https://openscience.atlassian.net/browse/ENG-10083
Purpose
Add type and is_digest "relation" and update subscription
Changes
Side Effects
QE Notes
CE Notes
Documentation