-
Notifications
You must be signed in to change notification settings - Fork 72
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
fixes #677 Replace GET with POST when necessary, and refactored a bit… #685
fixes #677 Replace GET with POST when necessary, and refactored a bit… #685
Conversation
…efactored a bit the post utils in js
@@ -87,6 +87,12 @@ function openConfirmationBox(name) { | |||
$('#confirmation-dialog').dialog('open'); | |||
} | |||
|
|||
function postWithCsrf(path) { |
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.
Could we pass a Csrf param into params
of post
instead of making a new function? Any reason against?
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.
nope we can do this - i was starting with a "soft refactoring" 😂
I'll have a look
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 think I actually like that function as a "shortcut" so we don't have to paste the token everywhere (see next comment)
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.
Sure 👍
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.
This is not reviewable, I can't resolve this discussion = (
<a id="allow_button" class="button button--small button-primary button--primary--positive" | ||
href="{% url 'organisation_allow_join' join_request.id %}">Allow</a></td> | ||
<button id="allow_button" class="button button--small button-primary button--primary--positive" | ||
onclick="post('{% url 'organisation_allow_join' join_request.id %}', { |
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.
Question as I'm not sure exactly what's happening here, can this post be now replaced withpostWithCsrf()
if we decide to use it (or post with a param if you accept my suggestion)?
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.
yup -
here i think postWithCsrf might prove more concise
… the post utils in js
The following requests are now using POST instead of GET:
Assigning / unassigning school admins: was POST already (tested on codeforlife.education)
Same for leaving a school