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

Http verbs #1837

Merged
merged 7 commits into from
Jan 16, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions evap/rewards/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,21 +205,26 @@ def setUpTestData(cls):
course=course,
)

cls.url = f"/rewards/reward_semester_activation/{cls.semester.pk}/"
cls.url = reverse("rewards:semester_activation_edit", args=[cls.semester.pk])

def test_invalid(self):
niklasmohrin marked this conversation as resolved.
Show resolved Hide resolved
baker.make(SemesterActivation, semester=self.semester, is_active=False)
self.app.post(self.url, user=self.manager, params={"activation_status": "invalid"}, status=400)
self.assertFalse(is_semester_activated(self.semester))

def test_activate(self):
baker.make(SemesterActivation, semester=self.semester, is_active=False)
self.app.post(self.url + "on", user=self.manager)
self.app.post(self.url, user=self.manager, params={"activation_status": "on"})
self.assertTrue(is_semester_activated(self.semester))

def test_deactivate(self):
baker.make(SemesterActivation, semester=self.semester, is_active=True)
self.app.post(self.url + "off", user=self.manager)
self.app.post(self.url, user=self.manager, params={"activation_status": "off"})
self.assertFalse(is_semester_activated(self.semester))

def test_activate_after_voting(self):
baker.make(SemesterActivation, semester=self.semester, is_active=False)
self.assertEqual(0, reward_points_of_user(self.student))
response = self.app.post(self.url + "on", user=self.manager)
response = self.app.post(self.url, user=self.manager, params={"activation_status": "on"}).follow()
self.assertContains(response, "3 reward points were granted")
self.assertEqual(3, reward_points_of_user(self.student))
2 changes: 1 addition & 1 deletion evap/rewards/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@
path("reward_point_redemption_event/<int:event_id>/export", views.reward_point_redemption_event_export, name="reward_point_redemption_event_export"),
path("reward_point_redemption_event/delete", views.reward_point_redemption_event_delete, name="reward_point_redemption_event_delete"),

path("reward_semester_activation/<int:semester_id>/<str:active>", views.semester_activation, name="semester_activation"),
path("semester_activation/<int:semester_id>/edit", views.semester_activation_edit, name="semester_activation_edit"),
]
17 changes: 11 additions & 6 deletions evap/rewards/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
SemesterActivation,
)
from evap.rewards.tools import grant_eligible_reward_points_for_semester, reward_points_of_user, save_redemptions
from evap.staff.views import semester_view


def redeem_reward_points(request):
Expand Down Expand Up @@ -137,13 +136,19 @@ def reward_point_redemption_event_export(request, event_id):
return response


@require_POST
@manager_required
def semester_activation(request, semester_id, active):
def semester_activation_edit(request, semester_id):
semester = get_object_or_404(Semester, id=semester_id)
active = active == "on"

status = request.POST.get("activation_status")
if status == "on":
active = True
elif status == "off":
active = False
else:
raise SuspiciousOperation("Invalid activation keyword")
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
Copy link
Member

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.

return redirect("staff:semester_view", semester_id) # semester_view(request=request, semester_id=semester_id)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return redirect("staff:semester_view", semester_id) # semester_view(request=request, semester_id=semester_id)
return redirect("staff:semester_view", semester_id)

27 changes: 8 additions & 19 deletions evap/staff/templates/staff_semester_view.html
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,12 @@
{% load evaluation_filters %}

{% block content %}
{{ block.super }}

{{ block.super }}
niklasmohrin marked this conversation as resolved.
Show resolved Hide resolved
<form id="form_activation_status" method="post" action="{% url 'rewards:semester_activation_edit' semester.id%}">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<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 %}">

<input type="hidden" id="activation_status" name="activation_status" value="on"> {# FIXME #}
Copy link
Member

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)

Copy link
Member

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

{% csrf_token %}
</form>
<h3>
{{ semester.name }}
{% if request.user.is_manager %}
Expand Down Expand Up @@ -69,7 +73,7 @@ <h3>
<div class="btn-switch-label">{% trans 'Reward points active' %}</div>
<div class="btn-switch btn-group icon-buttons">
<button type="button" onclick="activateRewardPointsModalShow({{ semester.id }}, '{{ semester.name|escapejs }}');" class="btn btn-sm btn-light{% if rewards_active %} active{% endif %}"><span class="fas fa-check" aria-hidden="true"></span></button>
<button type="button" onclick="deactivateRewardPoints()" class="btn btn-sm btn-light{% if not rewards_active %} active{% endif %}"><span class="fas fa-xmark" aria-hidden="true"></span></button>
<button type="submit" name="activation_status" value="off" form="form_activation_status" class="btn btn-sm btn-light{% if not rewards_active %} active{% endif %}"><span class="fas fa-xmark" aria-hidden="true"></span></button>
</div>
</div>
</div>
Expand Down Expand Up @@ -549,23 +553,8 @@ <h3>
{% trans 'Activate reward points' as action_text %}
{% include 'confirmation_modal.html' with modal_id='activateRewardPointsModal' title=title question=question action_text=action_text btn_type='primary' %}
<script type="text/javascript">
function activateRewardPointsModalAction(dataId) {
$.ajax({
type: "POST",
url: "{% url 'rewards:semester_activation' semester.id 'on' %}",
data: {},
success: function(){ location.reload(); },
error: function(){ window.alert("{% trans 'The server is not responding.' %}"); }
});
};
function deactivateRewardPoints() {
$.ajax({
type: "POST",
url: "{% url 'rewards:semester_activation' semester.id 'off' %}",
data: {},
success: function(){ location.reload(); },
error: function(){ window.alert("{% trans 'The server is not responding.' %}"); }
});
function activateRewardPointsModalAction() {
document.getElementById("form_activation_status").requestSubmit(); {# FIXME #}
}
</script>
{% endblock %}
Expand Down