-
Notifications
You must be signed in to change notification settings - Fork 146
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
Http verbs #1837
Http verbs #1837
Conversation
If you want to try at home: The branch is not based on the current version of our ... or we do it together at the next meeting :) |
bf5f19a
to
3d20866
Compare
3d20866
to
789a39c
Compare
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.
Code looks very nice. I have some minor formatting and some project management / organizational annotations though.
evap/rewards/views.py
Outdated
|
||
return semester_view(request=request, semester_id=semester_id) | ||
messages.get_messages(request).used = False | ||
return redirect("staff:semester_view", semester_id) # semester_view(request=request, semester_id=semester_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.
return redirect("staff:semester_view", semester_id) # semester_view(request=request, semester_id=semester_id) | |
return redirect("staff:semester_view", semester_id) |
evap/rewards/views.py
Outdated
SemesterActivation.objects.update_or_create(semester=semester, defaults={"is_active": active}) | ||
if active: | ||
grant_eligible_reward_points_for_semester(request, semester) | ||
|
||
return semester_view(request=request, semester_id=semester_id) | ||
messages.get_messages(request).used = False |
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.
Is this necessary / what does it do? I think we've used redirects before and everything worked fine with messages.
|
||
{{ block.super }} | ||
<form id="form_activation_status" method="post" action="{% url 'rewards:semester_activation_edit' semester.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.
<form id="form_activation_status" method="post" action="{% url 'rewards:semester_activation_edit' semester.id%}"> | |
<form id="form_activation_status" method="post" action="{% url 'rewards:semester_activation_edit' semester.id %}"> |
|
||
{{ block.super }} | ||
<form id="form_activation_status" method="post" action="{% url 'rewards:semester_activation_edit' semester.id%}"> | ||
<input type="hidden" id="activation_status" name="activation_status" value="on"> {# FIXME #} |
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'd oppose having a fixme without any comment here. I would be (hesitantly) okay with a reference to an issue (FIXME #123456
). Overall, I'm just in favor of adding the issue and not adding the fixme. TODOs in code have a tendency to stay there.
@niklasmohrin what do you think? (Follow-up to #1837 (comment))
(repeated below in the javascript)
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.
No strong opinion, we can make an issue if you want
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.
Looks good, final touches:
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.
Nice! Thanks :)
Thanks for submitting the PR :) |
It changes the HTTP-Endpoint for a semester activation.
Due to the nature of the confirmation modals, only the query directly effected was changed to a form.