Skip to content
This repository has been archived by the owner on May 19, 2020. It is now read-only.

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

Merged
merged 33 commits into from
Jul 24, 2017
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
88c99a3
Create a new catch to resole issues with existing space association
rememberlenny Jul 19, 2017
c48c3eb
Rename the constants associated to user invite notification and make …
rememberlenny Jul 19, 2017
4487850
Update tests and naming of actions associated to user store
rememberlenny Jul 19, 2017
28b56da
Add the new errors in the user store and change the invite naming
rememberlenny Jul 19, 2017
b123cd9
Update the naming of invite notification tests
rememberlenny Jul 19, 2017
f4b550f
Update the user actions to remove direct invite affiliation during na…
rememberlenny Jul 19, 2017
e8c967e
Update naming convention around the user notifications on the user list
rememberlenny Jul 19, 2017
1e30a66
Lint
rememberlenny Jul 19, 2017
d0a8766
Fix up errors
rememberlenny Jul 19, 2017
faf2819
Updates for syntax and simplicity
rememberlenny Jul 19, 2017
f782d65
Remove the USER_INVITE_STATUS_CREATED
rememberlenny Jul 20, 2017
24b0ba7
add test for emitting description of being unable to remove user
Jul 20, 2017
01eef76
Create a preliminary check before the users are verified for deletion
rememberlenny Jul 20, 2017
ba99e8d
Add an action to check a current user's space associations
rememberlenny Jul 20, 2017
67d0596
Create the additional cf commands for the space/org
rememberlenny Jul 20, 2017
0ed162f
Create a wrapper around the promises
rememberlenny Jul 20, 2017
3c883d5
move error handling logic to deleteUser
Jul 20, 2017
792214e
remove pre-emptive calls
Jul 20, 2017
9e8ea37
remove extra calls
Jul 20, 2017
fd13220
Merge
rememberlenny Jul 20, 2017
46bbd74
Fix the recall of the user delete
rememberlenny Jul 20, 2017
684ab14
Update the linting around promises
rememberlenny Jul 21, 2017
45bc78b
Update comments on the constants for user list notices
rememberlenny Jul 21, 2017
b7815d0
Update the changes to the notification
rememberlenny Jul 21, 2017
3c72fe2
Add tests for receivedOrgSpacesToExtractSpaceUsers
rememberlenny Jul 21, 2017
d9984cc
Add missing spaces
fureigh Jul 21, 2017
49465de
Add missing cf api request for fetchAllOrgSpaces
rememberlenny Jul 21, 2017
22ecabf
Create a promise resolve on the deleteUserIfNoSpaceAssociation
rememberlenny Jul 21, 2017
e00d4a6
Add tests on all functions associated to the deleteUserIfNoSpaceAssoc…
rememberlenny Jul 21, 2017
2afb68e
Merge with space update
rememberlenny Jul 21, 2017
07e0ddb
bring back code
Jul 24, 2017
237dc7e
fix tests for post deleteUser action
Jul 24, 2017
607daf9
fix test comment
Jul 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions static_src/actions/user_actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,25 @@ const userActions = {

clearInviteNotifications() {
AppDispatcher.handleViewAction({
type: userActionTypes.USER_INVITE_STATUS_DISMISSED
type: userActionTypes.USER_LIST_NOTICE_DISMISSED
});
},

createUserListNotification(noticeType, description) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

AppDispatcher.handleViewAction({
type: userActionTypes.USER_LIST_NOTICE_CREATED,
noticeType,
description
});
},

createUserSpaceAssociationNotification(notification) {
const description = notification;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these are unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified

const noticeType = 'error';

userActions.createUserListNotification(noticeType, description);
},

createInviteNotification(verified, email) {
let description;
const noticeType = 'finish';
Expand All @@ -223,11 +238,7 @@ const userActions = {
'be controlled below.';
}

AppDispatcher.handleViewAction({
type: userActionTypes.USER_INVITE_STATUS_DISPLAYED,
noticeType,
description
});
userActions.createUserListNotification(noticeType, description);
},

userInviteError(err, contextualMessage) {
Expand Down
12 changes: 6 additions & 6 deletions static_src/components/users.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ function stateSetter() {
loading: UserStore.loading,
empty: !UserStore.loading && !users.length,
users,
inviteNotices: UserStore._inviteNotification,
userInviteError: UserStore.getInviteError()
userListNotices: UserStore._userListNotification,
userListNoticeError: UserStore.getUserListNotificationError()
};
}

Expand Down Expand Up @@ -143,14 +143,14 @@ export default class Users extends React.Component {
let notification;
let userInvite;

if (this.state.inviteNotices.description) {
const notice = this.state.inviteNotices;
if (this.state.userListNotices.description) {
const notice = this.state.userListNotices;
notification = (
<Notification
message={ notice.description }
actions={ [] }
onDismiss={ this.onNotificationDismiss }
status="finish"
status={ notice.noticeType }
/>
);
} else {
Expand All @@ -164,7 +164,7 @@ export default class Users extends React.Component {
<UsersInvite
inviteDisabled={ this.state.inviteDisabled }
currentUserAccess={ this.state.currentUserAccess }
error={ this.state.userInviteError }
error={ this.state.userListNoticeError }
/>
</div>
);
Expand Down
6 changes: 4 additions & 2 deletions static_src/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,8 @@ const userActionTypes = keymirror({
USER_INVITE_TRIGGER: null,
// Action to trigger when invite status is triggered for front end.
USER_INVITE_STATUS_UPDATED: null,
// Action to trigger when user list notice is created.
USER_LIST_NOTICE_CREATED: null,
// Action to trigger email sent to user with cloud.gov invite url.
USER_ORG_ASSOCIATE: null,
// Action to associate user to organization on the server.
Expand All @@ -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,
Copy link
Contributor

@jcscottiii jcscottiii Jul 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

you dont need this. you have USER_LIST_NOTICE_CREATED

// Action to dismiss an invite notification
Copy link
Contributor

Choose a reason for hiding this comment

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

update comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it!

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment still accurate?

USER_INVITE_STATUS_DISMISSED: null,
USER_LIST_NOTICE_DISMISSED: null,
// Action to delete a user from an org.
USER_DELETE: null,
// Action when a user was deleted from an org on the server.
Expand Down
26 changes: 13 additions & 13 deletions static_src/stores/user_store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export class UserStore extends BaseStore {
this._error = null;
this._saving = false;
this._inviteDisabled = false;
this._inviteNotification = {};
this._userListNotification = {};
this._loading = {};
}

Expand Down Expand Up @@ -64,7 +64,7 @@ export class UserStore extends BaseStore {

case userActionTypes.USER_INVITE_TRIGGER: {
this._inviteDisabled = true;
this._inviteError = null;
this._userListNotificationError = null;
this.emitChange();
break;
}
Expand Down Expand Up @@ -179,7 +179,7 @@ export class UserStore extends BaseStore {
}

case userActionTypes.USER_INVITE_ERROR: {
this._inviteError = Object.assign({}, action.err, {
this._userListNotificationError = Object.assign({}, action.err, {
contextualMessage: action.contextualMessage
});
this._inviteDisabled = false;
Expand All @@ -197,18 +197,18 @@ export class UserStore extends BaseStore {
break;
}

case userActionTypes.USER_INVITE_STATUS_DISPLAYED: {
case userActionTypes.USER_LIST_NOTICE_CREATED: {
this._inviteDisabled = false;
const noticeType = action.noticeType;
const description = action.description;
const notice = Object.assign({}, { noticeType }, { description });
this._inviteNotification = notice;
this._userListNotification = notice;
this.emitChange();
break;
}

case userActionTypes.USER_INVITE_STATUS_DISMISSED: {
this._inviteNotification = {};
case userActionTypes.USER_LIST_NOTICE_DISMISSED: {
this._userListNotification = {};
this.emitChange();
break;
}
Expand Down Expand Up @@ -285,8 +285,8 @@ export class UserStore extends BaseStore {
case errorActionTypes.CLEAR: {
this._error = null;
this._saving = false;
this._inviteNotification = {};
this._inviteError = null;
this._userListNotification = {};
this._userListNotificationError = null;
this._loading = {};
this.emitChange();
break;
Expand Down Expand Up @@ -367,12 +367,12 @@ export class UserStore extends BaseStore {
return this._currentUserIsAdmin;
}

getInviteNotification() {
return this._inviteNotification;
getUserListNotification() {
return this._userListNotification;
}

getInviteError() {
return this._inviteError;
getUserListNotificationError() {
return this._userListNotificationError;
}

get isSaving() {
Expand Down
4 changes: 2 additions & 2 deletions static_src/test/unit/actions/user_actions.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ describe('userActions', function() {
});
});

describe('userInviteError()', function() {
describe('userListNoticeError()', function() {
let err;
let message;

Expand All @@ -267,7 +267,7 @@ describe('userActions', function() {
message = 'something happened when invititing';
sandbox.stub(AppDispatcher, 'handleServerAction');

userActions.userInviteError(err, message).then(done, done.fail);
userActions.userListNoticeError(err, message).then(done, done.fail);
});

it('should dispatch server action of type user error with error and optional message',
Expand Down
44 changes: 22 additions & 22 deletions static_src/test/unit/stores/user_store.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -737,16 +737,16 @@ describe('UserStore', function () {

beforeEach(function() {
notice = { noticeType: "finish", description: "An email invite was sent to undefined. Their account has been associated to this space, and their space roles can be controlled below." };
UserStore._inviteNotification = notice;
UserStore._userListNotification = notice;
sandbox.spy(UserStore, 'emitChange');

userActions.createInviteNotification();
});

it('should create notification for user invite', function() {
expect(UserStore.getInviteNotification()).toBeDefined();
expect(UserStore.getInviteNotification().description).toEqual(notice.description);
expect(UserStore.getInviteNotification().noticeType).toEqual(notice.noticeType);
expect(UserStore.getUserListNotification()).toBeDefined();
expect(UserStore.getUserListNotification().description).toEqual(notice.description);
expect(UserStore.getUserListNotification().noticeType).toEqual(notice.noticeType);
});

it('should emit a change event', function() {
Expand All @@ -759,16 +759,16 @@ describe('UserStore', function () {

beforeEach(function() {
notice = { noticeType: "finish", description: "message" };
UserStore._inviteNotification = notice;
UserStore._userListNotification = notice;
sandbox.spy(UserStore, 'emitChange');

userActions.clearInviteNotifications();
});

it('should clear notification for user invite', function() {
expect(UserStore.getInviteNotification()).toBeDefined();
expect(UserStore.getInviteNotification().description).not.toEqual(notice.description);
expect(UserStore.getInviteNotification().noticeType).not.toEqual(notice.noticeType);
expect(UserStore.getUserListNotification()).toBeDefined();
expect(UserStore.getUserListNotification().description).not.toEqual(notice.description);
expect(UserStore.getUserListNotification().noticeType).not.toEqual(notice.noticeType);
});

it('should emit a change event', function() {
Expand All @@ -781,27 +781,27 @@ describe('UserStore', function () {
let message;

beforeEach(function() {
UserStore._inviteError = null;
UserStore._userListNotificationError = null;
message = 'Inviting user did not work';
error = { status: 500, message: 'CF said this' };
sandbox.spy(UserStore, 'emitChange');

userActions.userInviteError(error, message);
userActions.userListNoticeError(error, message);
});

it('should add a error message to the user invite error field', function() {
expect(UserStore.getInviteError()).toBeDefined();
expect(UserStore.getInviteError().contextualMessage).toEqual(message);
expect(UserStore.getInviteError().message).toEqual(error.message);
expect(UserStore.getUserListNotificationError()).toBeDefined();
expect(UserStore.getUserListNotificationError().contextualMessage).toEqual(message);
expect(UserStore.getUserListNotificationError().message).toEqual(error.message);
});

it('should emit a change event', function() {
expect(UserStore.emitChange).toHaveBeenCalledOnce();
});
});

describe('getInviteNotification()', function () {
describe('user with _inviteNotification', function () {
describe('getUserListNotification()', function () {
describe('user with _userListNotification', function () {
let user, space, org, actual, notice;

beforeEach(function () {
Expand All @@ -819,9 +819,9 @@ describe('UserStore', function () {
UserStore.push(user);
});

it('returns notice when _inviteNotification has content', function () {
UserStore._inviteNotification = notice;
actual = UserStore.getInviteNotification()
it('returns notice when _userListNotification has content', function () {
UserStore._userListNotification = notice;
actual = UserStore.getUserListNotification()
expect(actual).toBe(notice);
});

Expand All @@ -831,10 +831,10 @@ describe('UserStore', function () {
describe('on CLEAR', () => {
beforeEach(function() {
const notice = { noticeType: "finish", description: "message" };
UserStore._inviteNotification = notice;
UserStore._userListNotification = notice;
UserStore._error = 'something';
UserStore._saving = true;
UserStore._inviteError = 'invite error';
UserStore._userListNotificationError = 'invite error';
UserStore._loading = {currentUser: true};
sandbox.spy(UserStore, 'emitChange');
AppDispatcher.handleViewAction({
Expand All @@ -843,10 +843,10 @@ describe('UserStore', function () {

});
it('resets all of the notifications and errors', () => {
expect(UserStore._inviteNotification).toEqual({});
expect(UserStore._userListNotification).toEqual({});
expect(UserStore._error).toBe(null);
expect(UserStore._saving).toEqual(false);
expect(UserStore._inviteError).toBe(null);
expect(UserStore._userListNotificationError).toBe(null);
expect(UserStore._loading).toEqual({});
});
});
Expand Down
14 changes: 13 additions & 1 deletion static_src/util/cf_api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,19 @@ export default {
deleteOrgUserPermissions(userGuid, orgGuid, apiKey) {
return http.delete(`${APIV}/organizations/${orgGuid}/${apiKey}/${userGuid}`)
.then((res) => res.response
);
).catch(res => {
const err = parseError(res);
if (res && res.response && res.response.status === 400) {
if (res.response.data.error_code === 'CF-AssociationNotEmpty') {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you combine these two conditionals into one long one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

const description = 'This user can\'t be removed because they still have a Space ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

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

also, spell out organization.
s/Org/organization

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

Choose a reason for hiding this comment

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

you don't need this.

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

Copy link
Contributor Author

@rememberlenny rememberlenny Jul 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

}
}
return Promise.reject(err);
});
},

putOrgUserPermissions(userGuid, orgGuid, permissions) {
Expand Down