diff --git a/channels/factories.py b/channels/factories.py index c9c6302e85..77330d7d36 100644 --- a/channels/factories.py +++ b/channels/factories.py @@ -26,6 +26,7 @@ class ChannelFactory(DjangoModelFactory): name = factory.fuzzy.FuzzyText(length=21) title = factory.Faker("text", max_nb_chars=50) + published = True public_description = factory.Faker("text", max_nb_chars=50) channel_type = factory.fuzzy.FuzzyChoice(ChannelType.names()) @@ -116,6 +117,9 @@ class ChannelTopicDetailFactory(DjangoModelFactory): class Meta: model = ChannelTopicDetail + class Params: + is_unpublished = factory.Trait(channel__published=False) + class ChannelDepartmentDetailFactory(DjangoModelFactory): """Factory for a channels.models.ChannelDepartmentDetail object""" @@ -128,6 +132,9 @@ class ChannelDepartmentDetailFactory(DjangoModelFactory): class Meta: model = ChannelDepartmentDetail + class Params: + is_unpublished = factory.Trait(channel__published=False) + class ChannelUnitDetailFactory(DjangoModelFactory): """Factory for a channels.models.ChannelUnitDetail object""" @@ -138,6 +145,9 @@ class ChannelUnitDetailFactory(DjangoModelFactory): class Meta: model = ChannelUnitDetail + class Params: + is_unpublished = factory.Trait(channel__published=False) + class ChannelPathwayDetailFactory(DjangoModelFactory): """Factory for a channels.models.ChannelPathwayDetail object""" diff --git a/channels/migrations/0015_channel_published.py b/channels/migrations/0015_channel_published.py new file mode 100644 index 0000000000..55d43a1062 --- /dev/null +++ b/channels/migrations/0015_channel_published.py @@ -0,0 +1,17 @@ +# Generated by Django 4.2.14 on 2024-08-06 14:39 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("channels", "0014_dept_detail_related_name"), + ] + + operations = [ + migrations.AddField( + model_name="channel", + name="published", + field=models.BooleanField(db_index=True, default=True), + ), + ] diff --git a/channels/models.py b/channels/models.py index 75a20b2e09..06d76cf27e 100644 --- a/channels/models.py +++ b/channels/models.py @@ -5,7 +5,7 @@ from django.contrib.auth.models import Group from django.core.validators import RegexValidator from django.db import models -from django.db.models import JSONField, deletion +from django.db.models import Case, JSONField, When, deletion from django.db.models.functions import Concat from imagekit.models import ImageSpecField, ProcessedImageField from imagekit.processors import ResizeToFit @@ -32,12 +32,18 @@ class ChannelQuerySet(TimestampedModelQuerySet): def annotate_channel_url(self): """Annotate the channel for serialization""" return self.annotate( - channel_url=Concat( - models.Value(frontend_absolute_url("/c/")), - "channel_type", - models.Value("/"), - "name", - models.Value("/"), + channel_url=Case( + When( + published=True, + then=Concat( + models.Value(frontend_absolute_url("/c/")), + "channel_type", + models.Value("/"), + "name", + models.Value("/"), + ), + ), + default=None, ) ) @@ -95,6 +101,7 @@ class Channel(TimestampedModel): configuration = models.JSONField(null=True, default=dict, blank=True) search_filter = models.CharField(max_length=2048, blank=True, default="") public_description = models.TextField(blank=True, default="") + published = models.BooleanField(default=True, db_index=True) featured_list = models.ForeignKey( LearningResource, null=True, blank=True, on_delete=deletion.SET_NULL @@ -115,9 +122,11 @@ def __str__(self): return self.title @cached_property - def channel_url(self) -> str: + def channel_url(self) -> str | None: """Return the channel url""" - return frontend_absolute_url(f"/c/{self.channel_type}/{self.name}/") + if self.published: + return frontend_absolute_url(f"/c/{self.channel_type}/{self.name}/") + return None class Meta: unique_together = ("name", "channel_type") diff --git a/channels/models_test.py b/channels/models_test.py new file mode 100644 index 0000000000..a95b0a8f45 --- /dev/null +++ b/channels/models_test.py @@ -0,0 +1,39 @@ +from urllib.parse import urlparse + +import pytest + +from channels.constants import ChannelType +from channels.factories import ( + ChannelDepartmentDetailFactory, + ChannelTopicDetailFactory, + ChannelUnitDetailFactory, +) + +pytestmark = [pytest.mark.django_db] + + +@pytest.mark.parametrize("published", [True, False]) +@pytest.mark.parametrize( + ( + "channel_type", + "detail_factory", + ), + [ + (ChannelType.department, ChannelDepartmentDetailFactory), + (ChannelType.topic, ChannelTopicDetailFactory), + (ChannelType.unit, ChannelUnitDetailFactory), + ], +) +def test_channel_url_for_departments(published, channel_type, detail_factory): + """Test that the channel URL is correct for department channels""" + channel = detail_factory.create( + channel__published=published, + ).channel + + if published: + assert ( + urlparse(channel.channel_url).path + == f"/c/{channel_type.name}/{channel.name}/" + ) + else: + assert channel.channel_url is None diff --git a/channels/plugins.py b/channels/plugins.py index d971c451d2..9b27037005 100644 --- a/channels/plugins.py +++ b/channels/plugins.py @@ -12,6 +12,29 @@ ChannelTopicDetail, ChannelUnitDetail, ) +from learning_resources.models import LearningResource + + +def unpublish_topics_for_resource(resource): + """ + Unpublish channels for topics that are used exclusively by the resource + + Args: + resource(LearningResource): The resource that was unpublished + """ + other_published = LearningResource.objects.filter( + published=True, + ).exclude(id=resource.id) + + channels = Channel.objects.filter( + topic_detail__topic__in=resource.topics.all(), + channel_type=ChannelType.topic.name, # Redundant, but left for clarity + published=True, + ).exclude(topic_detail__topic__learningresource__in=other_published) + + for channel in channels: + channel.published = False + channel.save() class ChannelPlugin: @@ -140,3 +163,29 @@ def offeror_delete(self, offeror): """ Channel.objects.filter(unit_detail__unit=offeror).delete() offeror.delete() + + @hookimpl + def resource_upserted(self, resource, percolate): # noqa: ARG002 + """ + Publish channels for the resource's topics + """ + channels = Channel.objects.filter( + topic_detail__topic__in=resource.topics.all(), published=False + ) + for channel in channels: + channel.published = True + channel.save() + + @hookimpl + def resource_before_delete(self, resource): + """ + Unpublish channels for the resource's topics + """ + unpublish_topics_for_resource(resource) + + @hookimpl + def resource_unpublished(self, resource): + """ + Unpublish channels for the resource's topics + """ + unpublish_topics_for_resource(resource) diff --git a/channels/plugins_test.py b/channels/plugins_test.py index faa64c62dd..8163565474 100644 --- a/channels/plugins_test.py +++ b/channels/plugins_test.py @@ -3,11 +3,16 @@ import pytest from channels.constants import ChannelType -from channels.factories import ChannelDepartmentDetailFactory, ChannelFactory +from channels.factories import ( + ChannelDepartmentDetailFactory, + ChannelFactory, + ChannelTopicDetailFactory, +) from channels.models import Channel from channels.plugins import ChannelPlugin from learning_resources.factories import ( LearningResourceDepartmentFactory, + LearningResourceFactory, LearningResourceOfferorFactory, LearningResourceSchoolFactory, LearningResourceTopicFactory, @@ -140,3 +145,83 @@ def test_search_index_plugin_offeror_delete(): ChannelPlugin().offeror_delete(offeror) assert Channel.objects.filter(id=channel.id).exists() is False assert LearningResourceOfferor.objects.filter(code=offeror.code).exists() is False + + +@pytest.mark.parametrize("action", ["delete", "unpublish"]) +@pytest.mark.parametrize( + ("published_resources", "to_remove", "expect_channel_published"), + [ + (2, 0, True), # 2 published resources remain + (2, 1, True), # 1 published resources remain + (2, 2, False), # 0 published resource remains + ], +) +@pytest.mark.django_db() +def test_resource_before_delete_and_resource_unpublish( + action, published_resources, to_remove, expect_channel_published +): + """ + Test that topic channels are unpublished when they no longer have any resources + remaining. + """ + topic1 = LearningResourceTopicFactory.create() # for to-be-deleted resources + topic2 = LearningResourceTopicFactory.create() # for to-be-deleted & others + topic3 = LearningResourceTopicFactory.create() # for to-be-deleted resources + detail1 = ChannelTopicDetailFactory.create(topic=topic1) + detail2 = ChannelTopicDetailFactory.create(topic=topic2) + detail3 = ChannelTopicDetailFactory.create(topic=topic3) + channel1, channel2, channel3 = detail1.channel, detail2.channel, detail3.channel + + resources_in_play = LearningResourceFactory.create_batch( + published_resources, + topics=[topic1, topic2, topic3], + ) + + # Create extra published + unpublished resources to ensure topic2 sticks around + LearningResourceFactory.create(topics=[topic2]) # extra resources + + assert channel1.published + assert channel2.published + assert channel3.published + + for resource in resources_in_play[:to_remove]: + if action == "delete": + ChannelPlugin().resource_before_delete(resource) + resource.delete() + elif action == "unpublish": + resource.published = False + resource.save() + ChannelPlugin().resource_unpublished(resource) + else: + msg = ValueError(f"Invalid action {action}") + raise msg + + channel1.refresh_from_db() + channel2.refresh_from_db() + channel3.refresh_from_db() + assert channel1.published is expect_channel_published + assert channel2.published is True + assert channel3.published is expect_channel_published + + +@pytest.mark.django_db() +def test_resource_upserted(): + """ + Test that channels are published when a resource is created or updated + """ + channel1 = ChannelFactory.create(is_topic=True, published=False) + channel2 = ChannelFactory.create(is_topic=True, published=False) + channel3 = ChannelFactory.create(is_topic=True, published=False) + + resource = LearningResourceFactory.create( + topics=[channel1.topic_detail.topic, channel2.topic_detail.topic] + ) + ChannelPlugin().resource_upserted(resource, None) + + channel1.refresh_from_db() + channel2.refresh_from_db() + channel3.refresh_from_db() + + assert channel1.published is True + assert channel2.published is True + assert channel3.published is False diff --git a/channels/serializers.py b/channels/serializers.py index 2fd8c92037..cf5757cebe 100644 --- a/channels/serializers.py +++ b/channels/serializers.py @@ -157,7 +157,7 @@ def get_channel_url(self, instance) -> str: class Meta: model = Channel - exclude = [] + exclude = ["published"] class ChannelTopicDetailSerializer(serializers.ModelSerializer): diff --git a/channels/views.py b/channels/views.py index 92d6a32a0c..18df82d3a1 100644 --- a/channels/views.py +++ b/channels/views.py @@ -88,7 +88,8 @@ def get_serializer_context(self): def get_queryset(self): """Return a queryset""" return ( - Channel.objects.prefetch_related( + Channel.objects.filter(published=True) + .prefetch_related( Prefetch( "lists", queryset=ChannelList.objects.prefetch_related( @@ -144,6 +145,7 @@ def get_object(self): Channel, channel_type=self.kwargs["channel_type"], name=self.kwargs["name"], + published=True, ) diff --git a/channels/views_test.py b/channels/views_test.py index c7b0851b9d..4a60bd9922 100644 --- a/channels/views_test.py +++ b/channels/views_test.py @@ -28,11 +28,14 @@ def test_list_channels(user_client): """Test that all channels are returned""" - channels = sorted(ChannelFactory.create_batch(10), key=lambda f: f.id) + ChannelFactory.create_batch(2, published=False) # should be filtered out + channels = sorted(ChannelFactory.create_batch(3), key=lambda f: f.id) + ChannelFactory.create_batch(2, published=False) # should be filtered out + url = reverse("channels:v0:channels_api-list") channel_list = sorted(user_client.get(url).json()["results"], key=lambda f: f["id"]) - assert len(channel_list) == 10 - for idx, channel in enumerate(channels[:10]): + assert len(channel_list) == 3 + for idx, channel in enumerate(channels[:3]): assert channel_list[idx] == ChannelSerializer(instance=channel).data @@ -206,20 +209,26 @@ def test_patch_channel_image(client, channel, attribute): assert len(size_image.read()) > 0 -def test_channel_by_type_name_detail(user_client): +@pytest.mark.parametrize( + ("published", "requested_type", "response_status"), + [ + (True, ChannelType.topic, 200), + (False, ChannelType.topic, 404), + (True, ChannelType.department, 404), + (False, ChannelType.department, 404), + ], +) +def test_channel_by_type_name_detail( + user_client, published, requested_type, response_status +): """ChannelByTypeNameDetailView should return expected result""" - channel = ChannelFactory.create(is_topic=True) + channel = ChannelFactory.create(is_topic=True, published=published) url = reverse( "channels:v0:channel_by_type_name_api-detail", - kwargs={"channel_type": ChannelType.topic.name, "name": channel.name}, - ) - response = user_client.get(url) - assert response.json() == ChannelSerializer(instance=channel).data - Channel.objects.filter(id=channel.id).update( - channel_type=ChannelType.department.name + kwargs={"channel_type": requested_type.name, "name": channel.name}, ) response = user_client.get(url) - assert response.status_code == 404 + assert response.status_code == response_status def test_update_channel_forbidden(channel, user_client): diff --git a/data_fixtures/migrations/0008_unpublish_empty_topic_channels.py b/data_fixtures/migrations/0008_unpublish_empty_topic_channels.py new file mode 100644 index 0000000000..67f0ea80c7 --- /dev/null +++ b/data_fixtures/migrations/0008_unpublish_empty_topic_channels.py @@ -0,0 +1,36 @@ +# Generated by Django 4.2.14 on 2024-08-01 20:31 + +from django.db import migrations + + +def unpublish_empty_topic_channels(apps, schema_editor): + """ + Set a topic channel to unpublished if it has no published learning resources. + """ + Channel = apps.get_model("channels", "Channel") + LearningResource = apps.get_model("learning_resources", "LearningResource") + + published_resources = LearningResource.objects.filter( + published=True, + ) + + channels = Channel.objects.filter(published=True, channel_type="topic").exclude( + topic_detail__topic__learningresource__in=published_resources + ) + + for channel in channels: + channel.published = False + channel.save() + + +class Migration(migrations.Migration): + dependencies = [ + ( + "data_fixtures", + "0007_topic_mappings_edx_add_programming_coding_to_computer_science", + ), + ] + + operations = [ + migrations.RunPython(unpublish_empty_topic_channels, migrations.RunPython.noop), + ] diff --git a/frontends/api/src/generated/v0/api.ts b/frontends/api/src/generated/v0/api.ts index 1b7392b9cd..1ef2105f74 100644 --- a/frontends/api/src/generated/v0/api.ts +++ b/frontends/api/src/generated/v0/api.ts @@ -1208,7 +1208,7 @@ export interface LearningResourceTopic { * @type {string} * @memberof LearningResourceTopic */ - channel_url: string + channel_url: string | null } /** * Serializer for News FeedItem diff --git a/frontends/api/src/generated/v1/api.ts b/frontends/api/src/generated/v1/api.ts index 3d10b40d30..e979df95b3 100644 --- a/frontends/api/src/generated/v1/api.ts +++ b/frontends/api/src/generated/v1/api.ts @@ -2334,7 +2334,7 @@ export interface LearningResourceTopic { * @type {string} * @memberof LearningResourceTopic */ - channel_url: string + channel_url: string | null } /** * SearchResponseSerializer with OpenAPI annotations for Learning Resources search diff --git a/frontends/mit-open/src/pages/TopicListingPage/TopicsListingPage.tsx b/frontends/mit-open/src/pages/TopicListingPage/TopicsListingPage.tsx index 3195c10087..69fda23a92 100644 --- a/frontends/mit-open/src/pages/TopicListingPage/TopicsListingPage.tsx +++ b/frontends/mit-open/src/pages/TopicListingPage/TopicsListingPage.tsx @@ -12,7 +12,7 @@ import { Breadcrumbs, } from "ol-components" import { Link } from "react-router-dom" -import { MetaTags } from "ol-utilities" +import { MetaTags, propsNotNil } from "ol-utilities" import { useLearningResourceTopics, @@ -204,7 +204,7 @@ const groupTopics = ( courseCounts: Record, programCounts: Record, ): TopicGroup[] => { - const sorted = topics.sort((a, b) => { + const sorted = topics.filter(propsNotNil(["channel_url"])).sort((a, b) => { return a.name.localeCompare(b.name) }) const groups: Record = Object.fromEntries( @@ -218,7 +218,7 @@ const groupTopics = ( courses: courseCounts[topic.name], programs: programCounts[topic.name], title: topic.name, - href: topic.channel_url || undefined, + href: topic.channel_url, }, ]), ) diff --git a/learning_resources/hooks.py b/learning_resources/hooks.py index 56865f798b..b588f3a05f 100644 --- a/learning_resources/hooks.py +++ b/learning_resources/hooks.py @@ -45,8 +45,8 @@ def bulk_resources_unpublished(self, resource_ids, resource_type): """Trigger actions after multiple learning resources are unpublished""" @hookspec - def resource_delete(self, resource): - """Trigger actions to remove a learning resource""" + def resource_before_delete(self, resource): + """Trigger actions before removing a learning resource""" @hookspec def resource_run_upserted(self, run): diff --git a/learning_resources/serializers.py b/learning_resources/serializers.py index 8f447729a0..0e7c2c9875 100644 --- a/learning_resources/serializers.py +++ b/learning_resources/serializers.py @@ -40,7 +40,7 @@ class LearningResourceTopicSerializer(serializers.ModelSerializer): Serializer for LearningResourceTopic model """ - channel_url = serializers.CharField(read_only=True) + channel_url = serializers.CharField(read_only=True, allow_null=True) class Meta: """Meta options for the serializer.""" diff --git a/learning_resources/utils.py b/learning_resources/utils.py index 4a697a3eab..8f99d45055 100644 --- a/learning_resources/utils.py +++ b/learning_resources/utils.py @@ -376,7 +376,8 @@ def resource_delete_actions(resource: LearningResource): """ pm = get_plugin_manager() hook = pm.hook - hook.resource_delete(resource=resource) + hook.resource_before_delete(resource=resource) + resource.delete() def bulk_resources_unpublished_actions(resource_ids: list[int], resource_type: str): diff --git a/learning_resources/utils_test.py b/learning_resources/utils_test.py index 34c748aad4..692b3b4be2 100644 --- a/learning_resources/utils_test.py +++ b/learning_resources/utils_test.py @@ -233,7 +233,11 @@ def test_resource_delete_actions(mock_plugin_manager, fixture_resource): resource_delete_actions function should trigger plugin hook's resource_deleted function """ utils.resource_delete_actions(fixture_resource) - mock_plugin_manager.hook.resource_delete.assert_called_once_with( + + with pytest.raises(LearningResource.DoesNotExist): + fixture_resource.refresh_from_db() + + mock_plugin_manager.hook.resource_before_delete.assert_called_once_with( resource=fixture_resource ) diff --git a/learning_resources/views_test.py b/learning_resources/views_test.py index 9c17a9848e..32a98768f1 100644 --- a/learning_resources/views_test.py +++ b/learning_resources/views_test.py @@ -8,7 +8,7 @@ from django.utils import timezone from rest_framework.reverse import reverse -from channels.factories import ChannelUnitDetailFactory +from channels.factories import ChannelTopicDetailFactory, ChannelUnitDetailFactory from channels.models import Channel from learning_resources.constants import ( LearningResourceRelationTypes, @@ -598,6 +598,26 @@ def test_topics_detail_endpoint(client): assert resp.data == LearningResourceTopicSerializer(instance=topic).data +@pytest.mark.parametrize("published", [True, False]) +def test_topic_channel_url(client, published): + """ + Check that the topic API returns 'None' for channel_url of unpublished channels. + + Note: The channel_url being None is also tested on the Channel model itself, + but the API may generate the channel_url in a slightly different manner (for + example, queryset annotation) + """ + topic = LearningResourceTopicFactory.create() + channel = ChannelTopicDetailFactory.create( + topic=topic, is_unpublished=not published + ).channel + resp = client.get(reverse("lr:v1:topics_api-detail", args=[topic.pk])) + + assert resp.data["channel_url"] == channel.channel_url + if not published: + assert resp.data["channel_url"] is None + + def test_departments_list_endpoint(client): """Test departments list endpoint""" departments = sorted( diff --git a/learning_resources_search/plugins.py b/learning_resources_search/plugins.py index 0312accdd1..e8980d2fbf 100644 --- a/learning_resources_search/plugins.py +++ b/learning_resources_search/plugins.py @@ -140,12 +140,11 @@ def bulk_resources_unpublished(self, resource_ids, resource_type): ) @hookimpl - def resource_delete(self, resource): + def resource_before_delete(self, resource): """ Remove a resource from the search index and then delete the object """ self.resource_unpublished(resource) - resource.delete() @hookimpl def resource_run_upserted(self, run): diff --git a/learning_resources_search/plugins_test.py b/learning_resources_search/plugins_test.py index 4a0d50ed72..70ab7f8879 100644 --- a/learning_resources_search/plugins_test.py +++ b/learning_resources_search/plugins_test.py @@ -8,7 +8,7 @@ LearningResourceFactory, LearningResourceRunFactory, ) -from learning_resources.models import LearningResource, LearningResourceRun +from learning_resources.models import LearningResourceRun from learning_resources_search.constants import COURSE_TYPE, PROGRAM_TYPE from learning_resources_search.plugins import SearchIndexPlugin @@ -74,18 +74,18 @@ def test_search_index_plugin_resource_unpublished( @pytest.mark.django_db() @pytest.mark.parametrize("resource_type", [COURSE_TYPE, PROGRAM_TYPE]) -def test_search_index_plugin_resource_delete(mock_search_index_helpers, resource_type): +def test_search_index_plugin_resource_before_delete( + mock_search_index_helpers, resource_type +): """The plugin function should remove a resource from the search index then delete the resource""" resource = LearningResourceFactory.create(resource_type=resource_type) resource_id = resource.id - SearchIndexPlugin().resource_delete(resource) + SearchIndexPlugin().resource_before_delete(resource) mock_search_index_helpers.mock_remove_learning_resource.assert_called_once_with( resource_id, resource.resource_type ) - assert LearningResource.objects.filter(id=resource.id).exists() is False - @pytest.mark.django_db() def test_search_index_plugin_resource_run_upserted(mock_search_index_helpers): diff --git a/openapi/specs/v0.yaml b/openapi/specs/v0.yaml index 7d7f5c231b..eb574f037c 100644 --- a/openapi/specs/v0.yaml +++ b/openapi/specs/v0.yaml @@ -1611,6 +1611,7 @@ components: channel_url: type: string readOnly: true + nullable: true required: - channel_url - id diff --git a/openapi/specs/v1.yaml b/openapi/specs/v1.yaml index 24fdebfe14..dbc63e0a56 100644 --- a/openapi/specs/v1.yaml +++ b/openapi/specs/v1.yaml @@ -8770,6 +8770,7 @@ components: channel_url: type: string readOnly: true + nullable: true required: - channel_url - id