Issue #2579 #2644: Analyst can merge user accounts - [BOB] #2649

rachidatecs · 2024-08-24T01:30:12Z

Ticket

Resolves #2579
Resolves #2644

Changes

New view/template for a transfer user page
Sidebar, breadcrumbs (<- @Katherine-Osos instead of a back to link), comparison panes, select and submit
The select is a combo box implemented outside of the DJA context which means spot-loading the required JS and CSS
Modal on submit
~~Transfer of user properties (portfolio and roles/perms)~~
Transfer of joined fields (DomainInfo, DomainRequest, UserDomainRole, VerifiedBy, Portfolio) defined as constants in the view class
Solid testing

Context for reviewers

This turned to be a bit large, so we shed a couple of items to other tickets:

Refactor the join lookup on user transfer to work with the refactored portfolio (multiple per user instead of one) #2644 will refactor the portfolio transfer now that a user can have more than one portfolio
Refactor the Join lookup on transfer user #2645 will attempt implementation of a join lookup that does not rely on constants
The combobox was tricky to implement outside of DJA and is missing styles for dark mode. We can fix this but I suggest we make it another follow-on small ticket <- @abroddrick (should be a 1 pointer, maybe make it 2 and loop in some scss reorg as described below)
I revised how we're dealing with DJA and USWDS button styles. All buttons in our admin should be consistent now but we could do some low priority work in cleaning up the admin scss (time consuming, no change to users, just code hygiene)

Setup

Try transferring users who own doman requests, domains, are creators on portfolios, have entries in the verified by (edge case, I don't think we'll be merging analysts), have a portfolio and roles and perms on it.

Designers

As of Friday night the sandbox build is not running. Check for green checkmarks before you go to bob.

Code Review Verification Steps

As the original developer, I have

Satisfied acceptance criteria and met development standards

Met the acceptance criteria, or will meet them in a subsequent PR
Created/modified automated tests
Added at least 2 developers as PR reviewers (only 1 will need to approve)
Messaged on Slack or in standup to notify the team that a PR is ready for review
Changes to “how we do things” are documented in READMEs and or onboarding guide
If any model was updated to modify/add/delete columns, makemigrations was ran and the associated migrations file has been commited.

Ensured code standards are met (Original Developer)

All new functions and methods are commented using plain language
Did dependency updates in Pipfile also get changed in requirements.txt?
Interactions with external systems are wrapped in try/except
Error handling exists for unusual or missing values

Validated user-facing changes (if applicable)

New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
Checked keyboard navigability
Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
Add at least 1 designer as PR reviewer

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

Pulled this branch locally and tested it
Reviewed this code and left comments
Checked that all code is adequately covered by tests
Made it clear which comments need to be addressed before this work is merged
If any model was updated to modify/add/delete columns, makemigrations was ran and the associated migrations file has been commited.

Ensured code standards are met (Code reviewer)

All new functions and methods are commented using plain language
Interactions with external systems are wrapped in try/except
Error handling exists for unusual or missing values
(Rarely needed) Did dependency updates in Pipfile also get changed in requirements.txt?

Validated user-facing changes as a developer

Note: Multiple code reviewers can share the checklists above, a second reviewers should not make a duplicate checklist

As a designer reviewer, I have

Verified that the changes match the design intention

Checked that the design translated visually
Checked behavior
Checked different states (empty, one, some, error)
Checked for landmarks, page heading structure, and links
Tried to break the intended flow

Validated user-facing changes as a designer

Checked keyboard navigability
Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
Tested with multiple browsers (check off which ones were used)
- Chrome
- Microsoft Edge
- FireFox
- Safari
(Rarely needed) Tested as both an analyst and applicant user

Screenshots

github-actions · 2024-08-24T02:38:59Z

🥳 Successfully deployed to developer sandbox bob.

rachidatecs · 2024-08-24T03:15:19Z

src/registrar/assets/js/get-gov-admin.js

-        console.error("Could not assign current user: no values found.")
-        return;
-    }
+    if (document.getElementById("id_investigator") && django && django.jQuery) {


Relax, this is just an additional check for document.getElementById("id_investigator") && django && django.jQuery) to get rid of a console error. Nothing else in this file.

github-actions · 2024-08-24T03:35:51Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2024-08-24T03:54:43Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2024-08-24T04:01:13Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2024-08-24T04:07:15Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2024-08-28T20:28:36Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2024-08-28T20:53:41Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2024-08-28T21:29:27Z

🥳 Successfully deployed to developer sandbox bob.

Katherine-Osos · 2024-08-28T21:50:04Z

@rachidatecs Looks good, just two small updates:

Can we use a different (muted) button style for the "Select and preview" button? Visually, it competes with the "Transfer ..." button, which is a much more important button. For example, could we use the gray button style that's used for the search fields in the admin?
In the confirmation modal: I reviewed some of the other modals we're using in the "Domains" section, and want to make a few tweaks to this one for consistency.
- Let's update the heading to be a question: "Are you sure you want to transfer data and delete this user?"
- Move the name & email to the body of the message and add the username (since I believe you/Kaitlin noted that's an important unique identifier).
- Update the button label to be a response to the question: "Yes, transfer and delete user"
- See layout below.

Are you sure you want to transfer data and delete this user?
Username: 987654321
Name: John Smith
Email: jsmith@ example.com

This action cannot be undone.

[Yes, transfer and delete user] [Cancel]

github-actions · 2024-08-28T22:22:18Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2024-08-30T15:55:47Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2024-08-30T16:00:33Z

🥳 Successfully deployed to developer sandbox bob.

Matt-Spence

Works perfectly well for me. I have a concern with adding a hole in our csp for select2, but I don't think it's an unacceptable risk. Let me know if there's no acceptably convenient way around using it.

Matt-Spence · 2024-09-05T21:18:51Z

src/registrar/config/settings.py

+CSP_STYLE_SRC = [
+    "'self'",
+    "https://www.ssa.gov/accessibility/andi/andi.css",
+    "https://cdn.jsdelivr.net/npm/[email protected]/dist/css/select2.min.css",


I assume there was no other easy way to do this? Adding another hole in our CSP is not something we should do lightly. If somehow that version alias of select2 were to get replaced with something malicious anybody who accesses a view that loads scripts from this source could be vulnerable.

If it's going to be super expensive to replace this then it may be worth it, but I want to make sure it's something we're considering carefully.

Matt-Spence · 2024-09-05T21:41:26Z

src/registrar/templates/admin/transfer_user.html

+    <script src="{% static 'admin/js/vendor/jquery/jquery.min.js' %}"></script>
+
+    <!-- Include Select2 JavaScript. Since this view technically falls outside of admin, this is needed. -->
+    <script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/js/select2.min.js"></script>


This is what I was referencing earlier. If hypothetically the developers of select2 got hacked (or were deliberately malicious) they could upload arbitrary code at this url and it would execute in the users browser. This is a risk we take with basically all our dependencies, so it's not unacceptable, but again want to make sure it's carefully considered.

select2 makes the django admin combobox possible on the transfer page. It's django source code basically. Without it, the transfer page would have to be registered as a django admin view (which it is not), or we'd need to use USWDS's combobox in this view's context. There might be a way to load this piece of Django's source code from our existing build but I couldn't figure it out.

One option: We download the select2 script and load it from our static folder. Is it worth it though?

^ Legit question, you or Alysia would be the ones best qualified to answer.

I'm cool with this as-is. Just wanted to make sure there wasn't an easy workaround.

asaki222 · 2024-09-05T22:48:13Z

src/registrar/templates/admin/transfer_user.html

+                </h2>
+                <div class="usa-prose">
+                    {% if selected_user != logged_in_user %}
+                        <p>Username: <b>{{ selected_user.username }}</b><br>


Is there an instance where the selected_user would be equal to the logged_in_user? Could we make a better 'else' message if so?

This will be refactored in a future ticket that disallows this feature for any staff (analyst, superuser) user

asaki222 · 2024-09-05T22:55:57Z

src/registrar/tests/test_admin.py

+    def test_transfer_user_modal(self):
+        """Assert modal on page"""
+        user_transfer_page = self.app.get(reverse("transfer_user", args=[self.user1.pk]))
+        self.assertContains(user_transfer_page, "This action cannot be undone.")


I really like the use of tests here for different scenarios!

asaki222 · 2024-09-05T23:00:18Z

src/registrar/views/transfer_user.py

+        current_user.save()
+
+    @classmethod
+    def log_change(cls, obj, field_name, field_value, new_value, change_logs):


This is a really clean way of logging changes!

Matt-Spence

All good

github-actions · 2024-09-06T17:55:51Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2024-09-06T18:02:09Z

🥳 Successfully deployed to developer sandbox bob.

rachidatecs added 2 commits August 23, 2024 16:33

new view, transfer and delete logic, modal, combobox

a7c8017

test and lint

c1df03d

rachidatecs requested a review from abroddrick as a code owner August 24, 2024 01:30

accessibility fix

778ad4d

rachidatecs requested review from therealslimhsiehdy, Matt-Spence, CocoByte, dave-kennedy-ecs, Katherine-Osos, erinysong, asaki222, zandercymatics and a team August 24, 2024 01:42

rachidatecs commented Aug 24, 2024

View reviewed changes

css fixes

875dc1d

add sample kwarg for url test

7ba93d0

zap exclusion

ec6e587

zap exclusion

04157ce

Katherine-Osos removed the request for review from a team August 27, 2024 21:39

rachidatecs added 2 commits August 28, 2024 16:20

design review tweaks

5f21cd9

merge main

c140f26

fix unit test

0076f25

combobox init js

dfdf5ae

design tweaks

ed8541e

Katherine-Osos approved these changes Aug 28, 2024

View reviewed changes

rachidatecs added 2 commits August 30, 2024 11:11

merge main

6a19666

Fix portfolio transfer

e1e812c

rachidatecs changed the title ~~Issue #2579: Analyst can merge user accounts - [BOB]~~ Issue #2579 #2644: Analyst can merge user accounts - [BOB] Aug 30, 2024

cleanup

c4dc380

Matt-Spence self-assigned this Sep 5, 2024

Matt-Spence reviewed Sep 5, 2024

View reviewed changes

asaki222 reviewed Sep 5, 2024

View reviewed changes

Matt-Spence approved these changes Sep 6, 2024

View reviewed changes

merge main

73e3ced

merge cleanup

db593fc

rachidatecs merged commit 59b647b into main Sep 6, 2024
10 checks passed

rachidatecs deleted the bob/2579-analyst-merge-user-accounts branch September 6, 2024 19:44

h-m-f-t mentioned this pull request Sep 25, 2024

Self-host select2 and remove it from CSP exceptions #2826

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #2579 #2644: Analyst can merge user accounts - [BOB] #2649

Issue #2579 #2644: Analyst can merge user accounts - [BOB] #2649

rachidatecs commented Aug 24, 2024 •

edited by abroddrick

Loading

github-actions bot commented Aug 24, 2024

rachidatecs Aug 24, 2024

github-actions bot commented Aug 24, 2024

github-actions bot commented Aug 24, 2024

github-actions bot commented Aug 24, 2024

github-actions bot commented Aug 24, 2024

github-actions bot commented Aug 28, 2024

github-actions bot commented Aug 28, 2024

github-actions bot commented Aug 28, 2024

Katherine-Osos commented Aug 28, 2024

github-actions bot commented Aug 28, 2024

github-actions bot commented Aug 30, 2024

github-actions bot commented Aug 30, 2024

Matt-Spence left a comment

Matt-Spence Sep 5, 2024

Matt-Spence Sep 5, 2024

rachidatecs Sep 6, 2024 •

edited

Loading

rachidatecs Sep 6, 2024

rachidatecs Sep 6, 2024

Matt-Spence Sep 6, 2024

asaki222 Sep 5, 2024

rachidatecs Sep 6, 2024

asaki222 Sep 5, 2024

asaki222 Sep 5, 2024

Matt-Spence left a comment

github-actions bot commented Sep 6, 2024

github-actions bot commented Sep 6, 2024

Issue #2579 #2644: Analyst can merge user accounts - [BOB] #2649

Issue #2579 #2644: Analyst can merge user accounts - [BOB] #2649

Conversation

rachidatecs commented Aug 24, 2024 • edited by abroddrick Loading

Ticket

Changes

Context for reviewers

Setup

Designers

Code Review Verification Steps

As the original developer, I have

Satisfied acceptance criteria and met development standards

Ensured code standards are met (Original Developer)

Validated user-facing changes (if applicable)

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

Ensured code standards are met (Code reviewer)

Validated user-facing changes as a developer

As a designer reviewer, I have

Verified that the changes match the design intention

Validated user-facing changes as a designer

Screenshots

github-actions bot commented Aug 24, 2024

Choose a reason for hiding this comment

github-actions bot commented Aug 24, 2024

github-actions bot commented Aug 24, 2024

github-actions bot commented Aug 24, 2024

github-actions bot commented Aug 24, 2024

github-actions bot commented Aug 28, 2024

github-actions bot commented Aug 28, 2024

github-actions bot commented Aug 28, 2024

Katherine-Osos commented Aug 28, 2024

github-actions bot commented Aug 28, 2024

github-actions bot commented Aug 30, 2024

github-actions bot commented Aug 30, 2024

Matt-Spence left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rachidatecs Sep 6, 2024 • edited Loading

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

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

Matt-Spence left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 6, 2024

github-actions bot commented Sep 6, 2024

rachidatecs commented Aug 24, 2024 •

edited by abroddrick

Loading

rachidatecs Sep 6, 2024 •

edited

Loading