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: fields and methods to set event type (wildfire or something else) #291

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

blenzi
Copy link
Contributor

@blenzi blenzi commented Oct 9, 2023

This PR adds the possibility to set an event type, different than wildfire. This event assessment is currently done via pyro-platform but this information is only stored locally (in the machine running it).

  • add options to EventType enum: domestic fire, chimney, cloud, other, undefined (default)
  • add type_set_by and type_set_ts to events table
  • add PUT /events/{event_id}/type endpoint and set_event_type method to client

- add options to EventType enum: domestic fire, chimney, cloud, other, undefined (default)
- add type_set_by and type_set_ts to events table
- add PUT /events/{event_id}/type endpoint
@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

Merging #291 (3abe13c) into main (5f735c3) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #291      +/-   ##
==========================================
+ Coverage   95.35%   95.43%   +0.07%     
==========================================
  Files          66       66              
  Lines        1571     1598      +27     
==========================================
+ Hits         1498     1525      +27     
  Misses         73       73              
Flag Coverage Δ
client 100.00% <100.00%> (ø)
unittests 95.20% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
client/pyroclient/client.py 100.00% <100.00%> (ø)
src/app/api/endpoints/events.py 100.00% <100.00%> (ø)
src/app/models/event.py 96.55% <100.00%> (+1.31%) ⬆️
src/app/models/user.py 93.75% <100.00%> (+0.41%) ⬆️
src/app/schemas/events.py 100.00% <100.00%> (ø)

@frgfm
Copy link
Member

frgfm commented Oct 18, 2023

Thanks for the PR!
I'm aligned with everything apart from the "set_by" and "set_ts". I think we're mixing the role of APM (being able to access logs / interactions) vs. what needs to be recorded in the DB. My suggestion:

  • simply removing the set columns
  • I can come up with a PR to add a cool APM I've been using (posthog) where we can explore events like those

Here is a screenshot once it's set:
image

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