diff --git a/common/static/common/styles/_extension.sass b/common/static/common/styles/_extension.sass index 294fbb40..8deb6fce 100644 --- a/common/static/common/styles/_extension.sass +++ b/common/static/common/styles/_extension.sass @@ -416,11 +416,17 @@ @extend .dropdown-divider +margin(0, top) + +margin(1, bottom) -.dropdown-item - &a +a + &.dropdown-item +padding(3, x) +a + &.dropdown-item-disabled + opacity: .5 + pointer-events: none + .extension-icon display: inline-block vertical-align: bottom diff --git a/common/templates/common/components/field.html b/common/templates/common/components/field.html index 5c8e347f..826cc098 100644 --- a/common/templates/common/components/field.html +++ b/common/templates/common/components/field.html @@ -14,7 +14,7 @@ {% if not field.is_hidden %} {% endif %} @@ -24,7 +24,7 @@ {% if not field.is_hidden %} {% endif %} diff --git a/extensions/forms.py b/extensions/forms.py index 07bf596b..17f88287 100644 --- a/extensions/forms.py +++ b/extensions/forms.py @@ -163,6 +163,18 @@ class ExtensionUpdateForm(forms.ModelForm): self.add_preview_formset.error_messages['too_few_forms'] = self.msg_need_previews + user_teams = self.request.user.teams.all() + if self.request.user in self.instance.authors.all() and len(user_teams) > 0: + team_slug = None + if self.instance.team: + team_slug = self.instance.team.slug + choices = [(None, 'None'), *[(team.slug, team.name) for team in user_teams]] + self.fields['team'] = forms.ChoiceField( + choices=choices, + required=False, + initial=team_slug, + ) + def is_valid(self, *args, **kwargs) -> bool: """Validate all nested forms and form(set)s first.""" if 'submit_draft' in self.data: @@ -198,6 +210,27 @@ class ExtensionUpdateForm(forms.ModelForm): return all(is_valid_flags) + def clean_team(self): + # don't modify instance if the field value wasn't sent + # empty value reset the team + if 'team' in self.data: + # TODO permissions check + # shouldn't happen normally: the form doesn't render the select + if self.request.user not in self.instance.authors.all(): + self.add_error('team', _('Not allowed to set the team')) + return + + team_slug = self.cleaned_data['team'] + if team_slug: + team = self.request.user.teams.filter(slug=team_slug).first() + if not team: + self.add_error('team', _('User does not belong to the team')) + return + else: + self.instance.team = team + else: + self.instance.team = None + def clean(self): """Perform additional validation and status changes.""" super().clean() diff --git a/extensions/models.py b/extensions/models.py index 91e5d98b..d6ba7ff0 100644 --- a/extensions/models.py +++ b/extensions/models.py @@ -128,12 +128,19 @@ class ExtensionManager(models.Manager): def unlisted(self): return self.exclude(status=self.model.STATUSES.APPROVED) - def authored_by(self, user_id: int): - return self.filter(maintainer__user_id=user_id) + def _authored_by_filter(self, user): + filter = Q(maintainer__user_id=user.pk) + user_teams = user.teams.all() + if user_teams: + filter = filter | Q(team__in=[t.pk for t in user_teams]) + return filter - def listed_or_authored_by(self, user_id: int): + def authored_by(self, user): + return self.filter(self._authored_by_filter(user)).distinct() + + def listed_or_authored_by(self, user): return self.filter( - Q(status=self.model.STATUSES.APPROVED) | Q(maintainer__user_id=user_id) + Q(status=self.model.STATUSES.APPROVED) | self._authored_by_filter(user) ).distinct() @@ -385,10 +392,14 @@ class Extension(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Mod ) def has_maintainer(self, user) -> bool: - """Return True if given user is listed as a maintainer.""" + """Return True if given user is listed as a maintainer or is a member of the team.""" if user is None or user.is_anonymous: return False - return user in self.authors.all() + if user in self.authors.all(): + return True + if self.team and user in self.team.users.all(): + return True + return False def can_rate(self, user) -> bool: """Return True if given user can rate this extension. diff --git a/extensions/templates/extensions/components/extension_form.html b/extensions/templates/extensions/components/extension_form.html new file mode 100644 index 00000000..1431581c --- /dev/null +++ b/extensions/templates/extensions/components/extension_form.html @@ -0,0 +1,16 @@ +{% if user in extension.authors.all and user.teams.count > 0 %} +
+
+ {# django won't allow submitting an empty field for a required field, so using a hack with an explicit required=True #} + {% include "common/components/field.html" with field=extension_form.team label="Assign Team" required=True %} +
+
+{% endif %} + +
+{% include "common/components/field.html" with field=extension_form.description label="Description" placeholder="Describe the extension..." %} +
+ +
+{% include "common/components/field.html" with field=extension_form.support placeholder="https://example.com" %} +
diff --git a/extensions/templates/extensions/draft_finalise.html b/extensions/templates/extensions/draft_finalise.html index 5e222a96..93d91ea3 100644 --- a/extensions/templates/extensions/draft_finalise.html +++ b/extensions/templates/extensions/draft_finalise.html @@ -1,5 +1,5 @@ {% extends "common/base.html" %} -{% load i18n common pipeline %} +{% load common filters i18n pipeline %} {% block page_title %} {% with extension=extension_form.instance %} @@ -38,16 +38,7 @@
- {% for field in extension_form %} - {% if field != 'tags' %} - {# TODO: fix handling of tags #} -
-
- {% include "common/components/field.html" with placeholder="Enter the text here..." %} -
-
- {% endif %} - {% endfor %} + {% include "extensions/components/extension_form.html" with extension_form=extension_form %}
diff --git a/extensions/templates/extensions/manage/update.html b/extensions/templates/extensions/manage/update.html index 3cdf6f11..640ff645 100644 --- a/extensions/templates/extensions/manage/update.html +++ b/extensions/templates/extensions/manage/update.html @@ -1,6 +1,5 @@ {% extends "common/base.html" %} -{% load filters %} -{% load i18n common pipeline %} +{% load common filters i18n pipeline %} {% block page_title %}{{ extension.name }}{% endblock page_title %} {% block content %} @@ -29,13 +28,7 @@ {{ form.errors }}
-
- {% include "common/components/field.html" with field=form.description label="Description" placeholder="Describe the extension..." %} -
- -
- {% include "common/components/field.html" with field=form.support placeholder="https://example.com" %} -
+ {% include "extensions/components/extension_form.html" with extension_form=form %}
diff --git a/extensions/tests/test_update.py b/extensions/tests/test_update.py index a09ef431..f9065def 100644 --- a/extensions/tests/test_update.py +++ b/extensions/tests/test_update.py @@ -5,10 +5,13 @@ from django.test import TestCase from common.tests.factories.extensions import create_approved_version, create_version from common.tests.factories.files import FileFactory, ImageFactory +from common.tests.factories.teams import TeamFactory +from common.tests.factories.users import UserFactory from common.tests.utils import _get_all_form_errors, CheckFilePropertiesMixin from extensions.models import Extension from files.models import File from reviewers.models import ApprovalActivity +from teams.models import TeamsUsers TEST_FILES_DIR = Path(__file__).resolve().parent / 'files' POST_DATA = { @@ -499,3 +502,128 @@ class UpdateTest(CheckFilePropertiesMixin, TestCase): response3 = self.client.get(url) self.assertEqual(response3.status_code, 302) self.assertEqual(response3['Location'], extension.get_draft_url()) + + def test_team_field_in_draft_form(self): + version = create_version( + extension__status=Extension.STATUSES.DRAFT, + ) + extension = version.extension + author = extension.authors.first() + self.client.force_login(author) + + team = TeamFactory(slug='test-team') + TeamsUsers(team=team, user=author).save() + + url = extension.get_draft_url() + response = self.client.get(url) + # a simple check that we have an input with the team option available + self.assertContains(response, 'value="test-team"') + + # post the form to save the team field + response = self.client.post( + url, + { + **POST_DATA, + 'team': 'test-team', + 'save_draft': '', + }, + ) + self.assertEqual(response.status_code, 302, _get_all_form_errors(response)) + extension.refresh_from_db() + self.assertEqual(extension.team.slug, 'test-team') + + # can't assign an invalid team slug + response = self.client.post( + url, + { + **POST_DATA, + 'team': '-', + 'save_draft': '', + }, + ) + self.assertEqual(response.status_code, 200, _get_all_form_errors(response)) + + # add another team member, they shouldn't see the field + user = UserFactory() + team2 = TeamFactory(slug='test-team2') + TeamsUsers(team=team, user=user).save() + TeamsUsers(team=team2, user=user).save() + self.client.force_login(user) + response = self.client.get(url) + self.assertNotContains(response, 'value="test-team"') + + response = self.client.post( + url, + { + **POST_DATA, + 'team': 'test-team2', + 'save_draft': '', + }, + ) + # the field is ignored: no error expected and the team wasn't updated + self.assertEqual(response.status_code, 302, _get_all_form_errors(response)) + extension.refresh_from_db() + self.assertEqual(extension.team.slug, 'test-team') + + def test_team_field_in_update_form(self): + """This test is a copy-paste of the one above, only status, url and form data differ.""" + version = create_version( + extension__status=Extension.STATUSES.APPROVED, + ) + extension = version.extension + author = extension.authors.first() + self.client.force_login(author) + + team = TeamFactory(slug='test-team') + TeamsUsers(team=team, user=author).save() + + url = extension.get_manage_url() + response = self.client.get(url) + # a simple check that we have an input with the team option available + self.assertContains(response, 'value="test-team"') + + # post the form to save the team field + response = self.client.post( + url, + { + **POST_DATA, + 'team': 'test-team', + 'save': '', + }, + ) + self.assertEqual(response.status_code, 302, _get_all_form_errors(response)) + extension.refresh_from_db() + self.assertEqual(extension.team.slug, 'test-team') + + # can't assign an invalid team slug + response = self.client.post( + url, + { + **POST_DATA, + 'team': '-', + 'save': '', + }, + ) + self.assertEqual(response.status_code, 200, _get_all_form_errors(response)) + + # add another team member, they shouldn't see the field + user = UserFactory() + team2 = TeamFactory(slug='test-team2') + TeamsUsers(team=team, user=user).save() + TeamsUsers(team=team2, user=user).save() + self.client.force_login(user) + response = self.client.get(url) + self.assertNotContains(response, 'value="test-team"') + + response = self.client.post( + url, + { + **POST_DATA, + 'team': 'test-team2', + 'save': '', + }, + ) + # the field is ignored: no error expected and the team wasn't updated + self.assertEqual(response.status_code, 302, _get_all_form_errors(response)) + extension.refresh_from_db() + self.assertEqual(extension.team.slug, 'test-team') diff --git a/extensions/tests/test_views.py b/extensions/tests/test_views.py index d47e6ccb..fc90b3f7 100644 --- a/extensions/tests/test_views.py +++ b/extensions/tests/test_views.py @@ -4,10 +4,11 @@ from django.test import TestCase from django.urls import reverse from common.tests.factories.extensions import create_version, create_approved_version +from common.tests.factories.teams import TeamFactory from common.tests.factories.users import UserFactory from extensions.models import Extension, Version from files.models import File -from teams.models import Team +from teams.models import Team, TeamsUsers def _create_extension(): @@ -190,7 +191,7 @@ class ExtensionDetailViewTest(_BaseTestCase): self._check_detail_page(response, extension) - def test_can_view_unlisted_extension_if_maintaner(self): + def test_can_view_unlisted_extension_if_maintainer(self): extension = _create_extension() self.client.force_login(extension.authors.first()) @@ -198,6 +199,20 @@ class ExtensionDetailViewTest(_BaseTestCase): self._check_detail_page(response, extension) + def test_can_view_unlisted_extension_if_team_member(self): + extension = _create_extension() + + team = TeamFactory(slug='test-team') + user = UserFactory() + TeamsUsers(team=team, user=user).save() + extension.team = team + extension.save() + + self.client.force_login(user) + response = self.client.get(extension.get_manage_url()) + + self._check_detail_page(response, extension) + def test_can_view_publicly_listed_extension_anonymously(self): extension = _create_extension() extension.approve() @@ -245,7 +260,7 @@ class ExtensionManageViewTest(_BaseTestCase): self.assertEqual(response.status_code, 302) - def test_can_view_manage_extension_page_if_maintaner(self): + def test_can_view_manage_extension_page_if_maintainer(self): extension = _create_extension() extension.approve() @@ -254,6 +269,20 @@ class ExtensionManageViewTest(_BaseTestCase): self._check_manage_page(response, extension) + def test_can_view_manage_extension_page_if_team_member(self): + extension = _create_extension() + extension.approve() + team = TeamFactory(slug='test-team') + user = UserFactory() + TeamsUsers(team=team, user=user).save() + extension.team = team + extension.save() + + self.client.force_login(user) + response = self.client.get(extension.get_manage_url()) + + self._check_manage_page(response, extension) + class ListedExtensionsTest(_BaseTestCase): def setUp(self): @@ -354,3 +383,17 @@ class UpdateVersionViewTest(_BaseTestCase): self.assertEqual(response2.status_code, 302) version.refresh_from_db() self.assertEqual(version.blender_version_max, '4.2.0') + + +class MyExtensionsTest(_BaseTestCase): + def test_team_members_see_extensions_in_my_extensions(self): + extension = _create_extension() + team = TeamFactory(slug='test-team') + user = UserFactory() + TeamsUsers(team=team, user=user).save() + extension.team = team + extension.save() + + self.client.force_login(user) + response = self.client.get(reverse('extensions:manage-list')) + self.assertContains(response, extension.name) diff --git a/extensions/views/manage.py b/extensions/views/manage.py index 7994a2e6..47f532c9 100644 --- a/extensions/views/manage.py +++ b/extensions/views/manage.py @@ -99,7 +99,16 @@ class ManageListView(LoginRequiredMixin, ListView): template_name = 'extensions/manage/list.html' def get_queryset(self): - return Extension.objects.authored_by(user_id=self.request.user.pk) + return Extension.objects.authored_by(self.request.user).prefetch_related( + 'authors', + 'preview_set', + 'preview_set__file', + 'ratings', + 'team', + 'versions', + 'versions__file', + 'versions__tags', + ) class UpdateExtensionView( diff --git a/extensions/views/mixins.py b/extensions/views/mixins.py index 568f26ae..dfb9cafe 100644 --- a/extensions/views/mixins.py +++ b/extensions/views/mixins.py @@ -23,7 +23,7 @@ class ExtensionQuerysetMixin: if self.request.user.is_staff: return Extension.objects.all() if self.request.user.is_authenticated: - return Extension.objects.listed_or_authored_by(user_id=self.request.user.pk) + return Extension.objects.listed_or_authored_by(self.request.user) return Extension.objects.listed @@ -32,7 +32,7 @@ class MaintainedExtensionMixin: def dispatch(self, *args, **kwargs): self.extension = get_object_or_404( - Extension.objects.authored_by(user_id=self.request.user.pk), + Extension.objects.authored_by(self.request.user), slug=self.kwargs['slug'], ) return super().dispatch(*args, **kwargs) diff --git a/extensions/views/public.py b/extensions/views/public.py index 32e65de7..3ec54025 100644 --- a/extensions/views/public.py +++ b/extensions/views/public.py @@ -50,6 +50,7 @@ class HomeView(ListedExtensionsView): 'preview_set', 'preview_set__file', 'ratings', + 'team', 'versions', 'versions__file', 'versions__tags', @@ -107,6 +108,7 @@ class SearchView(ListedExtensionsView): 'preview_set', 'preview_set__file', 'ratings', + 'team', 'versions', 'versions__file', 'versions__tags', diff --git a/extensions/views/submit.py b/extensions/views/submit.py index f78132c9..648c0f10 100644 --- a/extensions/views/submit.py +++ b/extensions/views/submit.py @@ -18,7 +18,7 @@ class UploadFileView(LoginRequiredMixin, CreateView): def get_context_data(self, **kwargs): context = super().get_context_data(**kwargs) - drafts = Extension.objects.authored_by(user_id=self.request.user.pk).filter( + drafts = Extension.objects.authored_by(self.request.user).filter( status=Extension.STATUSES.DRAFT ) context['drafts'] = drafts @@ -41,7 +41,7 @@ class UploadFileView(LoginRequiredMixin, CreateView): if parsed_extension_fields: # Try to look up extension by the same author and file info extension = ( - Extension.objects.authored_by(user_id=self.request.user.pk) + Extension.objects.authored_by(self.request.user) .filter(type=self.file.type, **parsed_extension_fields) .first() ) diff --git a/teams/models.py b/teams/models.py index eb3f3e2b..50764073 100644 --- a/teams/models.py +++ b/teams/models.py @@ -1,7 +1,7 @@ import logging from django.contrib.auth import get_user_model -from django.db import models +from django.db import models, transaction from django.urls import reverse from common.model_mixins import CreatedModifiedMixin @@ -49,3 +49,39 @@ class TeamsUsers(CreatedModifiedMixin, models.Model): @property def is_manager(self) -> bool: return self.role == TEAM_ROLE_MANAGER + + @transaction.atomic + def delete(self): + # This runs when a user is leaving a team. + # If the user had authored an extension, other team members shouldn't have access to it, + # unless the extension has another maintainer who is still on that team. + for extension in self.user.extensions.filter(team=self.team).all(): + # assuming small datasets, not optimizing db access + authors = extension.authors.all() + has_other_authors_from_the_team = False + for author in authors: + if author.pk == self.user.pk: + continue + if self.team in author.teams.all(): + has_other_authors_from_the_team = True + break + if not has_other_authors_from_the_team: + extension.team = None + extension.save(update_fields={'team'}) + + return super().delete() + + @property + def may_leave(self) -> bool: + nr_of_managers = TeamsUsers.objects.filter(role=TEAM_ROLE_MANAGER, team=self.team).count() + user_is_manager = ( + TeamsUsers.objects.filter( + role=TEAM_ROLE_MANAGER, + team=self.team, + user=self.user, + ).first() + is not None + ) + if user_is_manager and nr_of_managers < 2: + return False + return True diff --git a/teams/templates/teams/confirm_leave.html b/teams/templates/teams/confirm_leave.html new file mode 100644 index 00000000..e8d425fd --- /dev/null +++ b/teams/templates/teams/confirm_leave.html @@ -0,0 +1,49 @@ +{% extends "common/base.html" %} +{% load i18n %} +{% block content %} +
+
+
+

+ {% blocktranslate with team_name=object.name %}Leave team {{ team_name }}?{% endblocktranslate %} +

+ {% if may_leave %} +

+ {% blocktranslate %} + If you wish to join the team again in the future, you will need to ask the team manager to add you back. + {% endblocktranslate %} + + {% if will_lose_access_to %} +
+ {% blocktranslate %} + You will lose access to all team extensions that were not uploaded by you: + {% endblocktranslate %} +

    + {% for extension in will_lose_access_to %} +
  • {{ extension }}
    • + {% endfor %} +
+ {% endif %} +

+
+ + + {% trans 'Cancel' %} + +
+ {% csrf_token %} + +
+
+ {% else %} +

+ {% trans 'You cannot leave this team because you are the only manager.' %} +

+ {% endif %} +
+
+
+{% endblock content %} diff --git a/teams/templates/teams/team_list.html b/teams/templates/teams/team_list.html index 5c80005b..2fa22883 100644 --- a/teams/templates/teams/team_list.html +++ b/teams/templates/teams/team_list.html @@ -4,34 +4,63 @@

Teams

- - - - - - - - - {% for team_member in user.team_users.all %} - {% with team=team_member.team %} - - - - - {% endwith %} - {% endfor %} - -
- Team name - - Role -
- {{ team.name }} - -
- {{ team_member.get_role_display }} -
-
+ {% if team_memberships %} + + + + + + + + + + + {% for team_member in team_memberships %} + {% with team=team_member.team %} + + + + + + + {% endwith %} + {% endfor %} + +
+ Team name + + Role + + Users +
+ {{ team.name }} + +
+ {{ team_member.get_role_display }} +
+ 		+
+ {{ team.team_users.all.count }} +
+ 		+
+ + +
+
+ {% else %} +

+ You are not assigned to any teams yet. +

+ {% endif %}
{% endblock settings %} diff --git a/teams/tests/test_leave.py b/teams/tests/test_leave.py new file mode 100644 index 00000000..949ea4c7 --- /dev/null +++ b/teams/tests/test_leave.py @@ -0,0 +1,52 @@ +from django.test import TestCase +from django.urls import reverse + + +from common.tests.factories.extensions import create_version +from common.tests.factories.teams import TeamFactory +from common.tests.factories.users import UserFactory +from constants.base import TEAM_ROLE_MANAGER, TEAM_ROLE_MEMBER +from teams.models import TeamsUsers + + +class TeamLeaveTest(TestCase): + def test_the_only_manager_cant_leave(self): + team = TeamFactory(slug='test-team') + user = UserFactory() + TeamsUsers(team=team, user=user, role=TEAM_ROLE_MANAGER).save() + self.assertEqual(user.teams.count(), 1) + + self.client.force_login(user) + response = self.client.get(reverse('teams:leave-team', args=[team.slug])) + self.assertContains(response, 'cannot leave') + self.client.post(reverse('teams:leave-team', args=[team.slug])) + user.refresh_from_db() + self.assertEqual(user.teams.count(), 1) + + # create another manager + user2 = UserFactory() + TeamsUsers(team=team, user=user2, role=TEAM_ROLE_MANAGER).save() + # try to leave again + response = self.client.get(reverse('teams:leave-team', args=[team.slug])) + self.assertNotContains(response, 'cannot leave') + self.client.post(reverse('teams:leave-team', args=[team.slug])) + user.refresh_from_db() + self.assertEqual(user.teams.count(), 0) + + def test_extensions_lose_team_assignment(self): + team = TeamFactory(slug='test-team') + user = UserFactory() + TeamsUsers(team=team, user=user, role=TEAM_ROLE_MEMBER).save() + + extension = create_version().extension + extension.team = team + extension.authors.add(user) + extension.save() + + self.client.force_login(user) + self.client.post(reverse('teams:leave-team', args=[team.slug])) + user.refresh_from_db() + self.assertEqual(user.teams.count(), 0) + + extension.refresh_from_db() + self.assertIsNone(extension.team) diff --git a/teams/urls.py b/teams/urls.py index 98d883dc..3b0fada3 100644 --- a/teams/urls.py +++ b/teams/urls.py @@ -5,4 +5,9 @@ import teams.views app_name = 'teams' urlpatterns = [ path('settings/teams/', teams.views.TeamsView.as_view(), name='list'), + path( + 'settings/leave-team//', + teams.views.LeaveTeamView.as_view(), + name='leave-team', + ), ] diff --git a/teams/views.py b/teams/views.py index ef792c84..2bd3081a 100644 --- a/teams/views.py +++ b/teams/views.py @@ -1,12 +1,43 @@ """Team pages.""" from django.contrib.auth.mixins import LoginRequiredMixin +from django.shortcuts import redirect from django.views.generic import ListView +from django.views.generic.detail import DetailView -import teams.models +from extensions.models import Extension +from teams.models import Team, TeamsUsers class TeamsView(LoginRequiredMixin, ListView): - model = teams.models.Team + model = Team - def get_queryset(self): - return self.request.user.teams.all() + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + context['team_memberships'] = ( + self.request.user.team_users.select_related('team').order_by('team__name').all() + ) + return context + + +class LeaveTeamView(LoginRequiredMixin, DetailView): + model = Team + template_name = 'teams/confirm_leave.html' + + def post(self, request, *args, **kwargs): + team = self.get_object() + team_user = TeamsUsers.objects.filter(team=team, user=self.request.user).first() + if team_user and team_user.may_leave: + team_user.delete() + return redirect('teams:list') + + def get_context_data(self, **kwargs): + team = self.get_object() + team_user = TeamsUsers.objects.filter(team=team, user=self.request.user).first() + context = super().get_context_data(**kwargs) + context['may_leave'] = team_user.may_leave + context['will_lose_access_to'] = list( + Extension.objects.authored_by(self.request.user).exclude( + maintainer__user_id=self.request.user.pk + ) + ) + return context diff --git a/users/templates/users/settings/tabs.html b/users/templates/users/settings/tabs.html index cea767b2..37278b97 100644 --- a/users/templates/users/settings/tabs.html +++ b/users/templates/users/settings/tabs.html @@ -2,9 +2,7 @@
{% include "common/components/nav_link.html" with name="users:my-profile" title="Profile" classes="i-home py-2" %} - {% if user.teams.count %} - {% include "common/components/nav_link.html" with name="teams:list" title="Teams" classes="i-users py-2" %} - {% endif %} + {% include "common/components/nav_link.html" with name="teams:list" title="Teams" classes="i-users py-2" %}