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: add fields to store when an event was acknowledged and by whom #278

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

Conversation

blenzi
Copy link
Contributor

@blenzi blenzi commented Aug 7, 2023

To address #274 , this PR adds acknowledged_ts and acknowledged_by to events table

…274)

- add acknowledged_ts and acknowledged_by to events table
- Dockerfile-dev: update FROM image
@ghost
Copy link

ghost commented Aug 7, 2023

Nice of you to open a PR 🙏
When you're ready and want to get it reviewed, post a comment in this Pull Request with this message: /quack review

@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #278 (6d67f43) into main (fa2624d) will increase coverage by 0.26%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #278      +/-   ##
==========================================
+ Coverage   95.12%   95.38%   +0.26%     
==========================================
  Files          63       66       +3     
  Lines        1497     1582      +85     
==========================================
+ Hits         1424     1509      +85     
  Misses         73       73              
Flag Coverage Δ
client 100.00% <ø> (?)
unittests 95.15% <100.00%> (+0.03%) ⬆️

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

Files Changed Coverage Δ
src/app/api/endpoints/events.py 100.00% <100.00%> (ø)
src/app/models/event.py 95.83% <100.00%> (+0.59%) ⬆️
src/app/models/user.py 93.75% <100.00%> (+0.41%) ⬆️
src/app/schemas/events.py 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

fe51
fe51 previously approved these changes Aug 17, 2023
Copy link
Member

@fe51 fe51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Bruno, thanks a lot for the changes addressing #274 ! Looks good to me !

@blenzi
Copy link
Contributor Author

blenzi commented Aug 22, 2023

Thanks @fe51 ! There was a problem that caused the client tests to fail: I was recording the access_id instead of the user_id in events/acknowledge_by. This is fixed now.

@fe51
Copy link
Member

fe51 commented Aug 22, 2023

Thanks a lot @blenzi

Copy link
Member

@frgfm frgfm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I only have a doubt about adding the acknowledged_events list in the user table. If that's information needed by an external service, we can query the event table by filtering on the acknowledged_by column, don't you think?

@@ -28,9 +28,12 @@ class Event(Base):
start_ts = Column(DateTime, default=func.now())
end_ts = Column(DateTime, default=None, nullable=True)
is_acknowledged = Column(Boolean, default=False)
acknowledged_by = Column(Integer, ForeignKey("users.id"))
acknowledged_ts = Column(DateTime, default=None, nullable=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to follow the other named fields, perhaps a "acknowledged_at"?

@@ -23,6 +23,7 @@ class User(Base):

access: RelationshipProperty = relationship("Access", uselist=False, back_populates="user")
device: RelationshipProperty = relationship("Device", uselist=False, back_populates="owner")
acknowledged_events: RelationshipProperty = relationship("Event", back_populates="acknowledger")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmh, that's a tricky column to maintain I think?
We could get that information by querying the event table with the user.id. Especially for French regions were everyone uses the same login, that field value could become huge otherwise

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.

3 participants