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

Fix secure message return to inbox #961

Merged
merged 21 commits into from
Aug 23, 2024

Conversation

SteveScorfield
Copy link
Contributor

@SteveScorfield SteveScorfield commented Aug 9, 2024

What and why?

Return to the correct inbox was not working correctly for some technical and misc messages when there is recorded survey ID in the secure_message table. The code has been updated so that all technical and misc messages will return to the correct inbox.

How to test?

  1. Run unit tests
  2. Deploy this PR to your dev env and run the acceptance tests
  3. Create a technical message and misc message via Frontstage
  4. In rOps got to navigate to the 2 messages and click on them
  5. Click the return button and you should be redirected to the correct inbox

Jira

https://jira.ons.gov.uk/browse/RAS-1224

@SteveScorfield
Copy link
Contributor Author

/deploy scorfs

@ras-rm-pr-bot
Copy link
Collaborator

Deploying to dev cluster with following parameters:

  • namespace: scorfs

  • tag: pr-961

  • configBranch: main

  • paramKey: ``

  • paramValue: ``

@anwilkie
Copy link
Contributor

anwilkie commented Aug 9, 2024

/deploy wilkia

@ras-rm-pr-bot
Copy link
Collaborator

Deploying to dev cluster with following parameters:

  • namespace: wilkia

  • tag: pr-961

  • configBranch: main

  • paramKey: ``

  • paramValue: ``

Copy link
Contributor

@LJBabbage LJBabbage left a comment

Choose a reason for hiding this comment

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

This is only part of the story, perhaps it's not explicit in the card, but the original story was to make sure the green message box, with a link to the message (closed, message sent) is present. Unfortunately this still doesn't pass that information for these inboxes

@SteveScorfield
Copy link
Contributor Author

/deploy scorfs

@ras-rm-pr-bot
Copy link
Collaborator

Deploying to dev cluster with following parameters:

  • namespace: scorfs

  • tag: pr-961

  • configBranch: main

  • paramKey: ``

  • paramValue: ``

@anwilkie anwilkie self-requested a review August 13, 2024 13:59
@anwilkie
Copy link
Contributor

/deploy wilkia

@ras-rm-pr-bot
Copy link
Collaborator

Deploying to dev cluster with following parameters:

  • namespace: wilkia

  • tag: pr-961

  • configBranch: main

  • paramKey: ``

  • paramValue: ``

Copy link
Contributor

@anwilkie anwilkie left a comment

Choose a reason for hiding this comment

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

When I deployed and tested this PR out, I made the following observation:

  1. Create a technical message on frontstage.
  2. On r-ops, locate the message and assign the thread to a particular survey.
  3. Re-assign the thread back to technical/misc.
  4. In the message thread view, click on the "return to messages" button at the bottom or the "Messages" hyperlink at the top.

You will be redirected to the inbox for the survey that the thread used to be assigned to, rather than the technical/misc inbox.

@SteveScorfield
Copy link
Contributor Author

@anwilkie sorry I forgot to update the test steps. You no longer need to make changes in the DB.

@anwilkie
Copy link
Contributor

When I made the aforementioned observation, I didn't make any changes to the database, and I retested it now to ensure that. I'm not sure whether this bug is an issue with the PR itself or if it should be spun off into its own issue.

As things are, I am able to perform the actions described in the "how to test" part of the description without issue; it's just that there is this strange behaviour that is being showcased by message threads that have switched categories. It might be worth discussing this with the rest of the team to get their thoughts on it.

@SteveScorfield
Copy link
Contributor Author

@anwilkie I see what you mean. I've updated the code to fix the issue after category reassignment.

@SteveScorfield
Copy link
Contributor Author

/deploy scorfs

@ras-rm-pr-bot
Copy link
Collaborator

Deploying to dev cluster with following parameters:

  • namespace: scorfs

  • tag: pr-961

  • configBranch: main

  • paramKey: ``

  • paramValue: ``

response_operations_ui/views/messages.py Outdated Show resolved Hide resolved
tests/test_data/message/thread_misc.json Outdated Show resolved Hide resolved
tests/test_data/message/thread_technical.json Outdated Show resolved Hide resolved
tests/views/test_message.py Outdated Show resolved Hide resolved
tests/views/test_message.py Show resolved Hide resolved
Copy link
Contributor

@LJBabbage LJBabbage left a comment

Choose a reason for hiding this comment

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

A few minor issues, I think we might have to accept that all the tests in this area need work, the mocks are terrible, but good work on the tidy up. The only other query is the pipfile.lock update, it shouldn't really be done here

response_operations_ui/views/messages.py Outdated Show resolved Hide resolved
tests/views/test_message.py Show resolved Hide resolved
response_operations_ui/views/messages.py Outdated Show resolved Hide resolved
response_operations_ui/views/messages.py Outdated Show resolved Hide resolved
Copy link
Contributor

@arroyoAle arroyoAle left a comment

Choose a reason for hiding this comment

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

Looks good and works just a lot of the same comment with a question

tests/views/test_message.py Outdated Show resolved Hide resolved
tests/views/test_message.py Outdated Show resolved Hide resolved
tests/views/test_message.py Outdated Show resolved Hide resolved
tests/views/test_message.py Outdated Show resolved Hide resolved
tests/views/test_message.py Outdated Show resolved Hide resolved
tests/views/test_message.py Outdated Show resolved Hide resolved
Copy link
Contributor

@arroyoAle arroyoAle left a comment

Choose a reason for hiding this comment

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

Works as expected

@SteveScorfield SteveScorfield merged commit 1450be6 into main Aug 23, 2024
3 checks passed
@SteveScorfield SteveScorfield deleted the fix-secure-message-return-to-inbox branch August 23, 2024 11:39
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.

6 participants