Devices UX: Moving devices between applications

[joepavitt]

As part of Bulk Device Actions, users should be able to move multiple (even a single) Device easily from an Application to (a) another Application and (b) an Instance.

Tasks

Beta Give feedback

1. Support for bulk move devices (API) #4335
   +0
   
2. UI for Bulk Move Devices #4337
   +0
   
3. Update documentation for Bulk Move Operations #4345
   +0
   
4. Changelog for new feature - bulk move devices website#2453
   +0
   

[Steve-Mcl]

Initial thoughts around how to surface this in the UI:

Replace the new Trashcan bulk "delete" with an "Actions" drop-down that includes "Delete" and "Move..." options.
The actions button should behave similarly in that it should be disabled when no devices are selected.

[Steve-Mcl]

Functional design

Now I have spent some time looking though the application and code, more thoughts occur.

Below is a slightly disjointed brain dump that is intended to be thought provoking to get to the most intuitive behaviour of moving device(s). Please feel free to comment/correct/dismiss any or all of the below (feedback is wanted and required please)

Scope

• moving devices to another team is not in scope

Handling movement of mixed application/instance source devices?

• i.e. it is not unreasonable for an automation script to request to move a list of devices
  that may or may not belong to different applications/instances
• The alternative is to require the user to move "like" devices in separate requests
  which makes me lean towards the following:
  • Since this first iteration is a UI focused bulk operation, we should only support operations
    on devices that are "like" (i.e. moving devices from a single application or a single instance)
  • If the user wants to move devices from different applications/instances, they MUST do so in separate UI ops anyway
  • Therefore, any API ops that attempt to do a move of devices from APP-A-DEVICE + APP-B-DEVICE + INSTANCE-C-DEVICE to APP-D will raise an exception and fail (400)
  • This will simplify the implementation in this first iteration, and we can revisit if/when we decide to support a more flexible implementation
  • UPDATE: What I missed was the team devices page has bulk selection checkboxes. To be consistent we should support multiple source types (i.e. user has 3 devices, one in APP-A, one in INSTANCE-B and one is unassigned. It is perfectly logical the user will want to move these three devices into NEW-APP or NEW-INSTANCE)
    • Implementation doesnt really change that much, in fact somewhat simplifies things. The only real verification is that all devices belong to the same team and the destination is in the same team.
    • Please comment if I have missed something.
• RBACs to consider:
  • device:edit (or device:delete/device:create?)
    • device edit feels more like "settings" than "move" but it is a possible candidate
  • device:move (new permission, owner only)
  • My current position on this:
    • The permission(s) should have the "Owner" role (device create and delete are already owner roles)
    • To minimise the combination of checks on various RBACS, we should have a single permission
      that allows the user to move devices between applications/instances
      • That choice IMO would be a new permission "device:move" (owner only) as it differs
        from "edit" that i associate with settings changes
      • This will simplify the implementation in this first iteration, and we can revisit if/when needed

What if one of the devices is an application device-group member?

• should we remove it from the group or inhibit the operation?
• should we ask the user if devices should remain members of the current group they belong to or
  simply inform them that the devices will be removed from the group?
• RBACS to consider:
  • application:device-group:membership:update?
  • device:move (new permission - owner only?)
  • combination of the above?
• My Choice: permit the operation
  • remove any device from its group if it is being moved to an instance
  • leave the device in its existing group if it is being moved to same application
    • I realise from a UI perspective, this may not make sense but operationally, at the API level
      it may be the API selects 10x devices to move to "Application B" and one of those devices
      may already be a member of a group in "Application B"
    • any device moving to an instance or different application will be removed from its current group
• Ensure the confirmation dialog in the UI is clear about the behaviour

What if the target instance is protected?

• I suspect this is not an issue, but raised as a topic for discussion/consideration

What if the target instance is HA?

• I suspect this is not an issue, but raised as a topic for discussion/consideration

What do we do with devices having a target snapshot set

• likely not an issue but behaviour vs expectations needs to be understood and if necessary, documented

What do we do in terms of updating the devices upon move?

• devices in fleet mode will get reset for updated env vars (FF_APPLICATION_ID etc)
• devices in dev mode should remain "as is" until the user takes action
• Ensure the confirmation dialog in the UI is clear about this behaviour

Should we permit simple bulk move to "unassigned"?

• As a user I may wish to either decomission a bunch of devices or temporarily remove them from an application while i set them up
• UI Wise, the user would select "None" (or unassigned)

---

Once feedback is collated I will summarise it all for easier digestion to be served as an implementation guide.

[Steve-Mcl]

API design

There are two approaches I am considering.

1. Dedicated endpoint: POST /api/v1/teams/:teamId/devices/bulk/move
   1. This feels clean and is a single responsibility but it does become Yet Another Route.
2. Typical CRUD update endpoint PUT /api/v1/teams/:teamId/devices/bulk
   1. This could be overloaded for other bulk updates however while we are technically updating the devices this feels like an too much of an overload for a typical UPDATE route.

As it stands, I am strongly favouring POST /api/v1/teams/:teamId/devices/bulk/move. It feels like a natural extension of the existing DELETE /api/v1/teams/:teamId/devices/bulk route, clearly indicates a bulk op and clearly specifies the nature of the operation (moving devices) with the "move" suffix.

Also, POST feels (to me) like a better fit than say PUT or PATCH since POST is typically used for actions that (on top of creating resources) invlolve change the state of a resource. Moving devices from one app to another will involve altering their state (device groups cleared/target snapshot change etc). That said, PATCH would probably also be suitable without the /move but far less descriptive or have the same clear intent.

Lastly, in my previous comment, I reasoned that a new role "device:move" made sense. That also give weight to POST /bulk/move employing its own RBAC.

the route should proabably also be idempotent.

[joepavitt]

bulk/move is not very restful though...

If we have the dedicated bulk already, then it should be a PUT to bulk

If we don't have bulk endpoint already, we shouldn't add one as that's not RESTful standard.

PUT/DELETE to /devices is how it should be done

[Steve-Mcl]

The URI /api/v1/teams/:teamId/devices/bulk/move identifies the resource (devices within a team) and specifies the action (bulk move). That is RESTful to me. It is also super clear in intent.

Regarding PUT. PUT method is yypically used to replace an entire representation of a resource. This doesn't align with the "bulk move" operation. POST is for creating a new resource or perform an action that changes the state of resources.

Based on those 2 observations I strongly prefer POST /api/v1/teams/:teamId/devices/bulk/move

Endpoints are cheap & clarity feels more important here ;).

Happy to be overruled as alsways (this is design phase)

[joepavitt]

Regarding PUT. PUT method is yypically used to replace an entire representation of a resource. This doesn't align with the "bulk move" operation

PATCH is perhaps more appropriate then.

RESTful APIs shouldn't have verbs in them, the verb is always the POST/GET/PATCH, etc

[Steve-Mcl]

Regarding PUT. PUT method is yypically used to replace an entire representation of a resource. This doesn't align with the "bulk move" operation

PATCH is perhaps more appropriate then.

Fully open to that:

That said, PATCH would probably also be suitable without the /move but far less descriptive or have the same clear intent.

Lets see what/if any other more opinions come in before locking it in. (we can easily tweak in review)

[joepavitt]

No Verbs: https://stackoverflow.blog/2020/03/02/best-practices-for-rest-api-design/#h2-2eb87cd076da0

[Steve-Mcl]

We can disagree and throw links around all day but sometimes the "recommendation" doesnt fit

Sometimes you really have no way to map the action to a sensible RESTful structure. For example, a multi-resource search doesn't really make sense to be applied to a specific resource's endpoint. In this case, /search would make the most sense even though it isn't a noun. This is OK - just do what's right from the perspective of the API consumer and make sure it's documented clearly to avoid confusion.

But, for the sake of argument, I will use PATCH /api/v1/teams/:teamId/devices/bulk

[joepavitt]

That's fine but why do we have a /bulk endpoint? That doesn't make sense given that /devices by nature of its plurality is bulk?

Singular operations should be done at devices/:deviceId, which then causes a clash with /devices/bulk

[Steve-Mcl]

That's fine but why do we have a /bulk endpoint?

Twas debated in the original PR Joe. There was a reason that alludes me right now (something like other bulk ops that would overlap and eventually cause either overloading of an endpoint or mangling of a different URI to suit)

Anyhow, not to 100% renage on the previous comment but I think (to avoid future mangling to overloading) there may be a compromise - use of POST + param

POST /api/v1/teams/:teamId/devices/bulk/:action

Is that a suitable / sensible compromise? This permits future bulk ops that suite the same URI withut mangling or overloading.

Anyhow, off out for beers with friends. Will catch up with feedback on monday

PS, thanks a lot for feeding back out of hours Joe (I wanna to get a head start on this on Mon Morning)

[joepavitt]

Is that a suitable / sensible compromise? This permits future bulk ops that suite the same URI withut mangling or overloading.

Not really because we're duplicating effort/code.

We only need a PATCH endpoint for /devices.

The FE, or whatever else calls that API, provides the relevant data there.

If we have an endpoint for every action type, that's unnecessary code that we don't need to write and wasted time everytime we want to add a new action

[Steve-Mcl]

If we have an endpoint for every action type, that's unnecessary code that we don't need to write and wasted time everytime we want to add a new action

Not sure what you mean Joe. The :param suffix in the suggestion would have made this a single backend endpoint 🤷

Anyhow, lets move forward with your suggestion of the PATCH verb and a static endpoint (but note: the path will have to have the /bulk suffix (see below))

Singular operations should be done at devices/:deviceId, which then causes a clash with /devices/bulk

Ahha, I now see the confusion. Let me put you mind at ease - there is no clash. Those single resource operations are on a different API route. They are in forge/routes/api/device.js and are served at /api/v1/devices/:id whereas the bulk operations are in forge/routes/api/teamDevices.js and served from /api/v1/team/:teamId/devices which already had a sub-route /provisioning (and others). That is why the suffix /bulk was used. It was briefly debated at the time (just couldnt remember the reasoning).

[Steve-Mcl]

• Tested on prod 👍
• change log present on website 👍
• docs present on website 👍