Edit snapshot #4282

Steve-Mcl · 2024-07-30T16:10:06Z

closes #4281

Description

TLDR;
Introduces a common dialog and supporting endpoint for editing snapshots.

Implementation

All snapshot table views (instance, application, device) now have the kebab menu item "Edit Device".
When clicked, a form with Name and Description are pre-populated with the current values from the snapshot.
Validation
- Name has non empty validation only
- "Confirm" button should be disabled when name is invalid
- Description has no validation (can be blank)
Upon clicking Confirm, the API is called with the new Name/Description
- upon success the form is closed, a "success" toast is shown, and the table is updated (client side only)
- upon failure, an "error" (warning) toast is shown and the dialog remains open to permit a retry without having to re-type

Finer detail

Adds put to the main route of the top-level snapshot API
- expects data to contain name and/or description only at this time
Implements updateSnapshot in controller forge/db/controllers/Snapshot.js
Adds permission snapshot:edit that is used for both FE and BE routes
Adds FE API route updateSnapshot to frontend/src/api/snapshots.js
Adds shared dialog frontend/src/components/dialogs/SnapshotEditDialog.vue
Updates kebab in the instances, devices and applications snapshots views (see demo GIF)

Unit Tests added in `test/unit/forge/routes/api/snapshots_spec.js`

    Update snapshot
      instance
        ✔ Returns 404 for non-existent snapshot
        ✔ Non-member cannot update snapshot (72ms)
        ✔ Owner can update snapshot (52ms)
        ✔ Can update name only (51ms)
        ✔ Can update description only (53ms)
        ✔ Member cannot update snapshot (39ms)
      device
        ✔ Returns 404 for non-existent snapshot
        ✔ Non-member cannot update snapshot (44ms)
        ✔ Owner can update snapshot
        ✔ Can update name only
        ✔ Can update description only
        ✔ Member cannot update snapshot

e2e Test added in `test/e2e/frontend/cypress/tests/applications/snapshots.spec.js`

FlowForge - Application - Snapshots
  ✔ provides functionality to edit a snapshot

e2e Test added in `test/e2e/frontend/cypress/tests/instances/snapshots.spec.js`

FlowForge - Instance Snapshots
  ✔ provides functionality to edit a snapshot

e2e Test added in `test/e2e/frontend/cypress/tests-ee/devices/snapshots.spec.js`

FlowForge - Devices - With Billing
  ✔ provides functionality to edit a snapshot

Demo

TODO

Tests (unit and e2e)

Related Issue(s)

Owner #4281
Parent #3818

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

codecov · 2024-07-30T16:19:26Z

Codecov Report

Attention: Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.18%. Comparing base (14c6a53) to head (6cc940c).
Report is 41 commits behind head on main.

Files	Patch %	Lines
forge/db/controllers/Snapshot.js	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4282      +/-   ##
==========================================
+ Coverage   78.17%   78.18%   +0.01%     
==========================================
  Files         287      292       +5     
  Lines       13310    13434     +124     
  Branches     2986     3006      +20     
==========================================
+ Hits        10405    10504      +99     
- Misses       2905     2930      +25

Flag	Coverage Δ
backend	`78.18% <96.77%> (+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.

closes #4283

joepavitt · 2024-07-31T08:08:43Z

How does this impact any snapshots deployed to a device? Is there a need to push updates at that point? I'm assuming we're just running from Snapshot ID, so no need?

Steve-Mcl · 2024-07-31T08:35:25Z

How does this impact any snapshots deployed to a device? Is there a need to push updates at that point? I'm assuming we're just running from Snapshot ID, so no need?

Hmmm, I was 100% certain there was no need to update the device until you mentioned this 🤔

scurries off to double check

Steve-Mcl · 2024-07-31T08:49:57Z

How does this impact any snapshots deployed to a device? Is there a need to push updates at that point? I'm assuming we're just running from Snapshot ID, so no need?

Hmmm, I was 100% certain there was no need to update the device until you mentioned this 🤔

scurries off to double check

@joepavitt thanks for flagging this.

So, the device does get FF_SNAPSHOT_NAME and as this PR stands, the device is not informed/not restarted meaning the env var is out of step.

This is however purely informational, and does not impact operation of the device (snapshot id is still unchanged/still relevant)

The question becomes, do we need to perform a device update just because of snapshot name change (we dont currently push an update for the device name change!)? If we do, then it would be advisable to consider the UX (e.g. do we check ALL devices/instances associated with the snapshot, do we issue restart device automatically?)

It might be as well to toast info to the user (and update docs) to state something like:
"Renaming a snapshot that is in use will not cause device to automatically restart however they will need an update at some point to synchronise the FF_SNAPSHOT_NAME environment variable."

@knolleary any thoughts please?

knolleary · 2024-07-31T08:56:57Z

Agreed this should not trigger a device restart - if I'm doing housekeeping to mark a snapshot as 'good', I don't want it to redeploy and restart everything.

I would not use a toast notification either. It can be a simple one-line in the dialog (so the user knows this information before they make the change) to say something like "Changes to the snapshot will not be reflected on devices already running the snapshot".

Steve-Mcl · 2024-07-31T08:59:53Z

Note to reviewers:

the TODO for audit logging in lines 195-201 of this PR are handled in the incoming branch PR #4287 here

Steve-Mcl · 2024-07-31T09:19:25Z

would not use a toast notification either. It can be a simple one-line in the dialog (so the user knows this information before they make the change) to say something like "Changes to the snapshot will not be reflected on devices already running the snapshot".

Will do that. Thanks.

Steve-Mcl · 2024-07-31T09:25:55Z

Just hit a speed bump.

Issue:

In short, because I renamed an auto snapshot, its name no longer matches the prescribed format and we can no longer infer this is a system auto snapshot.

We currently infer a snapshot was an auto snapshot from its name and lack of userId due to lack of dedicated field in the DB (this has been mentioned in previous GH issue as being an MVP and will require a DB flag in the future). It may be that future is now OR we can "handle it"

Options (from simplest to hardest)

Leave it blank - size 0
Editor inherits the snapshot (current user info gets applied) - size 1
We update the database to have a field that records the originator or source of snapshot - size 2~3

Switching PR to draft for now until solution is determined/implemented.

hardillb · 2024-07-31T09:29:49Z

Gut feel is that if you edited, you own it...

knolleary · 2024-07-31T09:31:29Z

What other scenarios mean a snapshot has a null user property? Auto-generated snapshots is the only one I can think of - so why not use the lack of a user on the object to be the trigger to show the 'auto snapshot' label rather than parsing the name?

My only pause for thought is I cannot remember if our MVP for auto-snapshots included deleting old ones - and what logic that uses to determine what is an old auto-snapshot versus a snapshot a user wants to keep.

Steve-Mcl · 2024-07-31T09:55:48Z

What other scenarios mean a snapshot has a null user property?

Stretching my memory here but 99.9% certain there should NOT be any operations other than auto snapshot that are permitted to leave UserId null. My only pause is that I may have a distant memory of some snapshots already having no UserId (though that is very possibly due to local dev work)

Auto-generated snapshots is the only one I can think of - so why not use the lack of a user on the object to be the trigger to show the 'auto snapshot' label rather than parsing the name?

That is an option (though it does move from "infer" to "assume")

My only pause for thought is I cannot remember if our MVP for auto-snapshots included deleting old ones - and what logic that uses to determine what is an old auto-snapshot versus a snapshot a user wants to keep.

The premise was: if the name matches the specific format, there are 10 of them, delete the one with lowest id

What will happen here, after an auto snapshot is renamed, there will now effectively be only 9 Auto snapshots. The next auto snapshot will not cause a deletion (but rather make up the 10th) - so this is a non issue.

Gut feel is that if you edited, you own it...

I am slightly leaning to this also but there is merit in what Nick said about assuming null UserId as an Auto Snapshot - as keeps the status quo!

Since modifying the DB to identify snapshot source (discussed here and here) are not absolutely necessary at this point (only auto snapshots apply null UserId) we can move move forwards with the smaller iteration at this time. Therefore, could I get consensus on the 2 offerings please:

Assume any snapshot that has null UserId is an Auto Snapshot
User who changes an auto snapshot, now owns it

knolleary · 2024-07-31T09:58:32Z

Please go with my suggestion. The user field is who created it, not who owns it. If the field is blank, assume its an auto-snapshot - ie the system generated it. That is a safe assumption to make - unless the user who created has since been deleted. But that is an edge case for another day.

Steve-Mcl · 2024-07-31T10:13:29Z

I would not use a toast notification either. It can be a simple one-line in the dialog (so the user knows this information before they make the change) to say something like "Changes to the snapshot will not be reflected on devices already running the snapshot".

Addressed in 937807d

Steve-Mcl · 2024-07-31T10:16:30Z

If the field is blank, assume its an auto-snapshot - ie the system generated it.

Addressed in d46a226

joepavitt

FE all good. Server-side looks good too, but will wait for Nick's eyes before merging.

closes #4286

update docs for snapshot edit

knolleary · 2024-07-31T13:27:37Z

I'm not going to be able to review this properly until much later today.

Audit log entries for Snapshot Update

knolleary · 2024-08-01T08:37:14Z

Merging once the tests go green

Steve-Mcl added 2 commits July 30, 2024 16:51

Backend support for editing snapshots

9adf215

new shared edit snapshot dialog

45b6832

Steve-Mcl temporarily deployed to staging July 30, 2024 16:13 — with GitHub Actions Inactive

Steve-Mcl added 5 commits July 30, 2024 20:09

Add kebab menu entries to show snapshot edit dlg

bdfebd2

improve validation

69bafb9

simplify implementation

cc2f387

add unit tests for edit snapshot

cdeadca

update / add e2e tests for editing a snapshot

c8cff45

Steve-Mcl temporarily deployed to staging July 30, 2024 19:18 — with GitHub Actions Inactive

Steve-Mcl marked this pull request as ready for review July 31, 2024 07:23

Steve-Mcl requested review from knolleary and joepavitt July 31, 2024 07:24

add loggers for snapshot updated

0d67f1b

closes #4283

Steve-Mcl added 3 commits July 31, 2024 09:25

Add logging to route to capture snapshot edits

3d4580c

view snapshot updated events

44ed441

unit tests for snapshot updated audit logging

169d9b4

Steve-Mcl marked this pull request as draft July 31, 2024 09:26

Add note about snapshot updates will not auto reset device

937807d

Update snapshots.transformer.js to assume null UserId == Auto Snapshot

d46a226

Steve-Mcl temporarily deployed to staging July 31, 2024 10:14 — with GitHub Actions Inactive

Steve-Mcl marked this pull request as ready for review July 31, 2024 10:16

joepavitt mentioned this pull request Jul 31, 2024

Audit log entries for Snapshot Update #4287

Merged

10 tasks

joepavitt approved these changes Jul 31, 2024

View reviewed changes

Steve-Mcl and others added 2 commits July 31, 2024 14:22

update docs for snapshot edit

02d6655

closes #4286

Merge pull request #4294 from FlowFuse/3818-docs

4830d72

update docs for snapshot edit

knolleary temporarily deployed to staging July 31, 2024 13:30 — with GitHub Actions Inactive

knolleary approved these changes Aug 1, 2024

View reviewed changes

Merge pull request #4287 from FlowFuse/3818-edit-snapshot-audit-entries

6cc940c

Audit log entries for Snapshot Update

knolleary temporarily deployed to staging August 1, 2024 08:39 — with GitHub Actions Inactive

knolleary merged commit 881fbea into main Aug 1, 2024
14 checks passed

knolleary deleted the 3818-rename-snapshot branch August 1, 2024 08:56

knolleary temporarily deployed to staging August 1, 2024 08:56 — with GitHub Actions Inactive

joepavitt mentioned this pull request Aug 1, 2024

Changelog: Edit Snapshots, Notifications Inbox, Applications Search FlowFuse/website#2414

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edit snapshot #4282

Edit snapshot #4282

Steve-Mcl commented Jul 30, 2024 •

edited

Loading

codecov bot commented Jul 30, 2024 •

edited

Loading

joepavitt commented Jul 31, 2024

Steve-Mcl commented Jul 31, 2024

Steve-Mcl commented Jul 31, 2024 •

edited

Loading

knolleary commented Jul 31, 2024

Steve-Mcl commented Jul 31, 2024

Steve-Mcl commented Jul 31, 2024

Steve-Mcl commented Jul 31, 2024 •

edited

Loading

hardillb commented Jul 31, 2024

knolleary commented Jul 31, 2024

Steve-Mcl commented Jul 31, 2024 •

edited

Loading

knolleary commented Jul 31, 2024

Steve-Mcl commented Jul 31, 2024

Steve-Mcl commented Jul 31, 2024

joepavitt left a comment

knolleary commented Jul 31, 2024

knolleary commented Aug 1, 2024

Edit snapshot #4282

Edit snapshot #4282

Conversation

Steve-Mcl commented Jul 30, 2024 • edited Loading

Description

Finer detail

Unit Tests added in test/unit/forge/routes/api/snapshots_spec.js

e2e Test added in test/e2e/frontend/cypress/tests/applications/snapshots.spec.js

e2e Test added in test/e2e/frontend/cypress/tests/instances/snapshots.spec.js

e2e Test added in test/e2e/frontend/cypress/tests-ee/devices/snapshots.spec.js

Demo

TODO

Related Issue(s)

Checklist

Labels

codecov bot commented Jul 30, 2024 • edited Loading

Codecov Report

joepavitt commented Jul 31, 2024

Steve-Mcl commented Jul 31, 2024

Steve-Mcl commented Jul 31, 2024 • edited Loading

knolleary commented Jul 31, 2024

Steve-Mcl commented Jul 31, 2024

Steve-Mcl commented Jul 31, 2024

Steve-Mcl commented Jul 31, 2024 • edited Loading

Issue:

Options (from simplest to hardest)

hardillb commented Jul 31, 2024

knolleary commented Jul 31, 2024

Steve-Mcl commented Jul 31, 2024 • edited Loading

knolleary commented Jul 31, 2024

Steve-Mcl commented Jul 31, 2024

Steve-Mcl commented Jul 31, 2024

joepavitt left a comment

Choose a reason for hiding this comment

knolleary commented Jul 31, 2024

knolleary commented Aug 1, 2024

Steve-Mcl commented Jul 30, 2024 •

edited

Loading

Unit Tests added in `test/unit/forge/routes/api/snapshots_spec.js`

e2e Test added in `test/e2e/frontend/cypress/tests/applications/snapshots.spec.js`

e2e Test added in `test/e2e/frontend/cypress/tests/instances/snapshots.spec.js`

e2e Test added in `test/e2e/frontend/cypress/tests-ee/devices/snapshots.spec.js`

codecov bot commented Jul 30, 2024 •

edited

Loading

Steve-Mcl commented Jul 31, 2024 •

edited

Loading

Steve-Mcl commented Jul 31, 2024 •

edited

Loading

Steve-Mcl commented Jul 31, 2024 •

edited

Loading