1337 email owner as system admin group (un)assigned #3253

raftmsohani · 2024-10-31T14:35:02Z

Summary of Changes

Provide a brief summary of changes
Pull request closes #1337 _

How to Test

task up

Open http://localhost:3000/ and sign in.
Proceed to admin page and create a group called: "System Owner"
Then add this group to one of the users
Change another user's group from/to: "OFA System Admin"
Check the user with system owner group receives email

The following screenshot shows an example of email template for assignment:

Deliverables

More details on how deliverables herein are assessed included here.

Deliverable 1: Accepted Features

Checklist of ACs:

[insert ACs here]
lfrohlich and/or adpennington confirmed that ACs are met.

Deliverable 2: Tested Code

Are all areas of code introduced in this PR meaningfully tested?
- If this PR introduces backend code changes, are they meaningfully tested?
- If this PR introduces frontend code changes, are they meaningfully tested?
Are code coverage minimums met?
- Frontend coverage: [insert coverage %] (see CodeCov Report comment in PR)
- Backend coverage: [insert coverage %] (see CodeCov Report comment in PR)

Deliverable 3: Properly Styled Code

Are backend code style checks passing on CircleCI?
Are frontend code style checks passing on CircleCI?
Are code maintainability principles being followed?

Deliverable 4: Accessible

Does this PR complete the epic?
Are links included to any other gov-approved PRs associated with epic?
Does PR include documentation for Raft's a11y review?
Did automated and manual testing with iamjolly and ttran-hub using Accessibility Insights reveal any errors introduced in this PR?

Deliverable 5: Deployed

Was the code successfully deployed via automated CircleCI process to development on Cloud.gov?

Deliverable 6: Documented

Does this PR provide background for why coding decisions were made?
If this PR introduces backend code, is that code easy to understand and sufficiently documented, both inline and overall?
If this PR introduces frontend code, is that code easy to understand and sufficiently documented, both inline and overall?
If this PR introduces dependencies, are their licenses documented?
Can reviewer explain and take ownership of these elements presented in this code review?

Deliverable 7: Secure

Does the OWASP Scan pass on CircleCI?
Do manual code review and manual testing detect any new security issues?
If new issues detected, is investigation and/or remediation plan documented?

Deliverable 8: User Research

Research product(s) clearly articulate(s):

the purpose of the research
methods used to conduct the research
who participated in the research
what was tested and how
impact of research on TDP
(if applicable) final design mockups produced for TDP development

tdrs-backend/tdpservice/email/tasks.py

tdrs-backend/tdpservice/email/helpers/admin_notifications.py

…aft-tech/TANF-app into 1337-sys-admin-permission-change-email

codecov · 2024-11-06T12:59:48Z

Codecov Report

Attention: Patch coverage is 90.24390% with 4 lines in your changes missing coverage. Please review.

Project coverage is 91.52%. Comparing base (bf0bcec) to head (770dcf7).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
tdrs-backend/tdpservice/email/tasks.py	66.66%	2 Missing ⚠️
tdrs-backend/tdpservice/users/signals.py	89.47%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3253      +/-   ##
===========================================
- Coverage    91.52%   91.52%   -0.01%     
===========================================
  Files          297      298       +1     
  Lines         8415     8456      +41     
  Branches       608      612       +4     
===========================================
+ Hits          7702     7739      +37     
- Misses         603      606       +3     
- Partials       110      111       +1

Flag	Coverage Δ
dev-backend	`91.36% <90.24%> (-0.01%)`	⬇️
dev-frontend	`92.66% <ø> (ø)`

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

Files with missing lines	Coverage Δ
tdrs-backend/tdpservice/email/email_enums.py	`100.00% <100.00%> (ø)`
...nd/tdpservice/email/helpers/admin_notifications.py	`92.59% <100.00%> (+6.87%)`	⬆️
tdrs-backend/tdpservice/users/apps.py	`100.00% <100.00%> (ø)`
tdrs-backend/tdpservice/email/tasks.py	`79.78% <66.66%> (-0.90%)`	⬇️
tdrs-backend/tdpservice/users/signals.py	`89.47% <89.47%> (ø)`

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8e618a...770dcf7. Read the comment docs.

andrew-jameson

LGTM!

jtimpe · 2024-11-08T18:47:07Z

tdrs-backend/tdpservice/email/helpers/admin_notifications.py

+    from tdpservice.users.models import User
+    from tdpservice.email.email_enums import EmailType
+    from tdpservice.email.email import automated_email, log
+    from tdpservice.email.tasks import get_system_owner_email


is there a reason for the imports in the function? i see it in other functions in this file, so just curious

I think originally I got circular error, but will check back again because I changed things around

jtimpe · 2024-11-08T18:48:03Z

tdrs-backend/tdpservice/users/signals.py

+    ACTIONS = {
+            'PRE_REMOVE': 'pre_remove',
+            'PRE_ADD': 'pre_add',
+            'PRE_CLEAR': 'pre_clear'


jtimpe · 2024-11-12T13:29:01Z

tdrs-backend/tdpservice/email/templates/system-admin-role-changed.html

+<!-- Body copy -->
+<p style="color: #000000;">
+
+    <p>The following Admin User account for the TANF Data Portal (TDP) has been {{ action }}.</p>


This reads a little bit weird with "added" and "removed" as the action. Could we do "assigned" and "unassigned"? Or change the sentence a little.

"The System Admin role has been (assigned/unassigned) for a user of the TANF Data Portal (TDP)"

just a suggestion. sounds a little like the account has been deleted otherwise.

jtimpe

minor feedback. works as written

jtimpe · 2024-11-12T14:51:04Z

@raftmsohani one note i forgot to make: the description says "system admins" should receive the email, but i believe it should say "system owners"

also, the addition of the new System Owners group means that the frontend no longer shows the expected links (kibana, grafana, admin)

raftmsohani · 2024-11-12T15:09:03Z

@raftmsohani one note i forgot to make: the description says "system admins" should receive the email, but i believe it should say "system owners"

also, the addition of the new System Owners group means that the frontend no longer shows the expected links (kibana, grafana, admin)

@jtimpe I don't see anything wrong, basically when system admin group is being (un)assigned then system owner should receive email

ADPennington · 2024-11-13T16:43:46Z

Proceed to admin page and create a group called: "System Owner"
Then add this group to one of the users
Change another user's group from/to: "OFA System Admin"
Check the user with system owner group receives email

@raftmsohani is it expected for sys admins to create this new user group? if so, why? Additionally, what permissions does this user group need?

raftmsohani · 2024-11-13T16:52:28Z

Proceed to admin page and create a group called: "System Owner"
Then add this group to one of the users
Change another user's group from/to: "OFA System Admin"
Check the user with system owner group receives email

@raftmsohani is it expected for sys admins to create this new user group? if so, why? Additionally, what permissions does this user group need?

Instead of creating the group in migrations, we decided to create it one manually in prod, and then pull all group creations out of migrations and add it to load data command.

I thought the new SystemOwner group is what the ticket is asking. I assume same permissions as OFAAdmin??

ADPennington · 2024-11-13T16:58:51Z

Proceed to admin page and create a group called: "System Owner"
Then add this group to one of the users
Change another user's group from/to: "OFA System Admin"
Check the user with system owner group receives email

@raftmsohani is it expected for sys admins to create this new user group? if so, why? Additionally, what permissions does this user group need?

Instead of creating the group in migrations, we decided to create it one manually in prod, and then pull all group creations out of migrations and add it to load data command.

I thought the new SystemOwner group is what the ticket is asking. I assume same permissions as OFAAdmin??

okay; is this approach simpler? if so, can you attach a video demonstrating how to do this? @raftmsohani

Send email using signals

2963c1f

raftmsohani self-assigned this Oct 31, 2024

revised email template

72516dc

raftmsohani added the raft review This issue is ready for raft review label Nov 1, 2024

raftmsohani requested review from andrew-jameson, jtimpe and elipe17 November 1, 2024 12:55

Merge branch 'develop' into 1337-sys-admin-permission-change-email

81a5db4

raftmsohani commented Nov 4, 2024

View reviewed changes

tdrs-backend/tdpservice/email/tasks.py Show resolved Hide resolved

raftmsohani commented Nov 5, 2024

View reviewed changes

tdrs-backend/tdpservice/email/helpers/admin_notifications.py Show resolved Hide resolved

raftmsohani added 4 commits November 5, 2024 13:17

corrected removing the user message

8c29b02

Merge branch '1337-sys-admin-permission-change-email' of github.com:r…

74730d0

…aft-tech/TANF-app into 1337-sys-admin-permission-change-email

linting

3e2c6e2

fixed signal receiver

7d42439

andrew-jameson approved these changes Nov 7, 2024

View reviewed changes

Merge branch 'develop' into 1337-sys-admin-permission-change-email

770dcf7

jtimpe reviewed Nov 8, 2024

View reviewed changes

tdrs-backend/tdpservice/users/signals.py

ACTIONS = {

'PRE_REMOVE': 'pre_remove',

'PRE_ADD': 'pre_add',

'PRE_CLEAR': 'pre_clear'

Copy link

jtimpe Nov 8, 2024

Choose a reason for hiding this comment

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

enum?

jtimpe reviewed Nov 12, 2024

View reviewed changes

jtimpe approved these changes Nov 12, 2024

View reviewed changes

raftmsohani added QASP Review and removed raft review This issue is ready for raft review labels Nov 12, 2024

raftmsohani requested review from ADPennington and removed request for elipe17 November 12, 2024 13:48

ADPennington added the Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI label Nov 13, 2024

ADPennington added Blocked Label for Pull Requests that are currently blocked by a dependency and removed Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI labels Nov 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1337 email owner as system admin group (un)assigned #3253

1337 email owner as system admin group (un)assigned #3253

raftmsohani commented Oct 31, 2024 •

edited

Loading

codecov bot commented Nov 6, 2024 •

edited

Loading

andrew-jameson left a comment

jtimpe Nov 8, 2024

raftmsohani Nov 8, 2024

jtimpe Nov 8, 2024

jtimpe Nov 12, 2024 •

edited

Loading

jtimpe left a comment

jtimpe commented Nov 12, 2024

raftmsohani commented Nov 12, 2024

ADPennington commented Nov 13, 2024

raftmsohani commented Nov 13, 2024

ADPennington commented Nov 13, 2024 •

edited

Loading

1337 email owner as system admin group (un)assigned #3253

Are you sure you want to change the base?

1337 email owner as system admin group (un)assigned #3253

Conversation

raftmsohani commented Oct 31, 2024 • edited Loading

Summary of Changes

How to Test

Deliverables

codecov bot commented Nov 6, 2024 • edited Loading

Codecov Report

andrew-jameson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimpe Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

jtimpe left a comment

Choose a reason for hiding this comment

jtimpe commented Nov 12, 2024

raftmsohani commented Nov 12, 2024

ADPennington commented Nov 13, 2024

raftmsohani commented Nov 13, 2024

ADPennington commented Nov 13, 2024 • edited Loading

raftmsohani commented Oct 31, 2024 •

edited

Loading

codecov bot commented Nov 6, 2024 •

edited

Loading

jtimpe Nov 12, 2024 •

edited

Loading

ADPennington commented Nov 13, 2024 •

edited

Loading