Task/fp 1183 complete saga redux for removing user #474

nathanfranklin · 2021-08-28T01:33:38Z

Overview:

Complete removing user saga/redux.

Also added busy spinner and error message for those two cases when trying to remove a user in in modal.

Related Jira tickets:

FP-1183

Testing Steps:

Remove a user

UI Photos:

Notes:

Polishing of component will come later but I want to do the following two things quickly but i can do them in a separate PR later if that makes more sense:

test saga (normal case + failure case)
refactor removing of user from state from reducer to saga

Spinner is shown while deleting and error is shown when failed

codecov · 2021-08-28T01:36:44Z

Codecov Report

Merging #474 (a6943e8) into task/FP-737-no-ui (a137635) will decrease coverage by 1.23%.
The diff coverage is 91.17%.

@@                  Coverage Diff                  @@
##           task/FP-737-no-ui     #474      +/-   ##
=====================================================
- Coverage              65.31%   64.08%   -1.24%     
=====================================================
  Files                    405      198     -207     
  Lines                  12341     7063    -5278     
  Branches                2163      763    -1400     
=====================================================
- Hits                    8061     4526    -3535     
+ Misses                  3956     2334    -1622     
+ Partials                 324      203     -121

Flag	Coverage Δ
javascript	`?`
unittests	`64.08% <91.17%> (+0.20%)`	⬆️

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

Impacted Files	Coverage Δ
server/portal/apps/search/tasks.py	`38.29% <ø> (+2.09%)`	⬆️
server/portal/settings/settings_default.py	`0.00% <ø> (ø)`
server/portal/settings/unit_test_settings.py	`99.02% <ø> (ø)`
server/portal/apps/users/views.py	`59.22% <84.61%> (+4.38%)`	⬆️
server/portal/apps/users/utils.py	`65.09% <94.11%> (+7.02%)`	⬆️
...rver/portal/apps/accounts/managers/user_systems.py	`82.92% <100.00%> (ø)`
server/portal/apps/system_creation/utils.py	`100.00% <100.00%> (ø)`
server/portal/apps/users/urls.py	`100.00% <100.00%> (ø)`
server/portal/celery.py	`84.61% <100.00%> (ø)`
...dal/AllocationsUsageTable/AllocationsUsageTable.js
... and 163 more

Refactor some fixture into a fixture file

…x-for-removing-user

rstijerina

LGTM, some comments about structure/naming

rstijerina · 2021-09-01T18:34:29Z

client/src/redux/reducers/allocations.reducers.js

@@ -75,9 +81,23 @@ function allocations(state = initialState, action) {
        ...state,
        search: { ...state.search, results: action.payload.data }
      };
+    case 'ALLOCATION_OPERATION_REMOVE_USER_INIT': {


Not super important, but I was thinking about this verbosity. Do you think it's better than something like ALLOCATION_REMOVE_USER_INIT?

Yes. good idea

client/src/redux/sagas/allocations.sagas.js

rstijerina · 2021-09-01T18:38:49Z

client/src/redux/sagas/allocations.sagas.js

+    yield put({
+      type: 'ALLOCATION_OPERATION_REMOVE_USER_STATUS',
+      payload: {
+        removingUserOperation: {


Based on how we've been handling reducer state updates in the past, should we move the removingUserOperation portion of this payload to the reducer? That way it's clear what the reducer is doing to the state

Yes, so you are saying change the type here to something like "ALLOCATION_OPERATION_REMOVE_USER_START" (Note the START bit at the end and then only the username would be in the payload. Can you confirm if I am understanding correctly?

Yes, or you can even keep it as ..._STATUS or ..._UPDATE and pass loading and error as well

rstijerina · 2021-09-01T18:41:03Z

client/src/redux/reducers/allocations.reducers.js

+    case 'ALLOCATION_OPERATION_REMOVE_USER_INIT': {
+      return {
+        ...state,
+        removingUserOperation: {


Would this be better nested in an operations object, where we can include addUser as well?

Yes, totally. Would it be cool if I pick up the todo item Extend saga/reducer so that added user is added to list and then do that there?

Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com>

nathanfranklin · 2021-09-01T18:52:00Z

@rstijerina , I think i understand all your comments (but would like to confirm #474 (comment)).

Is it okay if I merge as-is. I would then pick up the Extend saga/reducer so that added user is added to list todo item for the 737 work and address these comments then as they sorta relate. cool?

rstijerina · 2021-09-01T18:53:20Z

@rstijerina , I think i understand all your comments (but would like to confirm #474 (comment)).

Is it okay if I merge as-is. I would then pick up the Extend saga/reducer so that added user is added to list todo item for the 737 work and address these comments then as they sorta relate. cool?

Yep 👍

* Scaffolding * stopgap * No UI; backend ready * Task/FP-1168: add backend unit tests for add/delete members of TAS projects (subtask of FP-737) (#467) * Remove unneeded change * Fix url for user delete * Remove logging statements and correct ApiException Second argument of ApiException is status (i.e. status_code) * Remove status code in ApiException as TAS returns 200 Main reason for not using TAS return code is that for errors (like missing user), TAS still returns a 200. Also, it isn't useful as we handle all ApiException as 400 and don't log that status_code that that was pass to ApiException * Add tests * Fix pep errors * Remove change to response for testing purposes * Fix use of ApiException * Fix tests * Improve logging message for forced allocation retrieval * Fix logging for add/delete of users to allocation * task/FP-737, task/FP-1180: Use Search component from Projects (#477) * use projects search; add common component * revert local dev change * Task/fp 1183 complete saga redux for removing user (#474) * Complete saga/redux for removing user Spinner is shown while deleting and error is shown when failed * Hide remove button for PI user * Add unit tests Refactor some fixture into a fixture file * Add remove user failure saga test * Refactor removing of user from team from reducer to saga * Update client/src/redux/sagas/allocations.sagas.js Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com> Co-authored-by: rstijerina <r.sal.tijerina@gmail.com> * task/FP-1190: Unified search for FP-737 (#479) * unified search * update placeholder * remove autocomplete package * merge main package files * FP/737: Polish UI and Add User Functionality (FP-1182 & FP-1198) (#514) * Add user functionality and styling * Text and variable name changes Co-authored-by: rstijerina <r.sal.tijerina@gmail.com> * Task/FP-1181: Basic unit tests for front-end components (#555) * Fix failing tests * Basic unit test for Allocations Management Modal Co-authored-by: Nathan Franklin <nfranklin@tacc.utexas.edu> Co-authored-by: rstijerina <r.sal.tijerina@gmail.com>

* Task/FP-737: Manage Allocations (#480) * Scaffolding * stopgap * No UI; backend ready * Task/FP-1168: add backend unit tests for add/delete members of TAS projects (subtask of FP-737) (#467) * Remove unneeded change * Fix url for user delete * Remove logging statements and correct ApiException Second argument of ApiException is status (i.e. status_code) * Remove status code in ApiException as TAS returns 200 Main reason for not using TAS return code is that for errors (like missing user), TAS still returns a 200. Also, it isn't useful as we handle all ApiException as 400 and don't log that status_code that that was pass to ApiException * Add tests * Fix pep errors * Remove change to response for testing purposes * Fix use of ApiException * Fix tests * Improve logging message for forced allocation retrieval * Fix logging for add/delete of users to allocation * task/FP-737, task/FP-1180: Use Search component from Projects (#477) * use projects search; add common component * revert local dev change * Task/fp 1183 complete saga redux for removing user (#474) * Complete saga/redux for removing user Spinner is shown while deleting and error is shown when failed * Hide remove button for PI user * Add unit tests Refactor some fixture into a fixture file * Add remove user failure saga test * Refactor removing of user from team from reducer to saga * Update client/src/redux/sagas/allocations.sagas.js Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com> Co-authored-by: rstijerina <r.sal.tijerina@gmail.com> * task/FP-1190: Unified search for FP-737 (#479) * unified search * update placeholder * remove autocomplete package * merge main package files * FP/737: Polish UI and Add User Functionality (FP-1182 & FP-1198) (#514) * Add user functionality and styling * Text and variable name changes Co-authored-by: rstijerina <r.sal.tijerina@gmail.com> * Task/FP-1181: Basic unit tests for front-end components (#555) * Fix failing tests * Basic unit test for Allocations Management Modal Co-authored-by: Nathan Franklin <nfranklin@tacc.utexas.edu> Co-authored-by: rstijerina <r.sal.tijerina@gmail.com> * Task/FP-1309 user new user search endpoint (#572) * Add TAS searching endpoint * Add basic unit tests * Use new endpoint and make temp changes to searchbar * Fix use of team members before ready * Improve tests * Add message to error response Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com> * Add docstr Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com> * Fix error Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com> * Add comment to explain use of Soap and Zeep Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com> Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com> * fix linting * Updated UserSearchbar and AllocationsManageTeamModal to jsx components * Task/FP-1269: Enable Manage Team modal for Project Delegates (#585) * rename js files to jsx * Modal switch between view and manage for pi & delegates * fix linting * Fixed unit tests * Fixed linting Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com> Co-authored-by: Garrett Edmonds <edmondsgarrett@gmail.com> * Fixed unit tests * Fixed AddUser allocations saga bug * Added error handling for AllocationsManageTeamModal * 737: Improve UI of user searching (#624) * Removed searchbar placeholder text and added help text under searchbar in AllocationsManageTeamModal * Simplified JS for text field and updated CSS to accommodate JS changes * Fixed userSearchbar Add button bug * task/FP-1635: 737 - Implement final Allocations UI (#636) * Update View Team link button to secondary button * Remove modal component from this file and export only manage team table component * Add manage team modal render here, incorporate tab functionality, and update role column to now be a dropdown * CSS styling for tabs in View modal * Change 'Manage Allocations' button to read 'Request New Allocation' * Remove errant reference to ManageTeamModal * Fix ManageTeamTable import, remove unused prop, and conditionally render 'Manage Team' tab * Fixed layout unit test, and moved ManageTeamModal test to AllocationsModals tests * Changed AllocationsManageTeamModal to AllocationsManageTeamTable and adjusted all references to this component * Fixed linting * Fixed adding user bug, removed horizontal scroll on manage team table, replaced dropdown in manage team role column with role text, and removed errant log * feat(fp-737): global mui tabs, simpler mui jsx (#646) * feat(fp-1635): remove unused css (#650) * 737: Use `_common` `<Button>` component in `<UserSearchBar>` component (#647) * Use common button component in UserSearchBar * fix linting * Removed manage team table horizontal scrollability + right-side border, and fixed saga state reference. * Fix linting * Fixed state loadingUsernames references to correctly update when adding users to allocation * Fixed key error for manage team table * Removed help text at bottom of manage team tab and adjusted modal size to accommodate this * Fixed another loadingUsernames reference in saga for failing unit test * 737: (FP-1678) Use "secondary" Button for UserSearchBar (#657) * fix(fp-1678): secondary button for usersearchbar * fix(fp-1678): button above (z) input usersearchbar * 737: Use `_common` `<Button>` component in View Team button (#649) * feat(fp-737): use our button for view team button I am not sure if design knows the color change is coming... Inquiry: https://tacc-team.slack.com/archives/G01K9ET7JM7/p1653604140397549 * fix(fp-1678): new styles vers. = more button pad. * fix(fp-1678): use small button * fix(fp-1678): do not let button overlap `<th>` * fix(fp-1678): prettier:fix + related comment tweak * fix(fp-1678): install @tacc/core-styles@0.5.2-c 1. I messed up 0.5.2 publish. After unpublish, I can't republish. 2. I messed up 0.5.2(-)b publish. After unpublish, I can't republish. 3. I published up 0.5.2-c. I also moved v tag beyond the fix, planning I could republish as 0.5.2. So GitHub is at 0.5.2. And NPM is at 0.5.2-c. But there is an 0.6.x from tup-ui/…/core-styles, so 0.5.x ends. Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com> * fix(fp-737): make ui closer to design (#655) * fix(fp-737): make ui closer to design * docs(fp-737): note why set button position static * fix(fp-1678): manage team list wrap height change This value makes modals height match but only at specific window height. Setting height of modals is better but led me to change too much code. Instead, a TACC Modal component could handle height from prop. * fix(fp-1678): validation error * fix(fp-1678): add space below manage team table * fix(fp-1678): revert change to font size Design, a while back, approved 14px font size for tables. Reference: #401 * fix(fp-1650): `<td>` vert padding to match design * fix(fp-1650): port classname validation error fix Source: 38af56b * fix(fp-1650): npm run prettier:fix Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com> * Updated loading state of manage team add button to switch to loading once addUser action is fired Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com> Co-authored-by: Wesley B <62723358+wesleyboar@users.noreply.github.com> * Increased allocations table alignment style selector specificity and fixed background styling for disabled 'remove' button in manage team modal * Frontend error handling for add/remove operations * Render nulls instead of empty strings in absence of user operation errors * fix(fp-737): remove bkgd color from disabled link (#659) * fix(fp-737): remove bkgd color from disabled link Similar result as d60ff3e, but without overwriting CSS. * feat(fp-737): prettier:fix Co-authored-by: Owais Jamil <47395902+owaisj@users.noreply.github.com> Co-authored-by: Nathan Franklin <nfranklin@tacc.utexas.edu> Co-authored-by: Nathan Franklin <nathan.franklin@gmail.com> Co-authored-by: Owais Jamil <ojamil@tacc.utexas.edu> Co-authored-by: edmondsgarrett <43251554+edmondsgarrett@users.noreply.github.com> Co-authored-by: Garrett Edmonds <edmondsgarrett@gmail.com> Co-authored-by: Wesley B <62723358+wesleyboar@users.noreply.github.com>

nathanfranklin added 2 commits August 27, 2021 20:21

Complete saga/redux for removing user

ecdfab6

Spinner is shown while deleting and error is shown when failed

Hide remove button for PI user

45a93ac

nathanfranklin requested review from rstijerina and owaisj August 28, 2021 01:33

nathanfranklin changed the base branch from main to task/FP-737-no-ui August 28, 2021 01:34

nathanfranklin and others added 4 commits August 30, 2021 20:50

Add unit tests

ee8f2fd

Refactor some fixture into a fixture file

Add remove user failure saga test

baf42cf

Refactor removing of user from team from reducer to saga

89e8a5f

Merge branch 'task/FP-737-no-ui' into task/FP-1183-complete-saga-redu…

16b7cbb

…x-for-removing-user

rstijerina reviewed Sep 1, 2021

View reviewed changes

Update client/src/redux/sagas/allocations.sagas.js

a6943e8

Co-authored-by: Sal Tijerina <r.sal.tijerina@gmail.com>

rstijerina approved these changes Sep 1, 2021

View reviewed changes

rstijerina merged commit ec6c3c5 into task/FP-737-no-ui Sep 1, 2021

rstijerina deleted the task/FP-1183-complete-saga-redux-for-removing-user branch September 1, 2021 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task/fp 1183 complete saga redux for removing user #474

Task/fp 1183 complete saga redux for removing user #474

nathanfranklin commented Aug 28, 2021 •

edited

Loading

codecov bot commented Aug 28, 2021 •

edited

Loading

rstijerina left a comment

rstijerina Sep 1, 2021 •

edited

Loading

nathanfranklin Sep 1, 2021

rstijerina Sep 1, 2021

nathanfranklin Sep 1, 2021

rstijerina Sep 1, 2021 •

edited

Loading

rstijerina Sep 1, 2021

nathanfranklin Sep 1, 2021

rstijerina Sep 1, 2021

nathanfranklin commented Sep 1, 2021

rstijerina commented Sep 1, 2021

Task/fp 1183 complete saga redux for removing user #474

Task/fp 1183 complete saga redux for removing user #474

Conversation

nathanfranklin commented Aug 28, 2021 • edited Loading

Overview:

Related Jira tickets:

Testing Steps:

UI Photos:

Notes:

codecov bot commented Aug 28, 2021 • edited Loading

Codecov Report

rstijerina left a comment

Choose a reason for hiding this comment

rstijerina Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

nathanfranklin Sep 1, 2021

Choose a reason for hiding this comment

rstijerina Sep 1, 2021

Choose a reason for hiding this comment

nathanfranklin Sep 1, 2021

Choose a reason for hiding this comment

rstijerina Sep 1, 2021 • edited Loading

Choose a reason for hiding this comment

rstijerina Sep 1, 2021

Choose a reason for hiding this comment

nathanfranklin Sep 1, 2021

Choose a reason for hiding this comment

rstijerina Sep 1, 2021

Choose a reason for hiding this comment

nathanfranklin commented Sep 1, 2021

rstijerina commented Sep 1, 2021

nathanfranklin commented Aug 28, 2021 •

edited

Loading

codecov bot commented Aug 28, 2021 •

edited

Loading

rstijerina Sep 1, 2021 •

edited

Loading

rstijerina Sep 1, 2021 •

edited

Loading