Sync will invite, uninvited, remove and ignore users #47
Conversation
It doesn't need to throw. Don't do it.
Previously, we were only looking at the users list, and inviting people who were not on it. This causes trouble when run twice. If we had invited someone, but they have not yet accepted their invitation, the code would attempt to invite them again. That is not allowed by the API. It would return an error and the program would end. Now, we look at not only who is in the users list, but also who has been invited. This has made the 'diffing' code a lot more complex, but it allows us to compare across two lists. For everyone in the input list, we check if they are in either the existing users and invited users lists. If they are there, we can ignore that entry. If they are not, we can invite them. For everyone in the existing users list that we want to remove, there are two things we might have to do: remove them from the users list, or remove them from the invited users list. Since the users and invited users are separate API calls there are two separate operations for deleting users or uninviting them.
|
LGTM! Just realize that we can also have another PR for a reinviting user feature: if an invitation get expired, or that user missed the invitation email, we can
It's might be in a different PR I think |
|
Yeah! I think the |
| print("+\(user.username)") | ||
| case .remove(_, let user, _): | ||
| print("-\(user.username)") | ||
| case .ignore(let username): |
There was a problem hiding this comment.
There is probably a better way of formatting this input but I can't think of anything right now.
There was a problem hiding this comment.
This is the output from the sync command. It's called input here in conformance with the Renderer protocol.
You're right that there should be a better a way of showing it, but I didn't get up to that. Originally, I had this rendering with + for users added, and - for users removed. I did that because I wanted it to look like a diff. That didn't stand up when I wanted seperate output for removing users and rescinding invitations.
Sources/AppStoreConnectCLI/Commands/Users/SyncUsersCommand.swift
Outdated
Show resolved
Hide resolved
| return client | ||
| .request(APIEndpoint.invite(user: user)) | ||
| .map { _ in change } | ||
| .map { _ in operation } |
There was a problem hiding this comment.
🧐 I don't like this very much. Seems real side-effecty. Ie, the only thing we actually want here is the side effect and then we're mapping it to the input.
There was a problem hiding this comment.
You're right! These effects should be done in the receiveValue function right at the end of the whole thing. That would feel better, in my opinion.
|
|
||
| private func invitationsInAppStoreConnect(_ client: HTTPClient) -> AnyPublisher<[UserInvitation], Error> { | ||
| client | ||
| .request(.invitedUsers()) |
There was a problem hiding this comment.
Will this include expired invitations? Users with expired invitations should be re-invited.
There was a problem hiding this comment.
I didn't test that! As far as I am aware it will not include them in this list, and will therefore re-invite them. This will need to be explored, though
There was a problem hiding this comment.
I remember it is needed to cancel an invitation before reinvite, even it's an expired invitation. otherwise, it will show an error
|
Sorry @huwr, gonna close this and look at it again in future. |
|
Haha that's okay! A stub is implemented, but has a few problems. I think there would be a different way to implement it now, too, with all the changes made to the codebase since this PR was opened.
… On 1 May 2020, at 11:32, Oliver Jones ***@***.***> wrote:
Sorry @huwr, gonna close this and look at it again in future.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Previously, we were only looking at the users list, and inviting people who were not on it. This causes trouble when run twice. If we had invited someone, but they have not yet accepted their invitation, the code would attempt to invite them again. That is not allowed by the API. It would return an error and the program would end.
The reason the code would attempt to invite them twice is because it only checked the Users API endpoint. It also needed to check the Pending Invitations to see who has been invited.
Now, we look at not only who is in the users list, but also who has been invited. This means you can run the program many times with the same list, and it will have the same effect. A user who has already invited won't be invited a second time.
For everyone in the input list, we check if they are in either the existing users or the invited users lists. If they are, we can ignore that entry. If they are not, we invite them.
For everyone in the existing users list that we want to remove, there are two things we might have to do: remove them from the users list, or remove them from the invited users list. Since the users and invited users are separate API calls there are two separate operations for deleting users or uninviting them.
This does everything in #5 except for modification. If a user in the input file has a role that they do not have in the API, we should be adding that role.
📝 Summary of Changes
Changes proposed in this pull request:
Please don't do this on the IBA users list. 👍
🧐🗒 Reviewer Notes
💁 Example
Say this is our current user list:
We wish to remove
Edith, who never accepted the invitation, andFran, who no longer works here.Start by grabbing all my existing users. We'll use this as our input file:
I'll add
Alice,BobandDavetousers.json, and removeEdithandFran.If I run that a second, third, or fourth time it becomes:
All the entries are now
ignored. They already exist as users or have pending invitations.🔨 How To Test
Please do not do this on IBA's users list. I recommend doing it on your own App Store Connect account. Remember uninviting and removing are separate operations.