Add notification API tests #5715
Conversation
d-rita
force-pushed
the
chore/add-notifications-tests
branch
2 times, most recently
from
e663cad
to
9ad0953
Compare
|
@d-rita is this ready for review? |
@Aadesh-Baral not yet, two failing tests for the get and post unread count api endpoints |
d-rita
force-pushed
the
chore/add-notifications-tests
branch
from
9ad0953
to
b400d8d
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
@d-rita What's the status of this PR? |
d-rita
force-pushed
the
chore/add-notifications-tests
branch
from
b400d8d
to
76f122f
Compare
|
@Aadesh-Baral this is ready for review.
|
| message_type_filters = [ | ||
| key.strip().lower() for key in message_type.split(",") | ||
| ] | ||
|
|
||
| filter_values = [ | ||
| key.value | ||
| for key in MessageType | ||
| if key.name.lower() in message_type_filters | ||
| ] | ||
|
|
||
| query = query.filter(Message.message_type.in_(filter_values)) |
There was a problem hiding this comment.
Can you explain this change?
There was a problem hiding this comment.
While testing, I could not retrieve any notifications if I specified the message type. Not when I used numbers (for the mapped integer value assigned to a type under the MessageType enum class), e.g. 1 (system), 2 (broadcast)., etc. Nor when I typed in the string value itself, e.g. system/SYSTEM/System. That was with the previous implementation.
So, this new implementation lets a user retrieve the specific messages when they filter by the string value of the message type, e.g. system, mention_notification, broadcast, etc.
Could you test to confirm if my findings were accurate or not? @Aadesh-Baral
There was a problem hiding this comment.
This change has introduced a problem on the frontend. In our frontend code, we have four notification types that are associated with different combinations of MessageTypes. You can find more information about this issue in the following link: Comment.
For instance, when applying a teams filter in the frontend, a query parameter is passed as "types=6,7,11" in the NotificationsAllAPI. To filter and retrieve the notifications for the provided message types on the backend, this value needs to be split by comma (',') and converted to integers.
However, due to this change, the value is being split correctly but is not being converted to integers, resulting in empty results being returned.
There was a problem hiding this comment.
Noted. I have adjusted the tests, removed this change, and added tests for the sortBy and sortDirection query parameters.
Sidenote: For future reference, we should probably include in the documentation the expected values for the message_type filter and what type the respective numerical values correspond to.
There was a problem hiding this comment.
For future reference, we should probably include in the documentation the expected values for the message_type filter and what type the respective numerical values correspond to.
Sounds good
There was a problem hiding this comment.
For future reference, we should probably include in the documentation the expected values for the message_type filter and what type the respective numerical values correspond to.
@d-rita Let's do this in this PR itself.
d-rita
force-pushed
the
chore/add-notifications-tests
branch
from
76f122f
to
aa69d22
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
This PR adds tests related to the Notifications API endpoints.
Related issue:
How to test:
test_git fetch origin && git checkout chore/add-notifications-testspython3 -m unittest discover tests/backend