Add functions to add users/admins to a group

[haoliangyu]

The group addUsers operation is not yet covered in this library and it is an important feature to add.

The ArcGIS REST API has one single endpoint to add both users and admins. But for simplicity, we can add two functions addGroupAdmins(groupId, admins) and addGroupUsers(groupId, users), which take an array of usernames as the second parameter.

A limitation of the original endpoint is that it only accepts up to 25 usernames per request and throws an error if the developer passes a long array. In practice, the developer needs to send multiple requests if there are many group members to add. The logic is like this:

// pass a groupId and a long array of usernames
function addManyGroupUsers (groupId, usernames) {
  // break the long array into smaller ones, each has up to 25 elements
  const chunks = _.chunk(usernames, 25);

  // send multiple requests at the same time (or maybe sequentially)
  const promises = chunks.map((users) => sendAddUserRequest(groupId, users));

  return Promise.all(promises);
}

This batch request logic can be included the new functions and the user can forget the endpoint limitation.

The problem of batching requests is from the request error. Fortunately, the addUsers operation is idempotent and the developer can retry many times until succeed, without worrying about duplicate members. So I think it can simply fail the whole function if one request fails.

Otherwise, the request error can be caught as

const promises = chunks.map(
  (users) => sendAddUserRequest(groupId, users)
    .catch(err => {
      return {
        // these usernames are not added because of the request failure
        notAdded: chunks,
        // it needs a flag to indicate whether the "notAdded" is from a request
        // failure or a user issue (already added, group member limit reached,
        // etc)
        requestFailure: true
      }
    })
);

The original issue is posted at Esri/ember-arcgis-portal-services#157

@tomwayson for review

[tomwayson]

This would be a most welcome addition to the library!

Re: addGroupUsers() & addGroupAdmins() I'd be tempted to mimic the REST API's parameters (i.e. one fn that takes optional users, and/or admins), but I have vague recollections of 🐲s when working w/ this endpoint. I mean, there's probably a good reason we made 2 fn's in the groups-service, and that was before trying to tackle any batching. So I could go either way.

Regarding the batching, I wish we could use Promise.allSettled(), but alas it's stage 3, so I like your idea of using .map(), but looking at the possible responses from the end point, I think the most helpful thing for the user would be to aggregate all the successful and error response into a single hash like:

{
  notAdded: ['userFromChunk2', 'userFromChunk3', 'anotherUserFromChunk3'], // concat the notAdded from each successful chunk
  // we should probably not include this array of errors if there weren't any
  errors: [
    {
      // let the consuming app know what chunk we were processing
      // I wonder if it would be better to call this something other than notAdded?
      notAdded: [...], // this is all the users in the chunk
      // everything below is passed on directly from the server response
      "code": 400,
      "messageCode": "SOME_CODE",
      "message": "Some message from the server",
      "details": []
    }
  ]
}

To build that, I think you would just need to .reduce() over the Promise.all() response and concatenate the notAdded arrays from the successful responses, and build an array of errors out of the error responses.

[haoliangyu]

@tomwayson All great points!

I'd be tempted to mimic the REST API's parameters (i.e. one fn that takes optional users, and/or admins)

I agree. We can send separate requests for admins and users, but the function parameters should be the same as the REST API.

I think the most helpful thing for the user would be to aggregate all the successful and error response into a single hash

👍

I wonder if it would be better to call this something other than notAdded?

or attach the request body and URL?

[tomwayson]

or attach the request body and URL?

Good idea!

[tomwayson]

Actually, I think the err's you'll catch will be instances of ArcGISRequestError, which I includes the original request options. If so, I think it would be sufficient to return an array of those, so something like:

interface IAddGroupUsersResponse {
  notAdded?: string[],
  // we should not include this array of errors if there weren't any
  errors?: ArcGISRequestError[]
}

[haoliangyu]

👍 thanks @tomwayson. I think I have enough information to start with.