flowfuse icon indicating copy to clipboard operation
flowfuse copied to clipboard

Bulk update team devices API (bulk move support only)

Open Steve-Mcl opened this issue 1 year ago • 2 comments

closes #4335

Description

Adds backend API route for team devices bulk update:

  • test/unit/forge/routes/api/teamDevices_spec.js
  • Method: PUT
  • URI: /api/v1/teams/:teamId/devices/bulk

The route was debated in design phase in the issue but after much debate, I have stuck with the same verb and definition already established in the existing devices API (that are responsible for single device combined update / device move endpoint). More than happy to change this around if desired.

Tests Added

  Team Devices API
    Team Devices
      Supports bulk operations
        update
          move devices
            ✔ Rejects invalid application (404)
            ✔ Rejects invalid instance (404)
            ✔ Rejects moving a device to another team application (400)
            ✔ Rejects moving a device to another team instance (400)
            ✔ Rejects invalid input
            ✔ Member cannot move devices (403)
            ✔ Non team member cannot move devices (404)
            ✔ Move all to Application 2 (48ms)
            ✔ Move all to Project 2 (50ms)
            ✔ Only updates devices not already assigned to the target application (46ms)
            ✔ Only updates devices not already assigned to the target project (49ms)
            ✔ Only unassigns devices not already unassigned

Related Issue(s)

Owner #4335 Parent #4290

Checklist

  • [x] I have read the contribution guidelines
  • [x] Suitable unit/system level tests have been added and they pass
  • [ ] Documentation has been updated
    • [ ] Upgrade instructions
    • [ ] Configuration details
    • [ ] Concepts
  • [ ] Changes flowforge.yml?
    • [ ] Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • [ ] Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • [ ] Includes a DB migration? -> add the area:migration label

Steve-Mcl avatar Aug 06 '24 19:08 Steve-Mcl

Codecov Report

Attention: Patch coverage is 84.25197% with 20 lines in your changes missing coverage. Please review.

Project coverage is 78.33%. Comparing base (bb8f0a3) to head (455a8b3).

Files Patch % Lines
forge/routes/api/teamDevices.js 64.10% 14 Missing :warning:
forge/db/controllers/Device.js 93.18% 6 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4336      +/-   ##
==========================================
+ Coverage   78.25%   78.33%   +0.08%     
==========================================
  Files         292      292              
  Lines       13439    13558     +119     
  Branches     3014     3050      +36     
==========================================
+ Hits        10517    10621     +104     
- Misses       2922     2937      +15     
Flag Coverage Δ
backend 78.33% <84.25%> (+0.08%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Aug 06 '24 19:08 codecov[bot]

NOTE: I mentioned in Eng Meeting that there was a sensible refactoring - that is the controller method moveDevices should replace the code in https://github.com/FlowFuse/flowfuse/blob/f58bc20fb1ffa838dbf24b57240bb91f04eb5955/forge/routes/api/device.js#L435-L518

There reason I was in two minds was related to one of our guys raising a concern about devices being reset when moved (even though they were in developer mode) and whether I would include or exclude that "feature".

As it happens, the implementation of the bulk update is a refactoring of this original route (with obvious modifications for multiple device support) INCLUDING the switching of a device to "autonomous" (fleet) mode when unassigned.

As it happens, with the introduction of bulk move feature, the user can side step the unassignment when moving devices from application to application and thus the switching out of developer mode so it kinda becomes less of an issue


In short, we can and should refactor the single version in the API Route to use the new controller method.

Will raise an issue as a follow up.

Steve-Mcl avatar Aug 07 '24 13:08 Steve-Mcl

@Steve-Mcl approved - I appreciate there are a few other PRs behind this. Will let you merge in the appropriate order.

knolleary avatar Aug 12 '24 13:08 knolleary

  • All branches merged into this.
  • Tested operations on pre-staging
  • Once all green - good to go.

Steve-Mcl avatar Aug 12 '24 16:08 Steve-Mcl