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

Fix notification api route methods #1415

Merged
merged 2 commits into from
Feb 3, 2025
Merged

Fix notification api route methods #1415

merged 2 commits into from
Feb 3, 2025

Conversation

jwr1
Copy link
Member

@jwr1 jwr1 commented Feb 3, 2025

I found two more routes that need their methods changed:

  • The notification settings route should use PUT to indicate that it updates a setting.
  • The notification test route should use POST to indicate that something happens on the backend (sends a test notification). This route was already released, but I do know both Interstellar and joinmbin.org don't use this route, and I did a quick check through a GitHub search and didn't find anything else, so I think we'll be fine updating this route. I'd say out of all the routes that exist, this is probably one of the least important ones anyway.

Just as an aside, for those who don't know, HTTP requests with the GET method are not supposed to have side effects. They should exclusively be a read-only operation that returns data from the server (nothing else should happen). For data mutating routes, POST is best for a create operation, PUT for update, and DELETE for deleting a resource.

@jwr1 jwr1 added the api API related issues and pull requests label Feb 3, 2025
@jwr1 jwr1 requested review from melroy89 and BentiGorlich February 3, 2025 19:33
@jwr1 jwr1 changed the title Change notification api route methods Fix notification api route methods Feb 3, 2025
@jwr1 jwr1 enabled auto-merge (squash) February 3, 2025 19:35
@melroy89
Copy link
Member

melroy89 commented Feb 3, 2025

I agree with this RESTful approach. I think it's indeed a common practice. In both cases.

@melroy89
Copy link
Member

melroy89 commented Feb 3, 2025

That being said. I'm pretty sure api_notification_push_test is used I think at /settings/notifications page. Andapi_notification_settings_update is also used to switch between default, muted or loud notification settings. Are you sure you are not breaking anything?

@jwr1
Copy link
Member Author

jwr1 commented Feb 3, 2025

Both of the routes I changed should only be used in the external facing API, not the ajax routes that the Mbin web client uses. For example, I just tried the "Test Push Notifications" button, and the browser made a request to /ajax/test_push not /api/notification/push/test. We can let @BentiGorlich verify though, just to make sure.

@BentiGorlich
Copy link
Member

if it is in the /api route than it is not used by the web frontend :)

@jwr1 jwr1 merged commit 7f9fef0 into main Feb 3, 2025
7 checks passed
@jwr1 jwr1 deleted the notification-api-method-fix branch February 3, 2025 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api API related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants