Skip to content

Commit

Permalink
Merge pull request #650 from MAKENTNU/cleanup/url-query-parameters-in…
Browse files Browse the repository at this point in the history
…stead-of-distinct-paths

Make some views use URL query parameters instead of distinct paths
  • Loading branch information
ddabble committed May 7, 2023
2 parents 82631fa + f70538e commit f84561b
Show file tree
Hide file tree
Showing 49 changed files with 1,139 additions and 488 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ A summary of changes made to the codebase, grouped per deployment.
- Enabled automated code quality checks from [Code Climate](https://codeclimate.com/quality)
- Renamed lots of views, forms and templates to comply with
[the style guides](https://github.com/MAKENTNU/web/blob/2826b57a6c6fe27446c88edb19ca167a728b5eb4/CONTRIBUTING.md#code-style-guides)
- Changed multiple pages' URLs to use [query parameters](https://en.wikipedia.org/wiki/Query_string),
instead of having multiple distinct paths for practically the same page
(see [#650](https://github.com/MAKENTNU/web/pull/650) for a list of all the URLs affected)
- Permanent redirects have been added for the URLs deemed most relevant, to redirect from the old to the new URL; see [a list of the added URLs in
the code](https://github.com/MAKENTNU/web/pull/650/files#diff-37d0e0a00e828360d35d68a7ada510a98e03252045f3d51d36a81fedfaea7907R111-R115)
- Changed some URLs so that they all consistently use `add` (instead of `create`) and `change` (instead of `edit` or `update`),
to comply with [the style guide](https://github.com/MAKENTNU/web/blob/2826b57a6c6fe27446c88edb19ca167a728b5eb4/CONTRIBUTING.md#endpoint-pathroute)
- Made the use of add/change verbs consistent among page titles and buttons, and some other parts of the website's UI
Expand Down
12 changes: 6 additions & 6 deletions src/checkin/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

from django.contrib import messages
from django.contrib.auth.mixins import PermissionRequiredMixin
from django.http import HttpResponse, HttpResponseRedirect, JsonResponse
from django.http import HttpResponse, HttpResponseRedirect
from django.shortcuts import get_object_or_404
from django.urls import reverse
from django.utils import timezone
Expand All @@ -15,7 +15,7 @@

from card import utils as card_utils
from card.views import RFIDView
from util.view_utils import PreventGetRequestsMixin
from util.view_utils import PreventGetRequestsMixin, UTF8JsonResponse
from .models import Profile, RegisterProfile, Skill, SuggestSkill, UserSkill


Expand Down Expand Up @@ -221,15 +221,15 @@ def post(self, request):
if forced:
Skill.objects.create(title=suggestion.title, title_en=suggestion.title_en, image=suggestion.image)
suggestion.delete()
return JsonResponse(response_dict)
return UTF8JsonResponse(response_dict)

suggestion.voters.add(request.user.profile)
response_dict['skill_passed'] = suggestion.voters.count() >= 5
if response_dict['skill_passed']:
Skill.objects.create(title=suggestion.title, title_en=suggestion.title_en, image=suggestion.image)
suggestion.delete()

return JsonResponse(response_dict)
return UTF8JsonResponse(response_dict)


class AdminAPISuggestSkillDeleteView(PermissionRequiredMixin, PreventGetRequestsMixin, DeleteView):
Expand All @@ -238,7 +238,7 @@ class AdminAPISuggestSkillDeleteView(PermissionRequiredMixin, PreventGetRequests

def delete(self, request, *args, **kwargs):
self.get_object().delete()
return JsonResponse({'suggestion_deleted': True})
return UTF8JsonResponse({'suggestion_deleted': True})


class AdminRegisterCardView(RFIDView):
Expand Down Expand Up @@ -271,7 +271,7 @@ def post(self, request):
request.user.card_number = card_number
request.user.save()
RegisterProfile.objects.all().delete()
return JsonResponse(response_dict)
return UTF8JsonResponse(response_dict)


class AdminProfilePictureUpdateView(PreventGetRequestsMixin, View):
Expand Down
5 changes: 5 additions & 0 deletions src/docs/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,3 +77,8 @@ def clean(self):
'current_content': _("The content does not belong to the given page"),
})
return cleaned_data


class DocumentationPageSearchQueryForm(forms.Form):
query = forms.CharField(required=False, max_length=1000, strip=False)
page = forms.IntegerField(required=False, min_value=1)
2 changes: 1 addition & 1 deletion src/docs/templates/docs/documentation_page_search.html
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
{% load i18n %}


{% block title %}{% translate "Search results" %} | {% translate "Docs" %}{% endblock title %}
{% block title %}{% translate "Search results" %}: "{{ query }}" | {% translate "Docs" %}{% endblock title %}

{% block extra_head %}
<script>
Expand Down
2 changes: 2 additions & 0 deletions src/docs/tests/test_urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ def test_all_get_request_paths_succeed(self):
Get(self.reverse('documentation_page_create'), public=False),
Get(self.reverse('documentation_page_update', self.page1.pk), public=False),
Get(self.reverse('documentation_page_search'), public=False),
Get(f"{self.reverse('documentation_page_search')}?query=lorem", public=False),
Get(f"{self.reverse('documentation_page_search')}?query=lorem&page=2", public=False),
Get('/robots.txt', public=True, translated=False),
Get('/.well-known/security.txt', public=True, translated=False),
]
Expand Down
12 changes: 7 additions & 5 deletions src/docs/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
from django.views.generic.edit import ModelFormMixin

from util.templatetags.string_tags import title_en
from util.view_utils import CustomFieldsetFormMixin, PreventGetRequestsMixin, insert_form_field_values
from .forms import AddPageForm, ChangePageVersionForm, PageContentForm
from util.view_utils import CustomFieldsetFormMixin, PreventGetRequestsMixin, QueryParameterFormMixin, insert_form_field_values
from .forms import AddPageForm, ChangePageVersionForm, DocumentationPageSearchQueryForm, PageContentForm
from .models import Content, MAIN_PAGE_TITLE, Page


Expand Down Expand Up @@ -166,8 +166,10 @@ class DocumentationPageDeleteView(PermissionRequiredMixin, PreventGetRequestsMix
success_url = reverse_lazy('home')


class DocumentationPageSearchView(TemplateView):
class DocumentationPageSearchView(QueryParameterFormMixin, TemplateView):
form_class = DocumentationPageSearchQueryForm
template_name = 'docs/documentation_page_search.html'

page_size = 10

@staticmethod
Expand All @@ -180,9 +182,9 @@ def pages_to_show(current_page, n_pages):

def get_context_data(self, **kwargs):
context_data = super().get_context_data(**kwargs)
query = self.query_params['query']
page = self.query_params['page'] or 1

query = self.request.GET.get('query', "")
page = int(self.request.GET.get('page', 1))
pages = Page.objects.filter(Q(title__icontains=query) | Q(current_content__content__icontains=query))
n_pages = ceil(pages.count() / self.page_size)
context_data.update({
Expand Down
94 changes: 55 additions & 39 deletions src/groups/tests/test_admin.py
Original file line number Diff line number Diff line change
@@ -1,51 +1,67 @@
from django.contrib.admin.sites import AdminSite
from django.test import TestCase
from http import HTTPStatus

from django.contrib.auth.models import Permission
from django.contrib.contenttypes.models import ContentType
from django.test import Client, TestCase

from users.models import User
from util.test_utils import set_without_duplicates
from web.tests.test_urls import ADMIN_CLIENT_DEFAULTS, reverse_admin
from ..admin import InheritanceGroupAdmin
from ..models import InheritanceGroup


class MockRequest:
pass
class InheritanceGroupAdminTestCase(TestCase):

def setUp(self):
self.admin = User.objects.create_user("admin", is_staff=True, is_superuser=True)
self.admin_client = Client(**ADMIN_CLIENT_DEFAULTS)
self.admin_client.force_login(self.admin)

class MockSuperUser:
self.org = InheritanceGroup.objects.create(name='Org')
self.mentor = InheritanceGroup.objects.create(name='Mentor')
self.dev = InheritanceGroup.objects.create(name='Dev')
self.event = InheritanceGroup.objects.create(name='Event')
self.chairperson = InheritanceGroup.objects.create(name='Leder')

def has_perm(self, *args, **kwargs):
return True
self.mentor.parents.add(self.org)
self.dev.parents.add(self.org)
self.event.parents.add(self.org)
self.chairperson.parents.add(self.mentor, self.dev, self.event)

self.org_perms = {
Permission.objects.create(codename='perm1', name="Perm 1", content_type=ContentType.objects.get_for_model(InheritanceGroup))
}
self.org.own_permissions.add(*self.org_perms)

class InheritanceGroupAdminTestCase(TestCase):
self.admin_add_url = reverse_admin('groups_inheritancegroup_add')
self.admin_change_dev_url = reverse_admin('groups_inheritancegroup_change', args=[self.dev.pk])

def setUp(self):
self.site = AdminSite()
self.request = MockRequest()
self.request.user = MockSuperUser()
org = InheritanceGroup.objects.create(name='Org')
mentor = InheritanceGroup.objects.create(name='Mentor')
mentor.parents.add(org)
dev = InheritanceGroup.objects.create(name='Dev')
dev.parents.add(org)
arr = InheritanceGroup.objects.create(name='Arrangement')
arr.parents.add(org)
InheritanceGroup.objects.create(name='Leder').parents.add(mentor, dev, arr)

def test_get_form(self):
admin = InheritanceGroupAdmin(InheritanceGroup, self.site)
def test_form_contains_expected_fields_and_related_field_querysets(self):
response = self.admin_client.get(self.admin_add_url)
self.assertEqual(response.status_code, HTTPStatus.OK)
form = response.context['adminform']
expected_fields = ['name', 'parents', 'own_permissions']
form = admin.get_form(self.request)
self.assertListEqual(list(form.base_fields), expected_fields)

expected_parents = InheritanceGroup.objects.all()
self.assertSetEqual(set(form.base_fields['parents'].queryset), set(expected_parents))

form = admin.get_form(self.request, obj=InheritanceGroup.objects.get(name='Dev'))
expected_parents = InheritanceGroup.objects.get(name='Dev').get_available_parents()
self.assertSetEqual(set(form.base_fields['parents'].queryset), set(expected_parents))

def test_inherited_permissions(self):
admin = InheritanceGroupAdmin(InheritanceGroup, self.site)
dev = InheritanceGroup.objects.get(name='Dev')
permissions = set(admin.get_inherited_permissions(dev))
for perm in dev.inherited_permissions:
self.assertIn(str(perm), permissions)
self.assertListEqual(list(form.fields), expected_fields)
self.assertSetEqual(
set_without_duplicates(self, form.fields['parents'].queryset),
set(InheritanceGroup.objects.all()),
)

response = self.admin_client.get(self.admin_change_dev_url)
self.assertEqual(response.status_code, HTTPStatus.OK)
form = response.context['adminform']
self.assertListEqual(list(form.fields), expected_fields)
self.assertSetEqual(
set_without_duplicates(self, form.fields['parents'].queryset),
set(self.dev.get_available_parents()),
)

def test_get_inherited_permissions_contains_expected_permissions(self):
response = self.admin_client.get(self.admin_change_dev_url)
self.assertEqual(response.status_code, HTTPStatus.OK)
admin: InheritanceGroupAdmin = response.context['adminform'].model_admin

permissions_string = admin.get_inherited_permissions(self.dev)
permissions = set_without_duplicates(self, permissions_string.splitlines())
self.assertSetEqual(permissions, {str(perm) for perm in self.org_perms})
2 changes: 0 additions & 2 deletions src/groups/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from ..models import Committee, InheritanceGroup


# See the `0008_add_default_inheritancegroups_and_committees.py` migration for which InheritanceGroups are created by default
class PermGroupTestCase(TestCase):

def setUp(self):
Expand Down Expand Up @@ -199,7 +198,6 @@ def test_inherited_permissions(self):
self.assertIn(perm, inherited_permissions)


# See the `0008_add_default_inheritancegroups_and_committees.py` migration for which Committees are created by default
class CommitteeTestCase(TestCase):

def setUp(self):
Expand Down
Loading

0 comments on commit f84561b

Please sign in to comment.