feat: add bulk delete all measurements for a device - #858
Conversation
Previously, clearing a device's measurement history required deleting each sensor's data individually. Users can now delete all measurements across all sensors at once from the device settings sidebar, with password confirmation and feedback for both success and empty states.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
scheidtdav
left a comment
scheidtdav
left a comment
There was a problem hiding this comment.
I think we should revisit the UX of this:
Currently on osem, the users are able to select "delete measurements" per sensor on the "sensors" page of the device settings.
I think we should keep that and add "delete all measurements" to that.
The confirmation by entering the password again is a good idea. We should totally keep that.
@JerryVincent There is no pressure for you to pick this up again. I am just leaving this here for our future reference :-) Of course, if you have fun doing it, feel free to do so.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds password-confirmed deletion of all measurements linked to a device’s sensors. The edit sensors page now provides the form and status feedback. English and German translations support the flow, and the separate deletion navigation entry is removed. ChangesDevice measurement deletion
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to Bulk deletion checks ownership separately from the destructive operation, so a concurrent ownership transfer could allow measurements to be deleted after authorization becomes stale. This concrete data-security risk should be fixed or explicitly accepted before merging. Sequence Diagram(s)sequenceDiagram
participant User
participant SensorsPage
participant SensorsAction
participant MeasurementModel
participant Database
User->>SensorsPage: Submit password
SensorsPage->>SensorsAction: POST delete-measurements
SensorsAction->>SensorsAction: Verify password
SensorsAction->>MeasurementModel: Delete device measurements
MeasurementModel->>Database: Select sensors and delete measurements
Database-->>MeasurementModel: Return deleted row count
MeasurementModel-->>SensorsAction: Return deletion result
SensorsAction-->>SensorsPage: Return success or error data
SensorsPage-->>User: Display status message
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 4 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e8e5b315-bc29-421b-8642-1e1e942277bd
📒 Files selected for processing (5)
app/db/models/measurement.server.tsapp/routes/device.$deviceId.edit.delete-measurements.tsxapp/routes/device.$deviceId.edit.tsxpublic/locales/de/delete-measurements.jsonpublic/locales/en/delete-measurements.json
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
| ) | ||
| } | ||
|
|
||
| const result = await deleteMeasurementsForDevice(deviceId) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '1,115p' 'app/routes/device.$deviceId.edit.delete-measurements.tsx'
sed -n '325,360p' app/db/models/measurement.server.ts
rg -n "update.*device|userId.*device|deviceId.*userId|set\\(.*userId|getUserDevice|deleteMeasurementsForDevice" app/db app/routesRepository: openSenseMap/frontend
Length of output: 9733
Authorization Bypass (CWE-367): Time-of-check Time-of-use (TOCTOU) Race Condition
Reachability: External · Exploitability: Difficult
Make the ownership check atomic with deletion.
getUserDevice checks ownership before the separate, unscoped deletion. Pass userId to deleteMeasurementsForDevice and include it in the deletion predicate. Add a race test that transfers ownership between these operations and confirms that no measurements are deleted.
📍 Affects 2 files
app/routes/device.$deviceId.edit.delete-measurements.tsx#L70-L70(this comment)app/db/models/measurement.server.ts#L339-L347
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b9a43aa6-25dd-4a82-b171-6f3a221e9051
📒 Files selected for processing (4)
app/routes/device.$deviceId.edit.sensors.tsxapp/routes/device.$deviceId.edit.tsxpublic/locales/de/edit-device-sensors.jsonpublic/locales/en/edit-device-sensors.json
💤 Files with no reviewable changes (1)
- app/routes/device.$deviceId.edit.tsx
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
2 participants
Type of Change
Implementation
Added a new Delete Measurements page under the device settings sidebar (/device/:deviceId/edit/delete-measurements). On the backend, deleteMeasurementsForDevice was added to measurement.server.ts — it uses a Drizzle subquery to fetch all sensor IDs belonging to the device and deletes their measurements in a single database operation. The route action follows the same auth pattern as the device deletion page: session-based ownership check plus password re-verification via verifyLogin before any data is touched. The action returns the deleted row count so the UI can distinguish between a successful deletion and a no-op (device had no measurements), showing a green success banner or a yellow warning respectively. EN and DE translations are included.
Checklist
devbranchAdditional Information