Handle deletion of a Channel with a related Community Library Submission #5551
Conversation
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Just a general concern regarding computing the has_community_library_submission for every channel load.
| def filter_deleted(self, queryset, name, value): | ||
| has_cl_filter = self.request.query_params.get( | ||
| "has_community_library_submission" | ||
| ) | ||
| if has_cl_filter and has_cl_filter.lower() == "true": | ||
| return queryset | ||
| return queryset.filter(deleted=value) |
There was a problem hiding this comment.
Oh, I think we can leave this responsibility to the frontend, so if we ever want to filter by deleted channels that have been submitted to the Community Library, we can just adjust the frontend code!
| # check if channel has any Community Library submissions | ||
| queryset = queryset.annotate( | ||
| has_community_library_submission=Exists( | ||
| CommunityLibrarySubmission.objects.filter(channel_id=OuterRef("id")) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
I am a bit skeptical about computing the has_community_library_submission here for every channel for a feature that may only appear in very few cases, such as removing a channel.
I think an alternative may be to create a RemoveChannelModal and load one Community Library submission for a given channel within that modal. With this, we will only load this data when needed, rather than on every single channel load. What do you think? @taoerman
…hub.com/taoerman/studio into issue-5459-Handle-deletion-of-a-Channel merge
| }; | ||
|
|
||
| if (params.has_community_library_submission === undefined) { | ||
| extendedParams.deleted = Boolean(params.deleted) && params.deleted.toString() === 'true'; |
There was a problem hiding this comment.
-
When filtering by Community Library:
- params.has_community_library_submission is present
- The if condition is false, so deleted is not set
- Backend doesn't filter by deleted → shows both deleted and non-deleted channels
-
When filtering by other types:
- params.has_community_library_submission is undefined
- The if condition is true, so deleted is set based on params.deleted
- Backend filters by deleted status as expected
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Just a couple of notes before merging this PR! 👐
| <VProgressCircular | ||
| indeterminate | ||
| size="24" | ||
| /> |
There was a problem hiding this comment.
Could we use the KCircularLoader component instead? As we are getting rid of the Vuetify components, its important to stop using them!
| import { defineComponent, ref, computed, watch, getCurrentInstance } from 'vue'; | ||
| import client from 'shared/client'; | ||
| export default defineComponent({ |
There was a problem hiding this comment.
Oh, we don't need here the defineComponent function, since we are working with component files, Vue automatically interprets that the default export of this file is the definition of the component.
| watch(dialog, newValue => { | ||
| if (newValue && props.canEdit) { | ||
| fetchCommunityLibrarySubmissionStatus(); | ||
| } else { | ||
| hasCommunityLibrarySubmission.value = false; | ||
| loading.value = false; | ||
| } | ||
| }); |
There was a problem hiding this comment.
I think that to prevent having to reset this data when the prop.value changes, a more common pattern we have across our products is to use the v-if directive rather than the v-model to mount and unmount these components. And expose a @close event to set the rendering variable to false.
So, if we call this component like this instead:
<RemoveChannelModal
v-if="deleteDialog"
:channel-id="channelId"
:can-edit="canEdit"
data-test="delete-modal"
@delete="handleDelete"
@close="deleteDialog = false"
/>Then we may only need to rely on the onMounted hook to fetch the data, and we wouldn't need to care about resetting the ref values, as they will always be defined when the modal is opened.
| const detailUrl = window.Urls.channel_detail(props.channelId); | ||
| const url = `${detailUrl.replace(/\/$/, '')}/has_community_library_submission`; |
There was a problem hiding this comment.
Here, we don't really need to invoke the channel_detail first and then concatenate the has_community_library_submission endpoint. What we can do instead is to abstract this call inside the Channel Resource and use the getUrlFunction instead, and pass the URL name that we defined in the viewset. Here is an example of how we do this for the language_exists action https://github.com/taoerman/studio/blob/ce0416e0eee2b01548811d40b514eb41d2905779/contentcuration/contentcuration/frontend/shared/data/resources.js#L1397
There was a problem hiding this comment.
Thanks Alex, very helpful!
| const $tr = messageId => { | ||
| return proxy.$tr(messageId); | ||
| }; |
There was a problem hiding this comment.
Here we don't need to define this $tr here! We already have a $tr function globally available in the template 👐
| > | ||
| {{ $tr('deleteChannelWithCLWarning') }} | ||
| </div> | ||
| {{ canEdit ? $tr('deletePrompt') : $tr('removePrompt') }} |
There was a problem hiding this comment.
So, if canEdit is false, we are not deleting the channel itself, just removing the user from the list of readers. In that case, I think it wouldn't make much sense to show the deleteChannelWithCLWarning warning either, because the channel won't be removed.
There was a problem hiding this comment.
Hi Alex, I thought my implement has matched your comment.
The warning only displays when both conditions are true:
When canEdit is false: the warning doesn't render (the v-if prevents it).
When canEdit is true: the warning only shows if the channel has a Community Library submission.
The data fetch also only runs when canEdit is true , avoiding unnecessary API calls.
There was a problem hiding this comment.
Oh, yes, mb didn't notice the v-if of the deleteChannelWithCLWarning node 😅. It is great, then!
| }; | ||
|
|
||
| if (params.has_community_library_submission === undefined) { | ||
| extendedParams.deleted = Boolean(params.deleted) && params.deleted.toString() === 'true'; |
AlexVelezLl
left a comment
AlexVelezLl
left a comment
There was a problem hiding this comment.
Code changes look good to me, and manual QA checks out. Thanks @taoerman!
2 participants
Summary
Shows a warning in delete confirmation modals when deleting a channel that has Community Library submissions, explaining that deletion in Studio does not remove it from the Community Library.
The backend exposes a has_community_library_submission boolean field for the frontend to detect these channels.
When admins filter by "Community Library" in the admin channels list, soft-deleted channels with Community Library submissions are now included, allowing admins to audit and manage them.
Includes backend tests for both features.
References
Fixed #5459
Reviewer guidance
Run unit tests Or test manually!