Add error message when Space user exists on Org user removal. #1163

rememberlenny · 2017-07-19T18:17:41Z

…it generic for the user list

…ming of user list notification

jcscottiii · 2017-07-19T19:09:25Z

static_src/constants.js

@@ -267,9 +269,9 @@ const userActionTypes = keymirror({
  // Action when something goes wrong in user invite and email process.
  USER_INVITE_ERROR: null,
  // Action to display an invite notification
-  USER_INVITE_STATUS_DISPLAYED: null,
+  USER_INVITE_STATUS_CREATED: null,
  // Action to dismiss an invite notification


update comment

jcscottiii · 2017-07-19T19:10:11Z

static_src/constants.js

@@ -267,9 +269,9 @@ const userActionTypes = keymirror({
  // Action when something goes wrong in user invite and email process.
  USER_INVITE_ERROR: null,
  // Action to display an invite notification
-  USER_INVITE_STATUS_DISPLAYED: null,
+  USER_INVITE_STATUS_CREATED: null,


i think you meant to get rid of this since you created the new one above https://github.com/18F/cg-dashboard/pull/1163/files#diff-286a478a0e7b0544d442395e82edf6c1R262

This was intentional. I moved away from having multiple constants that result in a notification being displayed. Instead, there are multiple constants that lead to the preparation of a notification, but only one actual constant that leads to the creation of a notification. As a result, this one is changed to reflect the preparation of the invite specific notification.

jcscottiii · 2017-07-19T19:12:41Z

static_src/util/cf_api.js

+    ).catch(res => {
+      const err = parseError(res);
+      if (res && res.response && res.response.status === 400) {
+        if (res.response.data.error_code === 'CF-AssociationNotEmpty') {


can you combine these two conditionals into one long one?

jcscottiii · 2017-07-19T19:16:39Z

static_src/util/cf_api.js

+      const err = parseError(res);
+      if (res && res.response && res.response.status === 400) {
+        if (res.response.data.error_code === 'CF-AssociationNotEmpty') {
+          const description = 'This user can\'t be removed because they still have a Space ' +


Uncapitalize non-proper nouns.
s/Space/space
s/Organization/organization
s/User/user

also, spell out organization.
s/Org/organization

jcscottiii · 2017-07-19T19:17:45Z

static_src/util/cf_api.js

+                              'role within the Organization. Please remove all Space ' +
+                              'associations before removing this User from the Org.';
+          userActions.createUserSpaceAssociationNotification(description);
+          return Promise.reject(err);


you don't need this.

also, a noob JS question here, do you want to have a return on userActions.createUserSpaceAssociationNotification(description);?

In this case, we dont need one. The createUserSpaceAssociationNotification kicks off the notification, but doesnt return anything.

jcscottiii

a few changes

el-mapache

As a general note, I don't hate the name inviteNotice. userListInviteNotice basically ties the data to a specific component. Was it changed because the UserList is the only current component with the invite functionality?

el-mapache · 2017-07-19T19:32:33Z

static_src/actions/user_actions.js

    });
  },

+  createUserListNotification(noticeType, description) {


I think we should start writing new actions in this format:

type: RAD_TYPE, data: { datum1: false, datum2: true }

Then every action presents a consistent interface regardless of the information being passed in. What do you think?

I think thats a good idea. It currently uses the ES6 syntax for exampling the variable name as the key. Instead of data, it does what you are describing with action. I think it would be a lot of refactoring to change them all.

i like the idea as well. but let's save that for a broader discussion.

el-mapache · 2017-07-19T19:32:58Z

static_src/actions/user_actions.js

+  },
+
+  createUserSpaceAssociationNotification(notification) {
+    const description = notification;


I think these are unnecessary

rememberlenny · 2017-07-19T21:54:24Z

@el-mapache Regarding the naming, Im a proponent of breaking out a global notification action. For now, this is highly tied into the User List panel because of where it sits. I was proposing starting with a global notification component, but since we didnt go that route, I think its better to explicitly tie the functionality to the location.

jcscottiii · 2017-07-20T04:43:00Z

static_src/constants.js

-  // Action to display an invite notification
-  USER_INVITE_STATUS_DISPLAYED: null,
+  // Action to prepare an invite notification
+  USER_INVITE_STATUS_CREATED: null,


you dont need this. you have USER_LIST_NOTICE_CREATED

jcscottiii · 2017-07-20T14:48:19Z

static_src/util/cf_api.js

+                               res.response.data.error_code === 'CF-AssociationNotEmpty'
+                              );
+      if (errorConditions) {
+        const description = 'This user can\'t be removed because they still have a space ' +


you don't have a test that calls deleteOrgUserPermissions and verifies that createUserSpaceAssociationNotification is called with the description

jcscottiii · 2017-07-20T19:30:48Z

static_src/actions/user_actions.js

+        } else {
+          userActions.deleteUser(userGuid, orgGuid);
+        }
+      });


we need a catch here

fureigh

Looking good! I added a few inline questions and comments. Once those are addressed, let's .

fureigh · 2017-07-20T22:41:06Z

static_src/actions/user_actions.js

+        });
+        if (usersSpaces.length > 0) {
+          const description = 'This user can\'t be removed because they still have a space ' +
+                          'role within the organization. Please remove all space ' +


The natural response to "Please remove all space associations" is "How/where do I do that?" Let's add a link or a few words to point them in the right direction.

fureigh · 2017-07-20T22:42:13Z

static_src/constants.js

@@ -266,10 +268,8 @@ const userActionTypes = keymirror({
  USER_ASSOCIATED_ORG_DISPLAYED: null,
  // Action when something goes wrong in user invite and email process.
  USER_INVITE_ERROR: null,
-  // Action to display an invite notification
-  USER_INVITE_STATUS_DISPLAYED: null,
  // Action to dismiss an invite notification


Is this comment still accurate?

fureigh · 2017-07-20T22:43:12Z

static_src/test/unit/util/cf_api.spec.js

@@ -1007,6 +1007,46 @@ describe('cfApi', function() {
        done();
      }).catch(done.fail);
    });
+    describe('error handling', function() {
+      it('should return display a generic error if unsuccessful', function(done) {


Is return display the intended phrasing?

fureigh · 2017-07-20T22:43:17Z

static_src/test/unit/util/cf_api.spec.js

+        }).catch(done.fail);
+      });
+
+      it('should return display a notification about user having existing roles', function(done) {


Is return display the intended phrasing?

fureigh · 2017-07-20T22:43:33Z

static_src/test/unit/util/cf_api.spec.js

+          response: {error_code: 'CF-AssociationNotEmpty'},
+        });
+
+        const description = "This user can't be removed because they still " +


Just noting that this will need to be updated if the other piece is.

fureigh · 2017-07-20T22:44:05Z

static_src/util/cf_api.js

@@ -386,9 +386,23 @@ export default {
      .then(() => {
        userActions.deletedUser(userGuid, orgGuid);
      }).catch((err) => {
-        if (err.response.data) {
-          userActions.errorRemoveUser(userGuid, err.data);
+        // Check if we got caught on user roles in spaces


if --> whether

(not a big deal)

fureigh · 2017-07-20T22:44:09Z

static_src/util/cf_api.js

+          error.response.data.error_code === 'CF-AssociationNotEmpty'
+        );
+        if (userHasSpaceRoles) {
+          const description = 'This user can\'t be removed because they still have a space ' +


Similar note as above.

let's talk about the need for deleteUserIfNoSpaceAssociation

rememberlenny · 2017-07-21T04:27:42Z

All the changes should be reflected! @jcscottiii @fureigh

rememberlenny · 2017-07-21T05:07:00Z

Note: I added some new documentation in the cg-site repo to correspond with the instructions about managing users.

cloud-gov/cg-site#977

jcscottiii · 2017-07-21T13:10:34Z

we need to talk because we have the description twice.
https://github.com/18F/cg-dashboard/pull/1163/files#diff-8a5ca76252b92305d2b0c5dece9c18fdR81

https://github.com/18F/cg-dashboard/pull/1163/files#diff-d45ddca2d392b7558d8957b40dd036ccR397

needs to be a constant or something

also, per my comment as to why i removed my approval last night

let's talk about the need for deleteUserIfNoSpaceAssociation

we need tests for that stuff.

el-mapache · 2017-07-21T15:19:26Z

static_src/util/cf_api.js

@@ -386,9 +386,24 @@ export default {
      .then(() => {
        userActions.deletedUser(userGuid, orgGuid);
      }).catch((err) => {
-        if (err.response.data) {
-          userActions.errorRemoveUser(userGuid, err.data);
+        // Check whether we got caught on user roles in spaces


I think we can probably move this error handling up into the calling function, it gives us a little more flexibility in case we want to handle the error another way in different contexts.

el-mapache · 2017-07-21T15:19:57Z

static_src/actions/user_actions.js

@@ -60,6 +60,35 @@ const userActions = {
    });
  },

+  receivedOrgSpacesToExtractSpaceUsers(orgSpaces) {


I think these methods need some tests.

fureigh

Thanks for those changes! I added some missing spaces in a quick commit (d9984cc).

LGTM, pending @el-mapache and @jcscottiii's comments being addressed.

…iation

rememberlenny · 2017-07-24T14:55:50Z

Looks good!

rememberlenny added 7 commits July 19, 2017 14:14

Create a new catch to resole issues with existing space association

88c99a3

Rename the constants associated to user invite notification and make …

c48c3eb

…it generic for the user list

Update tests and naming of actions associated to user store

4487850

Add the new errors in the user store and change the invite naming

28b56da

Update the naming of invite notification tests

b123cd9

Update the user actions to remove direct invite affiliation during na…

f4b550f

…ming of user list notification

Update naming convention around the user notifications on the user list

e8c967e

rememberlenny requested review from el-mapache and jcscottiii July 19, 2017 18:19

Lint

1e30a66

jcscottiii reviewed Jul 19, 2017

View reviewed changes

jcscottiii suggested changes Jul 19, 2017

View reviewed changes

el-mapache reviewed Jul 19, 2017

View reviewed changes

jcscottiii requested a review from fureigh July 19, 2017 19:43

rememberlenny added 2 commits July 19, 2017 17:41

Fix up errors

d0a8766

Updates for syntax and simplicity

faf2819

jcscottiii reviewed Jul 20, 2017

View reviewed changes

Remove the USER_INVITE_STATUS_CREATED

f782d65

jcscottiii reviewed Jul 20, 2017

View reviewed changes

James C. Scott and others added 5 commits July 20, 2017 11:46

add test for emitting description of being unable to remove user

24b0ba7

Create a preliminary check before the users are verified for deletion

01eef76

Add an action to check a current user's space associations

ba99e8d

Create the additional cf commands for the space/org

67d0596

Create a wrapper around the promises

0ed162f

jcscottiii reviewed Jul 20, 2017

View reviewed changes

static_src/actions/user_actions.js Outdated

} else {

userActions.deleteUser(userGuid, orgGuid);

}

});

Copy link

Contributor

jcscottiii Jul 20, 2017

Choose a reason for hiding this comment

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

we need a catch here

James C. Scott and others added 2 commits July 20, 2017 17:47

remove extra calls

9e8ea37

Merge

fd13220

jcscottiii previously approved these changes Jul 20, 2017

View reviewed changes

Fix the recall of the user delete

46bbd74

fureigh suggested changes Jul 20, 2017

View reviewed changes

rememberlenny added 3 commits July 20, 2017 23:02

Update the linting around promises

684ab14

Update comments on the constants for user list notices

45bc78b

Update the changes to the notification

b7815d0

rememberlenny mentioned this pull request Jul 21, 2017

Add documentation around removing a user from an organization or space cloud-gov/cg-site#977

Merged

el-mapache reviewed Jul 21, 2017

View reviewed changes

rememberlenny and others added 2 commits July 21, 2017 13:32

Add tests for receivedOrgSpacesToExtractSpaceUsers

3c72fe2

Add missing spaces

d9984cc

fureigh approved these changes Jul 21, 2017

View reviewed changes

rememberlenny added 4 commits July 21, 2017 14:14

Add missing cf api request for fetchAllOrgSpaces

49465de

Create a promise resolve on the deleteUserIfNoSpaceAssociation

22ecabf

Add tests on all functions associated to the deleteUserIfNoSpaceAssoc…

e00d4a6

…iation

Merge with space update

2afb68e

rememberlenny force-pushed the lkb-space-remove branch from da5e2be to 2afb68e Compare July 21, 2017 22:15

James C. Scott added 3 commits July 24, 2017 08:58

bring back code

07e0ddb

fix tests for post deleteUser action

237dc7e

fix test comment

607daf9

jcscottiii approved these changes Jul 24, 2017

View reviewed changes

rememberlenny merged commit c447c73 into master Jul 24, 2017

rememberlenny deleted the lkb-space-remove branch July 24, 2017 14:56

Add error message when Space user exists on Org user removal. #1163

Add error message when Space user exists on Org user removal. #1163

Conversation

rememberlenny commented Jul 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcscottiii Jul 19, 2017 • 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

rememberlenny Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

jcscottiii left a comment

Choose a reason for hiding this comment

el-mapache 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rememberlenny commented Jul 19, 2017

Choose a reason for hiding this comment

jcscottiii Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fureigh 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

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

rememberlenny commented Jul 21, 2017

rememberlenny commented Jul 21, 2017

jcscottiii commented Jul 21, 2017 • edited Loading

el-mapache Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fureigh left a comment

Choose a reason for hiding this comment

rememberlenny commented Jul 24, 2017

jcscottiii Jul 19, 2017 •

edited

Loading

rememberlenny Jul 19, 2017 •

edited

Loading

jcscottiii Jul 20, 2017 •

edited

Loading

jcscottiii commented Jul 21, 2017 •

edited

Loading

el-mapache Jul 21, 2017 •

edited

Loading