Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bulk update team devices API (bulk move support only) #4336

Merged
merged 33 commits into from
Aug 12, 2024

Conversation

Steve-Mcl
Copy link
Contributor

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

  • I have read the contribution guidelines
  • 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

Copy link

codecov bot commented Aug 6, 2024

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 ⚠️
forge/db/controllers/Device.js 93.18% 6 Missing ⚠️
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%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Steve-Mcl
Copy link
Contributor Author

NOTE: I mentioned in Eng Meeting that there was a sensible refactoring - that is the controller method moveDevices should replace the code in

const assignTo = request.body.instance ? 'instance' : (request.body.application ? 'application' : null)
if (!assignTo) {
// ### Remove device from application/project ###
const unassignApplication = request.body.application === null
const unassignInstance = request.body.instance === null
const commonUpdates = async () => {
// Clear its target snapshot, so the next time it calls home
// it will stop the current snapshot
await device.setTargetSnapshot(null)
// Since the project/application is being cleared, clear the deviceGroup
device.DeviceGroupId = null
sendDeviceUpdate = true
// disable developer mode
device.mode = 'autonomous'
await device.save()
}
if (unassignApplication && device.Application) {
const oldApplication = device.Application
// unassign from application
await device.setApplication(null)
await commonUpdates()
await app.auditLog.Team.team.device.unassigned(request.session.User, null, device?.Team, oldApplication, device)
await app.auditLog.Application.application.device.unassigned(request.session.User, null, oldApplication, device)
}
if (unassignInstance && device.Project) {
const oldProject = device.Project
// unassign from project
await device.setProject(null)
await commonUpdates()
// disable developer mode
device.mode = 'autonomous'
await device.save()
await app.auditLog.Team.team.device.unassigned(request.session.User, null, device?.Team, oldProject, device)
await app.auditLog.Project.project.device.unassigned(request.session.User, null, oldProject, device)
}
} else if (assignTo === 'instance') {
// ### Add device to instance ###
// Update includes a instance id?
if (device.Project?.id === request.body.instance) {
// Device is already assigned to this instance - nothing to do
} else {
// Check if the specified project is in the same team
assignToProject = await app.db.models.Project.byId(request.body.instance)
if (!assignToProject) {
reply.code(400).send({ code: 'invalid_instance', error: 'invalid instance' })
return
}
if (assignToProject.Team.id !== device.Team.id) {
reply.code(400).send({ code: 'invalid_instance', error: 'invalid instance' })
return
}
// Project exists and is in the right team - assign it to the project
sendDeviceUpdate = await assignDeviceToProject(device, assignToProject)
postOpAuditLogAction = 'assigned-to-project'
}
} else if (assignTo === 'application') {
// ### Add device to application ###
// Update includes a application id?
if (device.Application?.id === request.body.application) {
// Device is already assigned to this application - nothing to do
} else {
// Check if the specified application is in the same team
assignToApplication = await app.db.models.Application.byId(request.body.application)
if (!assignToApplication) {
reply.code(400).send({ code: 'invalid_application', error: 'invalid application' })
return
}
if (assignToApplication.Team.id !== device.Team.id) {
reply.code(400).send({ code: 'invalid_application', error: 'invalid application' })
return
}
// Device exists and is in the right team - assign it to the application
sendDeviceUpdate = await assignDeviceToApplication(device, assignToApplication)
postOpAuditLogAction = 'assigned-to-application'
}
}

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 Steve-Mcl mentioned this pull request Aug 8, 2024
10 tasks
Copy link
Member

@knolleary knolleary left a comment

Choose a reason for hiding this comment

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

One issue spotted.

forge/db/controllers/Device.js Outdated Show resolved Hide resolved
@knolleary
Copy link
Member

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

@Steve-Mcl
Copy link
Contributor Author

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

@Steve-Mcl Steve-Mcl merged commit acaaaff into main Aug 12, 2024
14 checks passed
@Steve-Mcl Steve-Mcl deleted the 4290-bulk-move-devices branch August 12, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for bulk move devices (API)
3 participants