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 device delete api #4209

Merged
merged 6 commits into from
Jul 24, 2024
Merged

Bulk device delete api #4209

merged 6 commits into from
Jul 24, 2024

Conversation

Steve-Mcl
Copy link
Contributor

@Steve-Mcl Steve-Mcl commented Jul 18, 2024

closes #4208

Description

Adds bulk operation for deleting multiple devices in a team.

Basic premise:

  1. Operates on a team level API
    • I initially wrote this as a top level API api/v1/devices/bulk but there were difficulties with needsPermission and team membership validation (would have to be handled manually). It made sense at this time to have this as a team level API as it is to be initially implemented in the UI at the device browser component (happy to move to top level if deemed better/necessary)
  2. All devices specified in the devices body property must exist otherwise operation throws
  3. All devices specified in the devices body property must must be in the same team otherwise operation throws
  4. Calls device destroy on where id in ids as a singular operation
  5. Calls billing.updateTeamDeviceCount is licenced
  6. logs bulk delete count
    • only count of deletion at this time

Endpoint

  • Adds endpoint DELETE /bulk to forge/routes/api/teamDevices.js
    • This equates to /api/v1/team/:teamId/devices/bulk
      • This is open to debate. I figured since we might one day have bulk update, bulk create etc - it feels clear and separate. Though in writing this up, it might be enough to simply leave the sub path at / e.g. DELETE /api/v1/team/:teamId/devices
    • Device IDs are expected in the body as an array of hash-ids in the property devices
  • Adds permission "team:device:bulk-delete"
  • Adds team level audit log event handler

Tests

  • Adds audit logger test
  Audit Log > Team
    ✔ Provides a logger for bulk deleting team devices
  • Adds route tests:
 Team Devices API
    Team Devices
      Supports bulk operations
        delete
          ✔ Non owner cannot delete devices
          ✔ Delete a single device
          ✔ Delete multiple devices
          ✔ Cannot delete device belonging to a different team
          ✔ Prevents deleting any devices mixed in with other team devices

Related Issue(s)

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 Jul 18, 2024

Codecov Report

Attention: Patch coverage is 84.21053% with 6 lines in your changes missing coverage. Please review.

Project coverage is 78.65%. Comparing base (940a813) to head (bb87735).

Files Patch % Lines
forge/db/controllers/Device.js 85.00% 3 Missing ⚠️
forge/routes/api/teamDevices.js 80.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4209      +/-   ##
==========================================
+ Coverage   78.64%   78.65%   +0.01%     
==========================================
  Files         286      286              
  Lines       13119    13157      +38     
  Branches     2934     2948      +14     
==========================================
+ Hits        10317    10349      +32     
- Misses       2802     2808       +6     
Flag Coverage Δ
backend 78.65% <84.21%> (+0.01%) ⬆️

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 Steve-Mcl requested a review from knolleary July 18, 2024 15:51
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.

All looks good. My only pause has been the choice of api endpoint - whether having /bulk in the url is the right choice. I've spent some time researching other APIs to see how (and if) they support bulk operations.... and it's pretty inconclusive. Having a dedicated bulk endpoint (rather than overloading an existing api) does appear to be one common approach - so on the basis of that, lets roll with what's proposed here.

@Steve-Mcl Steve-Mcl enabled auto-merge July 24, 2024 09:01
@Steve-Mcl Steve-Mcl merged commit 3f397c3 into main Jul 24, 2024
10 checks passed
@Steve-Mcl Steve-Mcl deleted the 4208-bulk-device-delete-api branch July 24, 2024 09:01
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.

Bulk Device Operations API (MVP: bulk delete)
2 participants