From 52759764a1a27f1921e9ddee89cd3e25df46a4b7 Mon Sep 17 00:00:00 2001 From: imperosol Date: Fri, 20 Feb 2026 18:46:37 +0100 Subject: [PATCH 1/4] feat: `User.all_groups` --- core/auth/mixins.py | 2 +- core/models.py | 29 +++++++++++++++++------------ core/tests/test_core.py | 8 ++++---- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/core/auth/mixins.py b/core/auth/mixins.py index 917200ed..b8d8ee10 100644 --- a/core/auth/mixins.py +++ b/core/auth/mixins.py @@ -308,5 +308,5 @@ class PermissionOrClubBoardRequiredMixin(PermissionRequiredMixin): if super().has_permission(): return True return self.club is not None and any( - g.id == self.club.board_group_id for g in self.request.user.cached_groups + g.id == self.club.board_group_id for g in self.request.user.all_groups ) diff --git a/core/models.py b/core/models.py index 27744775..a5ae84a3 100644 --- a/core/models.py +++ b/core/models.py @@ -356,23 +356,30 @@ class User(AbstractUser): ) if group_id is None: return False - if group_id == settings.SITH_GROUP_SUBSCRIBERS_ID: - return self.is_subscribed - if group_id == settings.SITH_GROUP_ROOT_ID: - return self.is_root - return any(g.id == group_id for g in self.cached_groups) + return any(g.id == group_id for g in self.all_groups) @cached_property - def cached_groups(self) -> list[Group]: + def all_groups(self) -> list[Group]: """Get the list of groups this user is in.""" - return list(self.groups.all()) + additional_groups = [] + if self.is_subscribed: + additional_groups.append(settings.SITH_GROUP_SUBSCRIBERS_ID) + if self.is_superuser: + additional_groups.append(settings.SITH_GROUP_ROOT_ID) + qs = self.groups.all() + if additional_groups: + # This is somewhat counter-intuitive, but this query runs way faster with + # a UNION rather than a OR (in average, 0.25ms vs 14ms). + # For the why, cf. https://dba.stackexchange.com/questions/293836/why-is-an-or-statement-slower-than-union + qs = qs.union(Group.objects.filter(id__in=additional_groups)) + return list(qs) @cached_property def is_root(self) -> bool: if self.is_superuser: return True root_id = settings.SITH_GROUP_ROOT_ID - return any(g.id == root_id for g in self.cached_groups) + return any(g.id == root_id for g in self.all_groups) @cached_property def is_board_member(self) -> bool: @@ -1099,9 +1106,7 @@ class PageQuerySet(models.QuerySet): return self.filter(view_groups=settings.SITH_GROUP_PUBLIC_ID) if user.has_perm("core.view_page"): return self.all() - groups_ids = [g.id for g in user.cached_groups] - if user.is_subscribed: - groups_ids.append(settings.SITH_GROUP_SUBSCRIBERS_ID) + groups_ids = [g.id for g in user.all_groups] return self.filter(view_groups__in=groups_ids) @@ -1376,7 +1381,7 @@ class PageRev(models.Model): return self.page.can_be_edited_by(user) def is_owned_by(self, user: User) -> bool: - return any(g.id == self.page.owner_group_id for g in user.cached_groups) + return any(g.id == self.page.owner_group_id for g in user.all_groups) def similarity_ratio(self, text: str) -> float: """Similarity ratio between this revision's content and the given text. diff --git a/core/tests/test_core.py b/core/tests/test_core.py index 631e5b51..f6dc8570 100644 --- a/core/tests/test_core.py +++ b/core/tests/test_core.py @@ -418,16 +418,16 @@ class TestUserIsInGroup(TestCase): group_in = baker.make(Group) self.public_user.groups.add(group_in) - # clear the cached property `User.cached_groups` - self.public_user.__dict__.pop("cached_groups", None) + # clear the cached property `User.all_groups` + self.public_user.__dict__.pop("all_groups", None) # Test when the user is in the group - with self.assertNumQueries(1): + with self.assertNumQueries(2): self.public_user.is_in_group(pk=group_in.id) with self.assertNumQueries(0): self.public_user.is_in_group(pk=group_in.id) group_not_in = baker.make(Group) - self.public_user.__dict__.pop("cached_groups", None) + self.public_user.__dict__.pop("all_groups", None) # Test when the user is not in the group with self.assertNumQueries(1): self.public_user.is_in_group(pk=group_not_in.id) From 84ed180c1e49efdd7d856ff89c5b361f12039c40 Mon Sep 17 00:00:00 2001 From: imperosol Date: Sat, 21 Feb 2026 18:33:55 +0100 Subject: [PATCH 2/4] refactor sas moderation view permission --- sas/static/bundled/sas/viewer-index.ts | 417 ++++++++++++------------- sas/templates/sas/picture.jinja | 29 +- sas/tests/test_views.py | 16 +- sas/views.py | 29 +- 4 files changed, 239 insertions(+), 252 deletions(-) diff --git a/sas/static/bundled/sas/viewer-index.ts b/sas/static/bundled/sas/viewer-index.ts index 8b07b0cf..69d29592 100644 --- a/sas/static/bundled/sas/viewer-index.ts +++ b/sas/static/bundled/sas/viewer-index.ts @@ -109,232 +109,225 @@ interface ViewerConfig { /** id of the first picture to load on the page */ firstPictureId: number; /** if the user is sas admin */ - userIsSasAdmin: boolean; + userCanModerate: boolean; } /** * Load user picture page with a nice download bar **/ -exportToHtml("loadViewer", (config: ViewerConfig) => { - document.addEventListener("alpine:init", () => { - Alpine.data("picture_viewer", () => ({ - /** - * All the pictures that can be displayed on this picture viewer - **/ - pictures: [] as PictureWithIdentifications[], - /** - * The currently displayed picture - * Default dummy data are pre-loaded to avoid javascript error - * when loading the page at the beginning - * @type PictureWithIdentifications - **/ - currentPicture: { - // biome-ignore lint/style/useNamingConvention: api is in snake_case - is_moderated: true, - id: null as number, - name: "", - // biome-ignore lint/style/useNamingConvention: api is in snake_case - display_name: "", - // biome-ignore lint/style/useNamingConvention: api is in snake_case - compressed_url: "", - // biome-ignore lint/style/useNamingConvention: api is in snake_case - profile_url: "", - // biome-ignore lint/style/useNamingConvention: api is in snake_case - full_size_url: "", - owner: "", - date: new Date(), - identifications: [] as IdentifiedUserSchema[], - }, - /** - * The picture which will be displayed next if the user press the "next" button - **/ - nextPicture: null as PictureWithIdentifications, - /** - * The picture which will be displayed next if the user press the "previous" button - **/ - previousPicture: null as PictureWithIdentifications, - /** - * The select2 component used to identify users - **/ - selector: undefined as UserAjaxSelect, - /** - * Error message when a moderation operation fails - **/ - moderationError: "", - /** - * Method of pushing new url to the browser history - * Used by popstate event and always reset to it's default value when used - **/ - pushstate: History.Push, +document.addEventListener("alpine:init", () => { + Alpine.data("picture_viewer", (config: ViewerConfig) => ({ + /** + * All the pictures that can be displayed on this picture viewer + **/ + pictures: [] as PictureWithIdentifications[], + /** + * The currently displayed picture + * Default dummy data are pre-loaded to avoid javascript error + * when loading the page at the beginning + * @type PictureWithIdentifications + **/ + currentPicture: { + // biome-ignore lint/style/useNamingConvention: api is in snake_case + is_moderated: true, + id: null as number, + name: "", + // biome-ignore lint/style/useNamingConvention: api is in snake_case + display_name: "", + // biome-ignore lint/style/useNamingConvention: api is in snake_case + compressed_url: "", + // biome-ignore lint/style/useNamingConvention: api is in snake_case + profile_url: "", + // biome-ignore lint/style/useNamingConvention: api is in snake_case + full_size_url: "", + owner: "", + date: new Date(), + identifications: [] as IdentifiedUserSchema[], + }, + /** + * The picture which will be displayed next if the user press the "next" button + **/ + nextPicture: null as PictureWithIdentifications, + /** + * The picture which will be displayed next if the user press the "previous" button + **/ + previousPicture: null as PictureWithIdentifications, + /** + * The select2 component used to identify users + **/ + selector: undefined as UserAjaxSelect, + /** + * Error message when a moderation operation fails + **/ + moderationError: "", + /** + * Method of pushing new url to the browser history + * Used by popstate event and always reset to it's default value when used + **/ + pushstate: History.Push, - async init() { - this.pictures = ( - await paginated(picturesFetchPictures, { - // biome-ignore lint/style/useNamingConvention: api is in snake_case - query: { album_id: config.albumId }, - } as PicturesFetchPicturesData) - ).map(PictureWithIdentifications.fromPicture); - this.selector = this.$refs.search; - this.selector.setFilter((users: UserProfileSchema[]) => { - const resp: UserProfileSchema[] = []; - const ids = [ - ...(this.currentPicture.identifications || []).map( - (i: IdentifiedUserSchema) => i.user.id, - ), - ]; - for (const user of users) { - if (!ids.includes(user.id)) { - resp.push(user); - } + async init() { + this.pictures = ( + await paginated(picturesFetchPictures, { + // biome-ignore lint/style/useNamingConvention: api is in snake_case + query: { album_id: config.albumId }, + } as PicturesFetchPicturesData) + ).map(PictureWithIdentifications.fromPicture); + this.selector = this.$refs.search; + this.selector.setFilter((users: UserProfileSchema[]) => { + const resp: UserProfileSchema[] = []; + const ids = [ + ...(this.currentPicture.identifications || []).map( + (i: IdentifiedUserSchema) => i.user.id, + ), + ]; + for (const user of users) { + if (!ids.includes(user.id)) { + resp.push(user); } - return resp; - }); - this.currentPicture = this.pictures.find( - (i: PictureSchema) => i.id === config.firstPictureId, - ); - this.$watch( - "currentPicture", - (current: PictureSchema, previous: PictureSchema) => { - if (current === previous) { - /* Avoid recursive updates */ - return; - } - this.updatePicture(); - }, - ); - window.addEventListener("popstate", async (event) => { - if (!event.state || event.state.sasPictureId === undefined) { + } + return resp; + }); + this.currentPicture = this.pictures.find( + (i: PictureSchema) => i.id === config.firstPictureId, + ); + this.$watch( + "currentPicture", + (current: PictureSchema, previous: PictureSchema) => { + if (current === previous) { + /* Avoid recursive updates */ return; } - this.pushstate = History.Replace; - this.currentPicture = this.pictures.find( - (i: PictureSchema) => - i.id === Number.parseInt(event.state.sasPictureId, 10), - ); - }); - this.pushstate = History.Replace; /* Avoid first url push */ - await this.updatePicture(); - }, - - /** - * Update the page. - * Called when the `currentPicture` property changes. - * - * The url is modified without reloading the page, - * and the previous picture, the next picture and - * the list of identified users are updated. - */ - async updatePicture(): Promise { - const updateArgs = { - data: { sasPictureId: this.currentPicture.id }, - unused: "", - url: this.currentPicture.sas_url, - }; - if (this.pushstate === History.Replace) { - window.history.replaceState( - updateArgs.data, - updateArgs.unused, - updateArgs.url, - ); - this.pushstate = History.Push; - } else { - window.history.pushState(updateArgs.data, updateArgs.unused, updateArgs.url); - } - - this.moderationError = ""; - const index: number = this.pictures.indexOf(this.currentPicture); - this.previousPicture = this.pictures[index - 1] || null; - this.nextPicture = this.pictures[index + 1] || null; - this.$refs.mainPicture?.addEventListener("load", () => { - // once the current picture is loaded, - // start preloading the next and previous pictures - this.nextPicture?.preload(); - this.previousPicture?.preload(); - }); - if (this.currentPicture.asked_for_removal && config.userIsSasAdmin) { - await Promise.all([ - this.currentPicture.loadIdentifications(), - this.currentPicture.loadModeration(), - ]); - } else { - await this.currentPicture.loadIdentifications(); - } - }, - - async moderatePicture() { - const res = await picturesModeratePicture({ - // biome-ignore lint/style/useNamingConvention: api is in snake_case - path: { picture_id: this.currentPicture.id }, - }); - if (res.error) { - this.moderationError = `${gettext("Couldn't moderate picture")} : ${(res.error as { detail: string }).detail}`; + this.updatePicture(); + }, + ); + window.addEventListener("popstate", async (event) => { + if (!event.state || event.state.sasPictureId === undefined) { return; } - this.currentPicture.is_moderated = true; - this.currentPicture.asked_for_removal = false; - }, + this.pushstate = History.Replace; + this.currentPicture = this.pictures.find( + (i: PictureSchema) => i.id === Number.parseInt(event.state.sasPictureId, 10), + ); + }); + this.pushstate = History.Replace; /* Avoid first url push */ + await this.updatePicture(); + }, - async deletePicture() { - const res = await picturesDeletePicture({ + /** + * Update the page. + * Called when the `currentPicture` property changes. + * + * The url is modified without reloading the page, + * and the previous picture, the next picture and + * the list of identified users are updated. + */ + async updatePicture(): Promise { + const updateArgs = { + data: { sasPictureId: this.currentPicture.id }, + unused: "", + url: this.currentPicture.sas_url, + }; + if (this.pushstate === History.Replace) { + window.history.replaceState(updateArgs.data, updateArgs.unused, updateArgs.url); + this.pushstate = History.Push; + } else { + window.history.pushState(updateArgs.data, updateArgs.unused, updateArgs.url); + } + + this.moderationError = ""; + const index: number = this.pictures.indexOf(this.currentPicture); + this.previousPicture = this.pictures[index - 1] || null; + this.nextPicture = this.pictures[index + 1] || null; + this.$refs.mainPicture?.addEventListener("load", () => { + // once the current picture is loaded, + // start preloading the next and previous pictures + this.nextPicture?.preload(); + this.previousPicture?.preload(); + }); + if (this.currentPicture.asked_for_removal && config.userCanModerate) { + await Promise.all([ + this.currentPicture.loadIdentifications(), + this.currentPicture.loadModeration(), + ]); + } else { + await this.currentPicture.loadIdentifications(); + } + }, + + async moderatePicture() { + const res = await picturesModeratePicture({ + // biome-ignore lint/style/useNamingConvention: api is in snake_case + path: { picture_id: this.currentPicture.id }, + }); + if (res.error) { + this.moderationError = `${gettext("Couldn't moderate picture")} : ${(res.error as { detail: string }).detail}`; + return; + } + this.currentPicture.is_moderated = true; + this.currentPicture.asked_for_removal = false; + }, + + async deletePicture() { + const res = await picturesDeletePicture({ + // biome-ignore lint/style/useNamingConvention: api is in snake_case + path: { picture_id: this.currentPicture.id }, + }); + if (res.error) { + this.moderationError = `${gettext("Couldn't delete picture")} : ${(res.error as { detail: string }).detail}`; + return; + } + this.pictures.splice(this.pictures.indexOf(this.currentPicture), 1); + if (this.pictures.length === 0) { + // The deleted picture was the only one in the list. + // As the album is now empty, go back to the parent page + document.location.href = config.albumUrl; + } + this.currentPicture = this.nextPicture || this.previousPicture; + }, + + /** + * Send the identification request and update the list of identified users. + */ + async submitIdentification(): Promise { + const widget: TomSelect = this.selector.widget; + await picturesIdentifyUsers({ + path: { // biome-ignore lint/style/useNamingConvention: api is in snake_case - path: { picture_id: this.currentPicture.id }, - }); - if (res.error) { - this.moderationError = `${gettext("Couldn't delete picture")} : ${(res.error as { detail: string }).detail}`; - return; - } - this.pictures.splice(this.pictures.indexOf(this.currentPicture), 1); - if (this.pictures.length === 0) { - // The deleted picture was the only one in the list. - // As the album is now empty, go back to the parent page - document.location.href = config.albumUrl; - } - this.currentPicture = this.nextPicture || this.previousPicture; - }, + picture_id: this.currentPicture.id, + }, + body: widget.items.map((i: string) => Number.parseInt(i, 10)), + }); + // refresh the identified users list + await this.currentPicture.loadIdentifications({ forceReload: true }); - /** - * Send the identification request and update the list of identified users. - */ - async submitIdentification(): Promise { - const widget: TomSelect = this.selector.widget; - await picturesIdentifyUsers({ - path: { - // biome-ignore lint/style/useNamingConvention: api is in snake_case - picture_id: this.currentPicture.id, - }, - body: widget.items.map((i: string) => Number.parseInt(i, 10)), - }); - // refresh the identified users list - await this.currentPicture.loadIdentifications({ forceReload: true }); + // Clear selection and cache of retrieved user so they can be filtered again + widget.clear(false); + widget.clearOptions(); + widget.setTextboxValue(""); + }, - // Clear selection and cache of retrieved user so they can be filtered again - widget.clear(false); - widget.clearOptions(); - widget.setTextboxValue(""); - }, + /** + * Check if an identification can be removed by the currently logged user + */ + canBeRemoved(identification: IdentifiedUserSchema): boolean { + return config.userCanModerate || identification.user.id === config.userId; + }, - /** - * Check if an identification can be removed by the currently logged user - */ - canBeRemoved(identification: IdentifiedUserSchema): boolean { - return config.userIsSasAdmin || identification.user.id === config.userId; - }, - - /** - * Untag a user from the current picture - */ - async removeIdentification(identification: IdentifiedUserSchema): Promise { - const res = await usersidentifiedDeleteRelation({ - // biome-ignore lint/style/useNamingConvention: api is in snake_case - path: { relation_id: identification.id }, - }); - if (!res.error && Array.isArray(this.currentPicture.identifications)) { - this.currentPicture.identifications = - this.currentPicture.identifications.filter( - (i: IdentifiedUserSchema) => i.id !== identification.id, - ); - } - }, - })); - }); + /** + * Untag a user from the current picture + */ + async removeIdentification(identification: IdentifiedUserSchema): Promise { + const res = await usersidentifiedDeleteRelation({ + // biome-ignore lint/style/useNamingConvention: api is in snake_case + path: { relation_id: identification.id }, + }); + if (!res.error && Array.isArray(this.currentPicture.identifications)) { + this.currentPicture.identifications = + this.currentPicture.identifications.filter( + (i: IdentifiedUserSchema) => i.id !== identification.id, + ); + } + }, + })); }); diff --git a/sas/templates/sas/picture.jinja b/sas/templates/sas/picture.jinja index b68312d5..b37f232d 100644 --- a/sas/templates/sas/picture.jinja +++ b/sas/templates/sas/picture.jinja @@ -17,10 +17,8 @@ {% from "sas/macros.jinja" import print_path %} -{% set user_is_sas_admin = user.is_root or user.is_in_group(pk = settings.SITH_GROUP_SAS_ADMIN_ID) %} - {% block content %} -
+
SAS / {{ print_path(album) }} @@ -50,15 +48,13 @@ It will be hidden to other users until it has been moderated. {% endtrans %}

- {% if user_is_sas_admin %} + {% if user.has_perm("sas.moderate_sasfile") %}