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

Task/FP-1375: Admin controlled messages #615

Merged
merged 40 commits into from
Jun 2, 2022

Conversation

duckonomy
Copy link
Contributor

@duckonomy duckonomy commented Mar 24, 2022

Overview:

Allow arbitrary and dismissable SectionMessage generation by admin (through core/admin) for components.

Related Jira tickets:

Summary of Changes:

  1. Created a /intromessages/custom/ endpoint (GET: Get templates & messages, PUT: Update unread state).
  2. Create a CustomMessage UI component as a wrapper around SectionMessages to utilize state.customMessages.
  3. Implemented necessary sagas/reducers to interact with the /intromessages/custom/ endpoint.
  4. Made messages of type intro have a dismiss button.

Testing Steps:

  1. Make sure to run migrations within the core_portal_django container: ./manage.py migrate
  2. Make yourself staff or superuser.
  3. Go to core/admin.
  4. Create a Custom message template.
  5. Go to the component you made it for (Will need refresh).
  6. Ensure that it is correctly rendered
  7. Repeat 3-5 for various different configurations (even multiple messages for single component) and ensure that they render accordingly
  8. Ensure that dismissed (click x) messages do not show up.
  9. Go to core/admin again.
  10. Try removing some that aren't dismissed.
  11. Refresh portal.
  12. Ensure that the deleted messages do not show.

UI Photos:

Screen.Recording.2022-03-24.at.2.14.26.PM.mov

Notes:

  1. (For CMD) Should there be a text limit for messages?
  2. I wanted to refactor intromessages related components (both frontend/backend) into a more generic messages component. But wasn't sure how the migration process would go. Would there be a way to achieve this?
  3. (For CMD) The info message type style does not seem to have an icon (SectionMessage, Message). Thus, the dismiss button (x), which is a child of the icon does not render.

@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

Merging #615 (1b3da0b) into main (b3aa730) will decrease coverage by 0.00%.
The diff coverage is 66.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #615      +/-   ##
==========================================
- Coverage   67.79%   67.79%   -0.01%     
==========================================
  Files         423      426       +3     
  Lines       13077    13173      +96     
  Branches     2394     2414      +20     
==========================================
+ Hits         8865     8930      +65     
- Misses       3921     3951      +30     
- Partials      291      292       +1     
Flag Coverage Δ
javascript 70.37% <41.33%> (-0.24%) ⬇️
unittests 65.74% <95.38%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...t/src/components/Allocations/AllocationsLayout.jsx 80.00% <ø> (ø)
...rc/components/Applications/AppLayout/AppLayout.jsx 89.18% <ø> (ø)
client/src/components/Dashboard/Dashboard.jsx 72.22% <ø> (ø)
client/src/components/DataFiles/DataFiles.jsx 75.00% <ø> (ø)
client/src/components/History/History.jsx 93.75% <ø> (ø)
...ent/src/components/ManageAccount/ManageAccount.jsx 55.00% <ø> (ø)
...lient/src/components/Onboarding/OnboardingUser.jsx 96.15% <ø> (ø)
client/src/components/UIPatterns/UIPatterns.jsx 0.00% <ø> (ø)
client/src/components/_common/Section/Section.jsx 75.00% <ø> (ø)
...al/apps/portal_messages/migrations/0001_initial.py 100.00% <ø> (ø)
... and 14 more

@duckonomy duckonomy requested review from rstijerina and removed request for jmartinez-tacc March 24, 2022 20:58
Copy link
Contributor

@edmondsgarrett edmondsgarrett left a comment

Choose a reason for hiding this comment

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

LGTM, works as described.

Copy link
Member

@rstijerina rstijerina left a comment

Choose a reason for hiding this comment

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

Nice work! Left a few comments, but overall 👍

@rstijerina
Copy link
Member

I wanted to refactor intromessages related components (both frontend/backend) into a more generic messages component. But wasn't sure how the migration process would go. Would there be a way to achieve this?

@duckonomy Django supports custom migrations, so one way to achieve that could be to deprecate the old model and migrate data over in a single new migration file. We've done something similar in designsafe, though that was a bit simpler

Copy link
Member

@rstijerina rstijerina left a comment

Choose a reason for hiding this comment

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

Marking as "Request Changes" to keep track

@duckonomy
Copy link
Contributor Author

duckonomy commented May 6, 2022

@rstijerina

  1. Refactored /intromessages/ in the backend to a more generic /messages/ folder. Also changed frontend intro.sagas.js and intro.reducers.js into message.sagas.js and message.reducers.js.

I'm not sure how this will affect the existing migrations.. because there is a dependency on the moved file/directory.

    dependencies = [
        ('message', '0001_initial'),
    ]

If this doesn't work out, we could rollback from 9681a8b.

  1. Adjusted TextField to CharField due to your comment about allowing new lines and added 200 char limit in the according of our conversation in slack.

  2. I wasn't sure how to allow resolve this case at the moment. Currently, both IntroMessages and CustomMessages requires a relation to a user to keep track track of unread or dismissal status. So, routes that do not require authorization like public-data aren't supported for now. I'm not sure if we should allow these message would be allowed to be dismissed or purely admin-handled. If dismissable, do we do what we used to do and keep it in localStorage?. If this is required, I could make a ticket tackling this issue.

@duckonomy duckonomy requested a review from rstijerina May 9, 2022 18:01
@rstijerina
Copy link
Member

rstijerina commented May 11, 2022

  1. I wasn't sure how to allow resolve Task/FP-1375: Admin controlled messages #615 (comment) at the moment. Currently, both IntroMessages and CustomMessages requires a relation to a user to keep track track of unread or dismissal status. So, routes that do not require authorization like public-data aren't supported for now. I'm not sure if we should allow these message would be allowed to be dismissed or purely admin-handled. If dismissable, do we do what we used to do and keep it in localStorage?. If this is required, I could make a ticket tackling this issue.

@duckonomy Yes, just what I was thinking too. We could use localStorage if the user is unauthenticated. New ticket sounds perfect 👍

@duckonomy
Copy link
Contributor Author

duckonomy commented May 11, 2022

5. I wasn't sure how to allow resolve Task/FP-1375: Admin controlled messages #615 (comment) at the moment. Currently, both IntroMessages and CustomMessages requires a relation to a user to keep track track of unread or dismissal status. So, routes that do not require authorization like public-data aren't supported for now. I'm not sure if we should allow these message would be allowed to be dismissed or purely admin-handled. If dismissable, do we do what we used to do and keep it in localStorage?. If this is required, I could make a ticket tackling this issue.

Yes, just what I was thinking too. We could use localStorage if the user is unauthenticated. New ticket sounds perfect 👍

👍 Created ticket FP-1644

Copy link
Member

@rstijerina rstijerina left a comment

Choose a reason for hiding this comment

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

Looking great! Just a few more comments

client/src/components/_common/Section/SectionMessages.jsx Outdated Show resolved Hide resolved
server/portal/apps/message/views.py Outdated Show resolved Hide resolved
server/portal/apps/message/views.py Outdated Show resolved Hide resolved
server/portal/apps/message/models.py Outdated Show resolved Hide resolved
server/portal/apps/message/models.py Outdated Show resolved Hide resolved
@duckonomy duckonomy requested a review from rstijerina May 17, 2022 22:03
@duckonomy
Copy link
Contributor Author

@rstijerina

  1. Changed introMessageName to messageName
  2. Changed batch behavior to single message update
  3. Re-did migration
  4. Changed route name to portal_messages
  5. Improved readability of messages in admin panel.

Copy link
Member

@rstijerina rstijerina left a comment

Choose a reason for hiding this comment

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

@duckonomy Looking great, just a couple more comments!

@duckonomy
Copy link
Contributor Author

duckonomy commented Jun 2, 2022

@rstijerina

  1. Simplified payload (templateId and unread) for PUT request for updating custom messages.
  2. Renamed introMessages to introMessageComponents and introMessageName to messageComponentName to reflect what state.introMessages actually is (map of the state of a message for certain component). I think this makes more sense and is coherent with the use of the the prop in Section and SectionMessages for customMessages as well.

@rstijerina rstijerina closed this Jun 2, 2022
@rstijerina rstijerina reopened this Jun 2, 2022
Copy link
Member

@rstijerina rstijerina left a comment

Choose a reason for hiding this comment

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

LGTM!

@rstijerina rstijerina merged commit 6040fa4 into main Jun 2, 2022
@rstijerina rstijerina deleted the task/FP-1375-admin-controlled-messages branch June 2, 2022 21:48
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.

4 participants