Skip to content

Hotfix/20250401 1358 issue3975 fix notifications #2464

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

Closed

Conversation

richard-jones
Copy link
Contributor

@richard-jones richard-jones commented Apr 4, 2025

  • Issue: [enter link to issue here]

Title <- provide a title for the PR

briefly describe the PR here

This PR...

  • has scripts to run
  • has migrations to run
  • adds new infrastructure
  • changes the CI pipeline
  • affects the public site
  • affects the editorial area
  • affects the publisher area
  • affects the monitoring

Developer Checklist

Developers should review and confirm each of these items before requesting review

  • Code meets acceptance criteria from issue
  • Unit tests are written and all pass
  • User Test Scripts (if required) are written and have been run through
  • Project's coding standards are met
    • No deprecated methods are used
    • No magic strings/numbers - all strings are in constants or messages files
    • ES queries are wrapped in a Query object rather than inlined in the code
    • Where possible our common library functions have been used (e.g. dates manipulated via dates)
    • Cleaned up commented out code, etc
    • Urls are constructed with url_for not hard-coded
  • Code documentation and related non-code documentation has all been updated
  • Migation has been created and tested
  • There is a recent merge from develop

Reviewer Checklist

Reviewers should review and confirm each of these items before approval
If there are multiple reviewers, this section should be duplicated for each reviewer

  • Code meets acceptance criteria from issue
  • Unit tests are written and all pass
  • User Test Scripts (if required) are written and have been run through
  • Project's coding standards are met
    • No deprecated methods are used
    • No magic strings/numbers - all strings are in constants or messages files
    • ES queries are wrapped in a Query object rather than inlined in the code
    • Where possible our common library functions have been used (e.g. dates manipulated via dates)
    • Cleaned up commented out code, etc
    • Urls are constructed with url_for not hard-coded
  • Code documentation and related non-code documentation has all been updated
  • Migation has been created and tested
  • There is a recent merge from develop

Testing

You can run the new testdrive with

http://localhost:5004/testdrive/notifications

and verify that the notifications are created correctly for each user type.

Deployment

No deployment considerations

@richard-jones richard-jones marked this pull request as ready for review April 8, 2025 15:02
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 32 out of 32 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (4)

portality/events/consumers/application_publisher_revision_notify.py:31

  • Explicitly returning None improves clarity; please verify that downstream code correctly handles this None value as expected.
return None

portality/events/consumers/application_publisher_revision_notify.py:51

  • Returning the notification object after sending the notification may affect caller expectations; confirm that consumers of this function are prepared to handle the returned object.
return notification

doajtest/fixtures/v2/applications.py:26

  • Removing 'current_journal' from the application source might impact tests or functionality that expect this key; verify that this change is intentional and safe.
del result["admin"]["current_journal"]

portality/events/consumers/application_editor_acceptreject_notify.py:25

  • Introducing a check for application_type changes the notification logic; please ensure that this new condition aligns with the intended business rules.
if application.application_type != constants.APPLICATION_TYPE_NEW_APPLICATION:

Copy link
Contributor

@Steven-Eardley Steven-Eardley left a comment

Choose a reason for hiding this comment

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

This looks good, although I'd have preferred a bit more docs for lib.dicts

@Steven-Eardley Steven-Eardley deleted the hotfix/20250401_1358_issue3975_fix_notifications branch April 17, 2025 10:18
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.

2 participants