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

added the notificationIds body parameter for mark multiple notifs as read #14697

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

apoorvapendse
Copy link

@apoorvapendse apoorvapendse commented Jun 10, 2023

This is made to solve issue #14484

Fixes 14484

Changes

//: added the notificationIds parameter for mark multiple notifs as read
image


UUID:2db323ad-4e44-4446-aed2-6d3846e1682a

@CuriousMagpie
Copy link
Member

@phillipthelen This PR looks good to me, but it also looks like it requires an update to the api-v3-integration test.

@apoorvapendse
Copy link
Author

apoorvapendse commented Jun 22, 2023

@phillipthelen This PR looks good to me, but it also looks like it requires an update to the api-v3-integration test.

Is there any way i can help here?

@SabreCat
Copy link
Member

Ah, this... doesn't work. The API expects a body parameter, not a path parameter, so changing the API URL to a path parameter breaks the route entirely. And even if we did change the code to use a path instead of the request body, it'd be a breaking change to existing API consumers for little benefit.

The source issue was simply that the body parameter wasn't documented, not that it needed to be in the path instead. See e.g. https://github.com/HabitRPG/habitica/blob/develop/website/server/controllers/api-v3/tasks.js#L59 -- something like that is all we need!

@apoorvapendse
Copy link
Author

@SabreCat Thanks for the feedback, I'll rectify my mistake and create another PR

@apoorvapendse
Copy link
Author

apoorvapendse commented Jun 24, 2023

Is this how it was expected @SabreCat ?

image

The changes i made are the highlighted lines in the image below:

image

@apoorvapendse apoorvapendse changed the title added the notificationIds parameter for mark multiple notifs as read added the notificationIds body parameter for mark multiple notifs as read Jul 2, 2023
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.

None yet

3 participants