-
Couldn't load subscription status.
- Fork 239
Prevent inconsistent states beteween public channels and community ch… #5500
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
Prevent inconsistent states beteween public channels and community ch… #5500
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look good overall! Just found some little bugs with the tooltips on dropdown options, and a nitpick comment around the Python tests organization
| }, | ||
| isCommunityChannel() { | ||
| const status = this.channel.latest_community_library_submission_status; | ||
| return status === 'APPROVED' || status === 'LIVE'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly a nitpick, but it's usually a good idea to avoid the usage of "magic strings" and use constants instead, just in case anything changes, we would need to update just one place. Here we could use the CommunityLibraryStatus constant from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Alex, I forgot using CommunityLibraryStatus instead of magic strings!! I've fixed.
| </VListTile> | ||
| </div> | ||
| </template> | ||
| <span>This channel has been added to the Community Library and cannot be marked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, this tooltip is appearing in the other corner of the screen.
We are currently working on a "KDS to Studio" project, and we will eventually need to migrate this to use our KDropdownMenu, and that component does not provide support for tooltips on disabled options. So, to prevent that, I think another idea could be to show this message on the make public dialog here? And show the error message on the dialog and disable the submit button if the selected channel is in the community library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That make sense.
| <VTooltip | ||
| v-if="currentChannel.public" | ||
| bottom | ||
| attach="body" | ||
| lazy | ||
| > | ||
| <template #activator="{ on }"> | ||
| <div v-on="on"> | ||
| <VListTile disabled> | ||
| <VListTileTitle>{{ $tr('submitToCommunityLibrary') }}</VListTileTitle> | ||
| </VListTile> | ||
| </div> | ||
| </template> | ||
| <span>{{ $tr('publicChannelCannotSubmitToCommunityLibrary') }}</span> | ||
| </VTooltip> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we already prevented a public channel from being submitted to the community library by showing a warning message and disable the submit button in the create submission side panel
So, I think we can revert this change (also because this is showing the tooltip on the other corner of the screen for some reason).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move this test to the public app tests instead? Seems like a more meaningful location, and perhaps a name like test_public_models_mutual_exclusivity may express the intention of the file a bit better? Just some thoughts, as right now contentcuration/tests/test_mutual_exclusivity does not sound like anything related to community channels or public channels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I've removed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @taoerman! Code changes look good to me, and tests are working fine. Merging! :)
e4c43a2
into
learningequality:unstable
…annels
Summary
Adds backend validation to ensure channels cannot be both public (visible in Kolibri's library) and community channels (curated for Community Library) at the same time.
The implementation includes model validation, serializer validation, and API endpoint validation with clear error messages.
Comprehensive tests verify the mutual exclusivity rules work correctly across all layers.
Tests the new logic manually.
References
#5302
Reviewer guidance
run the unit tests