[ENG-6682] Add reply-to and cc-ing features to Institutional Access #10841

Johnetordoff · 2024-12-11T15:01:01Z

Purpose

Add the CCing and Reply-to features that were added as a result of the ARB.

Changes

adds bool fields to serializers to indicates cc and/or reply-to
cleans-up logic in email utilities and updates them to allow for ccing and reply-to
adds tests for the new behavior.
clean-up indention and logic through-out send_mail utilities

QA Notes

Please make verification statements inspired by your code and what your code touches.

Verify
Verify

What are the areas of risk?

Any concerns/considerations/questions that development raised?

Documentation

Side Effects

Ticket

https://openscience.atlassian.net/browse/ENG-6682

PR:

I decided to split this off this PR, because it was only for features discussed in the ARB.

[ENG-6682] Add UserMessage feature for Institutional Access #10824

…wn address as a reply_to

…terForOpenScience/osf.io into institutional-access-user-message-arb * 'feature/institutional_access' of https://github.com/CenterForOpenScience/osf.io: add user message read/write permissions to full add new user message oauth scope and throttling classes Fix backfill, report Update changelog and bump versions Follow-up fix for target/next (start/end) month Fix failures caused by base class MonthlyReporter update [ENG-6506] Fix: counted-usage clobbers (CenterForOpenScience#10799) [ENG-6435] Fix: duplicate reports when run for past years (CenterForOpenScience#10800) Add PrivateSpamMetricsReport (CenterForOpenScience#10791) [ENG-4438] Add OOPSpam and Akismet metrics to spam report (CenterForOpenScience#10783) [ENG-6364] Migrate Preprint Affilations (CenterForOpenScience#10787)

brianjgeiger

One thing to check with product on. I'm worried about exposing user email addresses, and suspect product will be as well.

brianjgeiger · 2024-12-11T19:49:23Z

framework/email/tasks.py

+    if cc_addr:
+        msg['Cc'] = ', '.join(cc_addr)


Check with product: How do they want the To and CC handled to avoid giving someone's email address away? The way this appears to be implemented, the recipients will both see the other's email address. In the case of a cc:, it might be that you want to bcc: both recipients.

Yeah Product agrees BCC is better.

brianjgeiger

One more thing.

brianjgeiger · 2024-12-12T13:31:40Z

api_tests/users/views/test_user_message_institutional_access.py

+            to_addr=user_with_affiliation.username,
+            cc_addr=[institutional_admin.username],


This may be handled later on, but in the case of a bcc, both the sender and the recipient should be bcc'd, otherwise the sender will be able to see the recipient's email address.

brianjgeiger

Still a little bit more on the BCC. I don't think you quite caught what I was saying in my last review.

brianjgeiger · 2024-12-12T16:39:44Z

osf/models/user_message.py

        default=False,
-        help_text='The boolean value that indicates whether other institutional admins were CCed',
+        help_text='The boolean value that indicates whether other institutional admins were BCCed',


This should probably say, "The boolean value that indicates whether the sender is BCCed"

brianjgeiger · 2024-12-12T16:41:20Z

osf/models/user_message.py

@@ -87,7 +87,7 @@ def send_institution_request(self) -> None:
        send_mail(
            mail=MessageTypes.get_template(self.message_type),
            to_addr=self.recipient.username,


I think, in the case of BCC'ing the sender, the recipient also needs to be BCC'd. There would be no to_address in that case. Without the BCC, however, it makes sense to have the recipient be in the to_addr.

This is just a wrapper for the the two different email utilities that implement the BBC in different ways, so it's hard to say which behavior is actually "the best".

But anyway, if there's no to_addr and just bbc-ing, yes that can just BBC I just have to that meld with the two utility functions, seems reasonable from a developer experience perceptive to adapt that behavior to SendGrid and simple SMTP, but it's just making the send_email wrapper around SMTP and Sendgrid better if I understand correctly.

Yeah, I'm not super-concerned where it's changed, just that all recipients of the email are bcc'd if the bcc option is chosen.

Yeah I dealt with that here:

recipients = [to_addr] + (bcc_addr or [])

https://github.com/CenterForOpenScience/osf.io/pull/10841/files#diff-78b71751c095a6eb9a92be212c4acf17f402fc34266030b5440596cf2a57d243R112 sorry probably got lost with the clean-up, but that's new. Sendgrid has it's own way of handling it.

SMTP BCCs by just not CCing, so no additional action is necessary.

add code to allow cc-ing fellow institutional admins and put their o…

25d7857

…wn address as a reply_to

Johnetordoff changed the title ~~Institutional access user message arb~~ [ENG-6682] Add reply-to and cc-ing features to Institutional Access Dec 11, 2024

John Tordoff added 2 commits December 11, 2024 10:03

revert typo

dc2331e

Johnetordoff force-pushed the institutional-access-user-message-arb branch from 3bb43b8 to dc2331e Compare December 11, 2024 15:37

Johnetordoff marked this pull request as ready for review December 11, 2024 16:55

Johnetordoff mentioned this pull request Dec 11, 2024

[ENG-6682] Add UserMessage feature for Institutional Access #10824

Merged

Johnetordoff requested a review from brianjgeiger December 11, 2024 18:33

brianjgeiger requested changes Dec 11, 2024

View reviewed changes

Johnetordoff force-pushed the institutional-access-user-message-arb branch from 8cf6f25 to 87711e9 Compare December 12, 2024 13:11

brianjgeiger requested changes Dec 12, 2024

View reviewed changes

Johnetordoff force-pushed the institutional-access-user-message-arb branch from 87711e9 to 2130551 Compare December 12, 2024 13:50

change to bcc the sender instead of CC-ing them

8de394a

Johnetordoff force-pushed the institutional-access-user-message-arb branch from 2130551 to 8de394a Compare December 12, 2024 13:55

brianjgeiger requested changes Dec 12, 2024

View reviewed changes

brianjgeiger approved these changes Dec 12, 2024

View reviewed changes

Johnetordoff merged commit 7068791 into CenterForOpenScience:feature/institutional_access Dec 12, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENG-6682] Add reply-to and cc-ing features to Institutional Access #10841

[ENG-6682] Add reply-to and cc-ing features to Institutional Access #10841

Johnetordoff commented Dec 11, 2024 •

edited

Loading

brianjgeiger left a comment

brianjgeiger Dec 11, 2024

Johnetordoff Dec 12, 2024

brianjgeiger left a comment

brianjgeiger Dec 12, 2024

brianjgeiger left a comment

brianjgeiger Dec 12, 2024

brianjgeiger Dec 12, 2024

Johnetordoff Dec 12, 2024

brianjgeiger Dec 12, 2024

Johnetordoff Dec 12, 2024 •

edited

Loading

brianjgeiger Dec 12, 2024

		to_addr=user_with_affiliation.username,
		cc_addr=[institutional_admin.username],

[ENG-6682] Add reply-to and cc-ing features to Institutional Access #10841

[ENG-6682] Add reply-to and cc-ing features to Institutional Access #10841

Conversation

Johnetordoff commented Dec 11, 2024 • edited Loading

Purpose

Changes

QA Notes

Documentation

Side Effects

Ticket

PR:

brianjgeiger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianjgeiger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brianjgeiger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Johnetordoff Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Johnetordoff commented Dec 11, 2024 •

edited

Loading

Johnetordoff Dec 12, 2024 •

edited

Loading