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

feat(sequences): implement the frontend-specific routes #412

Merged
merged 26 commits into from
Jan 17, 2025

Conversation

frgfm
Copy link
Member

@frgfm frgfm commented Jan 16, 2025

This PR follows up on #405, and implements the final steps for the interactions with detections and sequences. I haven't implemented / updated the unit tests for now but used the end-to-end script to ensure this works well.

Data model

A few changes:

  • "is_wildfire" is moved from Detection into Sequence
  • removed "updated_at" from Detection since we're not using it for now (and it was meant for labeling, which will most likely happen at the sequence level)

Here is the updated version: https://dbdiagram.io/d/Pyronear-Alert-Schema-671e1b9897a66db9a3673d76

Route updates

  • removed GET detections/unlabeled/fromdate and PATCH detections/DET_ID/label
  • created PATCH sequences/SEQ_ID/label, GET sequences/unlabeled/latest (last 15 unlabeled sequences from last 24 hours), GET sequences/all/fromdate (all sequences from a specific date), and GET sequences/SEQ_ID/detections (fetch the last 10 detections from that sequence with their URL)

Creation/update logic

  • as discussed with @MateoLostanlen , apart from GET sequences/SEQ_ID/detections, the sequences route returns Sequences. So you'll have to use that route to fetch the detections, but it's fast
  • big warning here: we need to discuss the webhook/telegram workflow. Currently it sends a notification when a detection is created, I would suggest to do this only when a sequence is created to avoid flooding telegram (and potentially when a new detection is added to a sequence)

Review suggestion

  • check the changes in models.py in the first commit
  • check the changes in the detection and sequence routers
  • the rest is not important, utils mostly

Other considerations

I haven't put the camera name in the sequences yet, I'll do it when finalizing the PR if that's alright.

frgfm added 3 commits January 16, 2025 11:48

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@frgfm frgfm added this to the 0.2.0 milestone Jan 16, 2025
@frgfm frgfm self-assigned this Jan 16, 2025
@frgfm frgfm changed the title feat(sequences feat(sequences): implement the frontend-specific routes Jan 16, 2025
@MateoLostanlen
Copy link
Member

Thanks for the PR @frgfm

As discussed together I'm ok with the strategy

The code logic looks good to me, I don't know SQL so it's hard to say more without testing :)

You're missing the route for acknowledge, aren't you?

Good point about teegram, can you open an issue maybe?

I'll let you see about the test that fails

PS: You should remove the test required in 3.9

@frgfm
Copy link
Member Author

frgfm commented Jan 16, 2025

You're missing the route for acknowledge, aren't you?

The point was that this would be PATCH sequences/SEQ_ID/label, which sets the value for "is_wildfire". I thought this was already agreed but let me know!

Good point about telegram, can you open an issue maybe?

done

@MateoLostanlen
Copy link
Member

Ok my bad I missed that, event better I will make the change on the platform !

MateoLostanlen
MateoLostanlen previously approved these changes Jan 16, 2025
frgfm added 5 commits January 16, 2025 15:36

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 85.56701% with 14 lines in your changes missing coverage. Please review.

Project coverage is 85.23%. Comparing base (ccd95d2) to head (4167083).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/app/api/api_v1/endpoints/sequences.py 83.33% 9 Missing ⚠️
src/app/api/api_v1/endpoints/detections.py 16.66% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #412      +/-   ##
==========================================
- Coverage   85.56%   85.23%   -0.34%     
==========================================
  Files          38       38              
  Lines        1053     1097      +44     
==========================================
+ Hits          901      935      +34     
- Misses        152      162      +10     
Flag Coverage Δ
backend 84.57% <80.55%> (-0.58%) ⬇️
client 93.67% <100.00%> (+1.60%) ⬆️

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.

@github-actions github-actions bot added the topic: build Related to build, installation & CI label Jan 17, 2025
@frgfm frgfm marked this pull request as ready for review January 17, 2025 10:12
@frgfm frgfm merged commit 6c8b02f into main Jan 17, 2025
26 of 27 checks passed
@frgfm frgfm deleted the frgfm/frontend-routes branch January 17, 2025 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants