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

[ENG-6882]fixed double mail sending #10919

Conversation

bodintsov
Copy link
Contributor

Purpose

Fix sending 2 emails instead of 1 when using institutional access project requests

Changes

TBD

QA Notes

TBD

Documentation

TBD

Side Effects

TBD

Ticket

(https://openscience.atlassian.net/browse/ENG-6882)

@bodintsov bodintsov marked this pull request as ready for review January 14, 2025 18:33
@bodintsov
Copy link
Contributor Author

I didn`t add new test cases as existing ones covers new implementation

@Johnetordoff Johnetordoff force-pushed the feature/double_email_fix branch from 3e710a4 to 3c93931 Compare January 15, 2025 14:20
@Johnetordoff
Copy link
Contributor

@bodintsov I'm sorry I accidentally pushed to your branch while I testing some code. I revert now.

Copy link
Collaborator

@brianjgeiger brianjgeiger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of things I think shouldn't be in this PR.

Comment on lines 44 to 45
'localhost',
'127.0.0.1'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These shouldn't be in defaults. This should only be in your local settings overrides.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it`s my mistake, removed

@@ -192,6 +192,7 @@ flags:
- flag_name: INSTITUTIONAL_DASHBOARD_2024
name: institutional_dashboard_2024
note: whether to surface older or updated (in 2024) institutional metrics
everyone: false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be part of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it`s just my mistake, removed

@bodintsov bodintsov force-pushed the feature/double_email_fix branch from 3c93931 to bb243a1 Compare January 15, 2025 14:57
@Johnetordoff Johnetordoff merged commit e5cc7fe into CenterForOpenScience:feature/institutional_access Jan 15, 2025
6 checks passed
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.

3 participants