-
Notifications
You must be signed in to change notification settings - Fork 354
[hotfix/26.1.5] [ENG-10027] Fix broken review links in moderation digest for preprints & registrations #11537
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
Conversation
| context['message'] = f'has requested withdrawal of "{resource.title}".' | ||
| # Set submission url | ||
| context['reviews_submission_url'] = f'{DOMAIN}reviews/registries/{provider._id}/{resource._id}' | ||
| context['reviews_submission_url'] = f'{DOMAIN}registries/{provider._id}/{resource._id}?mode=moderator' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Does this one in
notifications/listeners.pyduplicates with the one inwebsite/reviews/listeners.py? - A follow-up question is why listeners are in two different places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these appear to be duplicates. One located in notifications/listeners.py should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe notifications/listeners.py was created to serve as an easily accessible location for notification signals. However, it looks like we decided not to move everything there at some point, yet the duplicates were never removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ostap-Zherebetskyi and @bodintsov
It seems our unit tests may have been using the wrong signal or both signals are being used, see failures: https://github.com/CenterForOpenScience/osf.io/actions/runs/20826183331/job/59828027689?pr=11537 so I removed the commit for removing this signal. I had problem with mailhog connection when running unit tests locally, so I will create another ticket for merging/fixing the two duplicate signals.
Ostap-Zherebetskyi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the "reviews_submission_url" for collections have the same change?
It's LGTM after the comments are resolved.
2e01adf to
afdf68f
Compare
felliott
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Ticket
Purpose
Fix link in moderator digest emails
Changes
Remove a duplicate and unused signalSide Effects
N/A
QE Notes
CE Notes
N/A
Documentation
N/A