Skip to content

urls: Change the method for adding alert words from PUT to POST. #6611

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

Closed
wants to merge 2 commits into from

Conversation

alenavolk
Copy link
Collaborator

@alenavolk alenavolk commented Sep 20, 2017

This deals with the last 1/4 of the issue #5676.

I think the best way to go about this issue is to simply switch the POST and PUT endpoints places, so that the POST endpoint is responsible for adding new values to the list of alert words, and the PUT endpoint is responsible for replacing the list of alert words with new values.

I corrected all related POST and PUT requests within the project, and did my best to make sure that apps like zulip-electron, zulip-mobile, etc. are not using this API, but it is always good to have someone else look over and check.

Please let me know if I missed something or there is a better way to approach this issue.

@zulipbot
Copy link
Member

Hello @zulip/server-api, @zulip/server-misc members, this pull request was labeled with the "area: api", "area: uploads" labels, so you may want to check it out!

@alenavolk alenavolk force-pushed the issue-5676-eliminate-put branch 3 times, most recently from 4676985 to b83fc18 Compare September 22, 2017 19:40
zproject/urls.py Outdated
'POST': 'zerver.views.alert_words.set_alert_words',
'PUT': 'zerver.views.alert_words.add_alert_words',
'POST': 'zerver.views.alert_words.add_alert_words',
'PUT': 'zerver.views.alert_words.set_alert_words',
Copy link
Member

Choose a reason for hiding this comment

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

I thought our idea was to just remove the PUT version and only have POST be add_alert_words. We don't need to support set_alert_words as an interface anymore; we should just remove the code for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the feedback! That's what I initially thought :) But then I noticed that replacing the list of alert words with new values seems useful in tests (for example, here) and got confused. I'll remove the PUT endpoint and change the test then.

@timabbott
Copy link
Member

Thanks for working on this @alenavolk! It looks good; I posted one comment.

@timabbott
Copy link
Member

timabbott commented Sep 23, 2017 via email

@alenavolk
Copy link
Collaborator Author

@timabbott Thanks for the idea, I'll look into that.

@alenavolk alenavolk force-pushed the issue-5676-eliminate-put branch from b83fc18 to 800c21f Compare September 25, 2017 22:09
@alenavolk alenavolk force-pushed the issue-5676-eliminate-put branch from 800c21f to 4d6cfd0 Compare September 26, 2017 00:55
@alenavolk alenavolk changed the title urls: Use POST to add alert words, and PUT to replace them. urls: Change the method for adding alert words from PUT to POST. Sep 26, 2017
@alenavolk
Copy link
Collaborator Author

@timabbott Could you please review this when you have a chance?

I split the changes between two commits for now.

The first one simply removes the old POST endpoint. I realized that there's no reason to use it in tests after all. The test case I mentioned above doesn't really require replacing the list of alert words with new values, because the user starts out with an empty list anyway.

The second one changes the method for adding alert words from PUT to POST and updates all related requests within the project.

@alenavolk alenavolk force-pushed the issue-5676-eliminate-put branch from 4d6cfd0 to 597c00b Compare September 26, 2017 02:02
@timabbott
Copy link
Member

This is great! Merged, thanks @alenavolk!

@timabbott
Copy link
Member

If you're looking for a next thing to do using what you learned doing this:

(1) Deleting the json/tutorial_send_message endpoint from zproject/legacy_urls.py would be great
(2) Moving json/tutorial_status from zproject/legacy_urls.py to be an API-style route would be great.

How do those projects sound?

@alenavolk
Copy link
Collaborator Author

@timabbott The projects sound great! I'll get to them soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants