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

Staff role should not suppress reviewer emails #3984

Conversation

bickelj
Copy link
Contributor

@bickelj bickelj commented Jun 12, 2024

Reviewer email notification for assigned application behavior corrected.

Fixes #3983

Test Steps

Set up a dummy email SMTP server or use file-based email.

  • Update a user to be both Staff and Admin
  • Assign that user to review an application/submission
  • See emails go through to that user

Reviewer email notification for assigned application behavior corrected.

Fixes #3983
@bickelj bickelj changed the title Staff role should not remove permissions Staff role should not suppress reviewer emails Jun 12, 2024
@frjo
Copy link
Contributor

frjo commented Jun 12, 2024

We need some way to configure this. OTF uses Slack and their staff do not want duplicate info via e-mail.

I'm thinking a settings "STAFF_NOTIFICATION_METHOD" = email|slack|…

@bickelj
Copy link
Contributor Author

bickelj commented Sep 26, 2024

@frjo That sounds simple enough. But then looking under the hood, it is not obvious if/where we track state of "I sent X type of message or not" so this might be more work than it seems.

@frjo
Copy link
Contributor

frjo commented Sep 30, 2024

@bickelj All notifications to staff are sent via Slack currently. That is the reason for and not reviewer.is_apply_staff.

We plan, hopefully this year, to make all notifications go via e-mail as default. Staff notifications via Slack will be optional then.

If you think it is worthwhile you can implement the setting for this particular case. But might be best/easiest to wait for us fixing it for all notifications.

@bickelj
Copy link
Contributor Author

bickelj commented Oct 1, 2024

@frjo Since this is a one-line merge conflict, I think we can leave this alone for now. I was trying to think of a durable, better, more general way of honoring such a setting but it eluded me. My best thought was something like "pass the list of enabled messengers in kwargs to every messenger" and/or "pass the list of already-processed messengers in kwargs to every next messenger" but any actions taken on those would be under the assumption that the list is in priority order, which is flimsy.

@frjo frjo closed this Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User is not receiving email notifications when assigned to review an application
3 participants