-
Notifications
You must be signed in to change notification settings - Fork 149
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
Notifications api endpoint #1347
Conversation
4ff1eb5
to
53a5c84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding these tests! This looks very ready. Left just a couple NIT comments :D
@@ -284,6 +284,30 @@ paths: | |||
type: string | |||
description: Reason for the failure | |||
description: '' | |||
/api/notifications/: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By running the tests the API schema and redoc docs get updated with the new endpoint 👌
tests/utils/trade.py
Outdated
path = reverse("notifications") | ||
params = f"?created_at={created_at}" | ||
headers = self.get_robot_auth(robot_index, first_encounter) | ||
self.response = self.client.get(path + params, **headers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During tests, trade.response
is always the return of the latest GET to /api/order
. Maybe here we want to create a new variable on the Trade class. for example: notification
. This way, in tests we evaluate (assert) the value of trade.response
for the latest state of an order, and of trade.notification
for the latest notification, and don't get about what is the type of response
.
I would therefore edit this one as:
self.notification = self.client.get(path + params, **headers)
notification
or last_notification
, the one name you prefer!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up moving it out of Trade, I think it didn't make sense to have it there
tests/test_trade_pipeline.py
Outdated
@@ -802,6 +955,17 @@ def test_taken_order_expires(self): | |||
|
|||
self.assert_order_logs(data["id"]) | |||
|
|||
trade.get_notifications() | |||
# Checks | |||
self.assertResponse(trade.response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use trade.notification
as the response stored by the get_notification() method, that will make tests more clear :)
53a5c84
to
eed8c11
Compare
Sorry for the confusion @Reckless-Satoshi 😆 test are not yet ready to be review, I'm investigating a possible wrong test on |
dfd7603
to
28620e9
Compare
08802b5
to
030663b
Compare
5117dbc
to
4a7a28b
Compare
c1b4307
to
284d0f4
Compare
284d0f4
to
326a2f1
Compare
2930d2b
to
5c08dd3
Compare
What does this PR do?
This PR introduces a new API endpoint for the robot to fetch all notifications sent to it. To reduce bandwidth it allows to filter by the creation date.
A new table Notification has been created to have a place to store these notifications.
Checklist before merging
pip install pre-commit
, thenpre-commit install
. Pre-commit installs git hooks that automatically check the codebase. If pre-commit fails when you commit your changes, please fix the problems it points out.