Delete events v2 #991

mitchross · 2021-04-13T00:42:52Z

Continuation of #858

This is a fork of 858 from @ScottRoach and just adding minor details... We can use this PR for extra notes/cleanup

@paularmstrong - I did not go optimistic route.... Figured you can modify/set pattern in a future refactor + snackbar. I just took your patch suggestion and tested it.

paularmstrong

Sorry for the delay! I haven't had a chance to test manually yet, but got a minute to provide some initial feedback. Overall, looks good, pending a few changes and manual testing

minor: What editor are you using? There are some weird formatting/spacing issues in here. If you are using vscode, the devcontainer in the release-0.9.0 branch should have everything set appropriately for you

Also, please run cd web && npm run lint to auto-fix the lint issues

docker-compose.yml

docker/Dockerfile.base

frigate/edgetpu.py

frigate/http.py

web/src/routes/Event.jsx

mitchross · 2021-04-15T15:50:49Z

Sorry for the delay! I haven't had a chance to test manually yet, but got a minute to provide some initial feedback. Overall, looks good, pending a few changes and manual testing

minor: What editor are you using? There are some weird formatting/spacing issues in here. If you are using vscode, the devcontainer in the release-0.9.0 branch should have everything set appropriately for you

Also, please run cd web && npm run lint to auto-fix the lint issues

I was going to ask about this... Yea VScode, ill run lint!

mitchross · 2021-04-15T16:23:56Z

@paularmstrong believe all comments are addressed now...

Side note @shivanetd is a reactjs / fullstack dev who is a friend of mine. He is willing to do the snackbar/optimistic work. Perhaps after this branch is merged and 9.0 is released you can outline a plan ( https://github.com/blakeblackshear/frigate/projects/2 ) of UI components you want and refactoring and we can team up to implement.

mitchross · 2021-04-26T01:00:00Z

@paularmstrong I just got back from vacation. Any more changes you need from me?

mitchross · 2021-05-10T01:39:50Z

@paularmstrong merge conflicts resolved. Would you please re-review? Thanks!

paularmstrong

Sorry for the delay, life has gotten pretty busy. I'm having issues testing this and it seems to stem from the problems noted inline

paularmstrong · 2021-05-10T15:17:47Z

web/src/routes/Event.jsx

+
+ let success;
+ try {
+ const response = await fetch(`${apiHost}/api/events/${eventId}`, { method: 'DELETE' });


I'm getting a CORS error here when running dev mode (web and backend on separate ports – which would also mean that anyone trying to build on the API externall)

Might need to add this to the nginx.conf in the api section, but needs testing. I'm having some system issues making docker builds problematic:

add_header 'Access-Control-Allow-Methods' 'GET, POST, PUT, DELETE, OPTIONS';

I think the recent merge of #1055 broke this. The change was rather large and I did a sync merge. Thanks for calling this out, ill try the approach here in ngnix.

This is resolved, the 204 in the python code caused all sorts of issues, using a 200 is working.

Still getting a CORS error while using a different host/port. Adding the line I mentioned to nginx/nginx.conf:115 clears up the issue.

diff --git a/nginx/nginx.conf b/nginx/nginx.conf index 51842cb..06760a0 100644 --- a/nginx/nginx.conf +++ b/nginx/nginx.conf @@ -112,6 +112,7 @@ http { location /api/ { add_header 'Access-Control-Allow-Origin' '*'; + add_header 'Access-Control-Allow-Methods' 'GET, POST, PUT, DELETE, OPTIONS'; add_header Cache-Control "no-store"; proxy_pass http:https://frigate_api/; proxy_pass_request_headers on;

Weird. I had that, didn't resolve it. Switched to 200 and removed that line and it fixed it.... No a big deal, I just added it back as I see no harm and logically it makes sense to have.

paularmstrong · 2021-05-10T15:22:27Z

frigate/http.py

+ media.unlink(missing_ok=True)
+
+ event.delete_instance()
+ return make_response(jsonify({"success": True, "message": "Event" + id + " deleted"}),204)


I got an empty response on this. Do 204s not allow response body, perhaps?

204s have nobody by design. If we want an entity returned we can use 200. Its up to you. Maybe to fix the issue below we can use 200 and a empty body.

https://stackoverflow.com/questions/2342579/http-status-code-for-update-and-delete

paularmstrong · 2021-05-10T15:26:48Z

web/src/routes/Event.jsx

+ setDeleteStatus(FetchStatus.ERROR);
+ }
+
+ if (success) {


Clicking "delete", I get the 204 response back from the API, but the box never disappears. Appears to be because of the 204 "NO CONTENT" issue from above, the code is jumping to the catch block and so it's marked as no success.

When this happens (there's an error), there's also no indication of what's going on. It just looks like the request was not made. We should have some sort of error case shown as well as fixing the above.

This is resolved, the 204 in the python code caused all sorts of issues, using a 200 is working.

mitchross · 2021-05-12T02:27:28Z

@paularmstrong Ready for re-re-re-review :)

Docker local issues can be resolved via #1076

paularmstrong · 2021-05-12T14:20:30Z

Still having that CORS issue, otherwise looks great!

paularmstrong

Thanks for sticking with it!

paularmstrong · 2021-05-12T14:26:02Z

This branch cannot be rebased due to conflicts

Sorry, one last rebase to get things in order?

mitchross · 2021-05-12T14:30:37Z

This branch cannot be rebased due to conflicts

Sorry, one last rebase to get things in order?

@paularmstrong
Everything is upto date... Any suggestions?

paularmstrong · 2021-05-12T14:40:38Z

Huh. That's weird.

I would try manually locally:

git remote add upstream https://github.com/blakeblackshear/frigate.git
git fetch upstream
git rebase upstream/release-0.9.0

…nt list view

…Dialog component for confirmation. Redirect to event list after delete

Co-authored-by: Paul Armstrong <[email protected]>

mitchross · 2021-05-12T14:59:33Z

Huh. That's weird.

I would try manually locally:

git remote add upstream https://github.com/blakeblackshear/frigate.git
git fetch upstream
git rebase upstream/release-0.9.0

@paularmstrong Try now, i did the local rebase. Usually I let gitkraken/github desktop do its work for me, but idk had to manually do it. I re-ran it locally and all was good.

paularmstrong · 2021-05-12T15:19:16Z

🎉

mitchross · 2021-05-12T15:20:43Z

🎉

@paularmstrong Happy to help on whatever is next... Id like to add sorting to the events view so I can filter on time.

Co-authored-by: Scott Roach <[email protected]> Co-authored-by: Paul Armstrong <[email protected]>

paularmstrong suggested changes Apr 15, 2021

View reviewed changes

mitchross requested a review from paularmstrong April 16, 2021 01:26

paularmstrong suggested changes May 10, 2021

View reviewed changes

paularmstrong self-requested a review May 12, 2021 14:25

paularmstrong approved these changes May 12, 2021

View reviewed changes

ScottRoach and others added 14 commits May 12, 2021 10:48

Allow deleting events and associated clips and snapshots from the eve…

bd32000

…nt list view

Update 'no more events' row to 9 colspan

a92d8ea

added missing icon file

1d74e67

feat(web): add minimal Dialog component

e0df231

Move delete logic from event list to event detail page and implement …

da80af8

…Dialog component for confirmation. Redirect to event list after delete

Typo in docs

f67f3e7

addressing PR comments

b282e32

wip

13fb9b2

prep for pr

4f56b93

revert debug stuff

35071a4

Update web/src/routes/Event.jsx

3b44a0b

Co-authored-by: Paul Armstrong <[email protected]>

PR comments

302803a

Fix cors issue and change to 200 to resolve dialog bug

5de531f

CORS update

81a4128

mitchross force-pushed the delete-events-v2 branch from 93680d9 to 81a4128 Compare May 12, 2021 14:58

paularmstrong merged commit ebb6d34 into blakeblackshear:release-0.9.0 May 12, 2021

herostrat pushed a commit to herostrat/frigate that referenced this pull request Jun 13, 2021

feat(web): Delete events from Event page and API (blakeblackshear#991)

476ee62

Co-authored-by: Scott Roach <[email protected]> Co-authored-by: Paul Armstrong <[email protected]>

herostrat pushed a commit to herostrat/frigate that referenced this pull request Jun 13, 2021

feat(web): Delete events from Event page and API (blakeblackshear#991)

92840aa

Co-authored-by: Scott Roach <[email protected]> Co-authored-by: Paul Armstrong <[email protected]>

herostrat pushed a commit to herostrat/frigate that referenced this pull request Jun 13, 2021

feat(web): Delete events from Event page and API (blakeblackshear#991)

bad4ba7

Co-authored-by: Scott Roach <[email protected]> Co-authored-by: Paul Armstrong <[email protected]>

sinamics pushed a commit to sinamics/frigate that referenced this pull request Aug 28, 2021

feat(web): Delete events from Event page and API (blakeblackshear#991)

8574bb6

Co-authored-by: Scott Roach <[email protected]> Co-authored-by: Paul Armstrong <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete events v2 #991

Delete events v2 #991

mitchross commented Apr 13, 2021 •

edited

Loading

paularmstrong left a comment

mitchross commented Apr 15, 2021

mitchross commented Apr 15, 2021

mitchross commented Apr 26, 2021

mitchross commented May 10, 2021

paularmstrong left a comment

paularmstrong May 10, 2021

mitchross May 10, 2021

mitchross May 12, 2021

paularmstrong May 12, 2021

paularmstrong May 12, 2021

mitchross May 12, 2021

paularmstrong May 10, 2021

mitchross May 10, 2021

mitchross May 12, 2021

paularmstrong May 10, 2021

mitchross May 12, 2021

mitchross commented May 12, 2021

paularmstrong commented May 12, 2021

paularmstrong left a comment

paularmstrong commented May 12, 2021

mitchross commented May 12, 2021

paularmstrong commented May 12, 2021

mitchross commented May 12, 2021

paularmstrong commented May 12, 2021

mitchross commented May 12, 2021

Delete events v2 #991

Delete events v2 #991

Conversation

mitchross commented Apr 13, 2021 • edited Loading

paularmstrong left a comment

Choose a reason for hiding this comment

mitchross commented Apr 15, 2021

mitchross commented Apr 15, 2021

mitchross commented Apr 26, 2021

mitchross commented May 10, 2021

paularmstrong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchross commented May 12, 2021

paularmstrong commented May 12, 2021

paularmstrong left a comment

Choose a reason for hiding this comment

paularmstrong commented May 12, 2021

mitchross commented May 12, 2021

paularmstrong commented May 12, 2021

mitchross commented May 12, 2021

paularmstrong commented May 12, 2021

mitchross commented May 12, 2021

mitchross commented Apr 13, 2021 •

edited

Loading