diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index aa8e2b1cf7..b8a7f4df70 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -62,7 +62,7 @@ jobs: cache: "poetry" - name: Validate lockfile - run: poetry lock --check + run: poetry check --lock - name: Install dependencies run: poetry install --no-interaction diff --git a/.github/workflows/production.yml b/.github/workflows/production.yml index 40a20793bf..19ef0f23b3 100644 --- a/.github/workflows/production.yml +++ b/.github/workflows/production.yml @@ -31,6 +31,9 @@ jobs: - name: Install frontend dependencies run: yarn install --immutable + - name: Set VERSION + run: echo "VERSION=$(./scripts/get_version.sh)" >> $GITHUB_ENV + - name: Build frontend run: NODE_ENV=production yarn build env: @@ -40,6 +43,8 @@ jobs: POSTHOG_TIMEOUT_MS: 1000 POSTHOG_PROJECT_ID: ${{ secrets.POSTHOG_PROJECT_ID_PROD }} POSTHOG_PROJECT_API_KEY: ${{ secrets.POSTHOG_PROJECT_API_KEY_PROD }} + SENTRY_DSN: ${{ secrets.SENTRY_DSN_PROD }} + SENTRY_ENV: ${{ secrets.MITOPEN_ENV_PROD }} MITOPEN_AXIOS_WITH_CREDENTIALS: true MITOPEN_API_BASE_URL: https://api.mitopen.odl.mit.edu MITOPEN_SUPPORT_EMAIL: mitopen-support@mit.edu diff --git a/.github/workflows/release-candidate.yml b/.github/workflows/release-candidate.yml index 923d3e8cbf..572c957d38 100644 --- a/.github/workflows/release-candidate.yml +++ b/.github/workflows/release-candidate.yml @@ -31,6 +31,9 @@ jobs: - name: Install frontend dependencies run: yarn install --immutable + - name: Set VERSION + run: echo "VERSION=$(./scripts/get_version.sh)" >> $GITHUB_ENV + - name: Build frontend run: NODE_ENV=production yarn build env: @@ -40,6 +43,8 @@ jobs: POSTHOG_TIMEOUT_MS: 1000 POSTHOG_PROJECT_ID: ${{ secrets.POSTHOG_PROJECT_ID_RC }} POSTHOG_PROJECT_API_KEY: ${{ secrets.POSTHOG_PROJECT_API_KEY_RC }} + SENTRY_DSN: ${{ secrets.SENTRY_DSN_RC }} + SENTRY_ENV: ${{ secrets.MITOPEN_ENV_RC }} MITOPEN_AXIOS_WITH_CREDENTIALS: true MITOPEN_API_BASE_URL: https://api.mitopen-rc.odl.mit.edu MITOPEN_SUPPORT_EMAIL: odl-mitopen-rc-support@mit.edu diff --git a/RELEASE.rst b/RELEASE.rst index 66f5c7305d..370ef5ef0b 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,26 @@ Release Notes ============= +Version 0.15.1 +-------------- + +- Update dependency redis to v5 (#1244) +- sort before comparing (#1372) +- Rename my 0008 to 0009 to prevent conflict (#1374) +- Add Manufacturing topic; update upserter to make adding child topics easier (#1364) +- Publish topic channels based on resource count (#1349) +- Update dependency opensearch-dsl to v2 (#1242) +- Update dependency opensearch-py to v2 (#1243) +- Fix issue with User List cards not updating on edit (#1361) +- Update dependency django-anymail to v11 (#1207) +- Update CI to check data migrations for conflicts (#1368) +- Fix frontend sentry configuration (#1362) +- Migrate config renovate.json (#1367) +- Update dependency sentry-sdk to v2 [SECURITY] (#1366) +- add a bullet about collecting demographics to PrivacyPage.tsx (#1355) +- user list UI updates (#1348) +- Subscription email template updates (#1311) + Version 0.15.0 (Released August 05, 2024) -------------- 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/data_fixtures/migrations/0009_topics_update_engineering_add_manufacturing.py b/data_fixtures/migrations/0009_topics_update_engineering_add_manufacturing.py new file mode 100644 index 0000000000..bb3e385db7 --- /dev/null +++ b/data_fixtures/migrations/0009_topics_update_engineering_add_manufacturing.py @@ -0,0 +1,59 @@ +""" +Update the topics; add Manufacturing under Engineering +This is intended to fix https://github.com/mitodl/hq/issues/5076 - no additional +mappings since the topics for Principals of Manufacturing include "Manufacturing" +so it should match on name directly. +""" + +from django.db import migrations + +from channels.constants import ChannelType +from learning_resources.utils import upsert_topic_data_string + +map_changes = """ +--- +topics: + - icon: + id: 4176e385-92c5-4e02-b3cc-4f34d9a4bf40 + mappings: [] + name: Manufacturing + children: [] + parent: 952604ab-ae23-45b3-a040-0e5f26fe42df +""" + + +def add_new_mapping(apps, schema_editor): + """Upsert the map_changes data above.""" + + upsert_topic_data_string(map_changes) + + +def rollback_new_mapping(apps, schema_editor): + """Remove the Manufacturing topic.""" + + LearningResourceTopic = apps.get_model( + "learning_resources", "LearningResourceTopic" + ) + Channel = apps.get_model("channels", "Channel") + + topic = LearningResourceTopic.objects.filter( + topic_uuid="4176e385-92c5-4e02-b3cc-4f34d9a4bf40", + ).get() + + Channel.objects.filter( + channel_type=ChannelType.topic.name, + topic_detail__topic=topic, + ).all().delete() + + topic.delete() + + +class Migration(migrations.Migration): + dependencies = [ + ( + "data_fixtures", + "0008_unpublish_empty_topic_channels", + ), + ] + + operations = [migrations.RunPython(add_new_mapping, rollback_new_mapping)] 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/api/src/hooks/learningResources/index.ts b/frontends/api/src/hooks/learningResources/index.ts index 13b930754a..e2deba4a0c 100644 --- a/frontends/api/src/hooks/learningResources/index.ts +++ b/frontends/api/src/hooks/learningResources/index.ts @@ -32,6 +32,7 @@ import type { import learningResources, { invalidateResourceQueries, invalidateUserListQueries, + invalidateResourceWithUserListQueries, updateListParentsOnAdd, updateListParentsOnDestroy, } from "./keyFactory" @@ -251,7 +252,10 @@ const useUserListUpdate = () => { PatchedUserListRequest: params, }), onSettled: (_data, _err, vars) => { - invalidateResourceQueries(queryClient, vars.id) + queryClient.invalidateQueries(learningResources.userlists._ctx.list._def) + queryClient.invalidateQueries( + learningResources.userlists._ctx.detail(vars.id).queryKey, + ) }, }) } @@ -262,7 +266,8 @@ const useUserListDestroy = () => { mutationFn: (params: ULDestroyRequest) => userListsApi.userlistsDestroy(params), onSettled: (_data, _err, vars) => { - invalidateResourceQueries(queryClient, vars.id) + invalidateUserListQueries(queryClient, vars.id) + invalidateResourceWithUserListQueries(queryClient, vars.id) }, }) } diff --git a/frontends/api/src/hooks/learningResources/keyFactory.ts b/frontends/api/src/hooks/learningResources/keyFactory.ts index 774eb17f3e..d2fcf3f262 100644 --- a/frontends/api/src/hooks/learningResources/keyFactory.ts +++ b/frontends/api/src/hooks/learningResources/keyFactory.ts @@ -155,9 +155,35 @@ const listHasResource = (page: PaginatedLearningResourceList) => page.results, ) : data.results + return resources.some((res) => res.id === resourceId) } +const resourcesHaveUserList = + (userListId: number) => + (query: Query): boolean => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const data = query.state.data as any + const resources: LearningResource[] = data.pages + ? data.pages.flatMap( + (page: PaginatedLearningResourceList) => page.results, + ) + : data.results + + return resources?.some((res) => + res.user_list_parents?.some((userList) => userList.parent === userListId), + ) + } + +const resourceHasUserList = + (userListId: number) => + (query: Query): boolean => { + const data = query.state.data as LearningResource + return !!data.user_list_parents?.some( + (userList) => userList.parent === userListId, + ) + } + /** * Invalidate Resource queries that a specific resource appears in. * @@ -202,6 +228,39 @@ const invalidateResourceQueries = ( }) } +/** + * Invalidate Resource queries that a resource that belongs to user list appears in. + */ +const invalidateResourceWithUserListQueries = ( + queryClient: QueryClient, + userListId: LearningResource["id"], +) => { + /** + * Invalidate resource detail query for resource that is in the user list. + */ + queryClient.invalidateQueries({ + queryKey: learningResources.detail._def, + predicate: resourceHasUserList(userListId), + }) + + /** + * Invalidate lists with a resource that is in the user list. + */ + const lists = [ + learningResources.list._def, + learningResources.learningpaths._ctx.list._def, + learningResources.search._def, + learningResources.featured._def, + ] + + lists.forEach((queryKey) => { + queryClient.invalidateQueries({ + queryKey, + predicate: resourcesHaveUserList(userListId), + }) + }) +} + const invalidateUserListQueries = ( queryClient: QueryClient, userListId: UserList["id"], @@ -210,6 +269,7 @@ const invalidateUserListQueries = ( learningResources.userlists._ctx.detail(userListId).queryKey, ) const lists = [learningResources.userlists._ctx.list._def] + lists.forEach((queryKey) => { queryClient.invalidateQueries({ queryKey, @@ -275,6 +335,7 @@ export default learningResources export { invalidateResourceQueries, invalidateUserListQueries, + invalidateResourceWithUserListQueries, updateListParentsOnAdd, updateListParentsOnDestroy, } diff --git a/frontends/mit-open/package.json b/frontends/mit-open/package.json index 5609eaf5e5..59112a5610 100644 --- a/frontends/mit-open/package.json +++ b/frontends/mit-open/package.json @@ -8,8 +8,8 @@ "url": "https://github.com/mitodl/mit-open.git" }, "scripts": { - "watch": "NODE_ENV=development ENVIRONMENT=local webpack serve", - "watch:docker": "NODE_ENV=development ENVIRONMENT=docker webpack serve", + "watch": "NODE_ENV=development LOAD_ENV_FILES=true webpack serve", + "watch:docker": "NODE_ENV=development webpack serve", "build": "webpack --config webpack.config.js --bail", "build-exports": "webpack --config webpack.exports.js --bail", "storybook": "storybook dev -p 6006", diff --git a/frontends/mit-open/public/images/mit-logo-transparent5.jpg b/frontends/mit-open/public/images/mit-logo-transparent5.jpg new file mode 100644 index 0000000000..fca614d62b Binary files /dev/null and b/frontends/mit-open/public/images/mit-logo-transparent5.jpg differ diff --git a/frontends/mit-open/public/images/mit-logo-transparent5.png b/frontends/mit-open/public/images/mit-logo-transparent5.png new file mode 100644 index 0000000000..91a9f6ab32 Binary files /dev/null and b/frontends/mit-open/public/images/mit-logo-transparent5.png differ diff --git a/frontends/mit-open/src/App.tsx b/frontends/mit-open/src/App.tsx index 828a739c2d..11d4f43e26 100644 --- a/frontends/mit-open/src/App.tsx +++ b/frontends/mit-open/src/App.tsx @@ -10,7 +10,7 @@ import AppProviders from "./AppProviders" Sentry.init({ dsn: APP_SETTINGS.SENTRY_DSN, release: APP_SETTINGS.VERSION, - environment: APP_SETTINGS.ENVIRONMENT, + environment: APP_SETTINGS.SENTRY_ENV, }) const container = document.getElementById("app-container") diff --git a/frontends/mit-open/src/common/urls.ts b/frontends/mit-open/src/common/urls.ts index 51eaaa4076..be80a3b73e 100644 --- a/frontends/mit-open/src/common/urls.ts +++ b/frontends/mit-open/src/common/urls.ts @@ -8,10 +8,6 @@ export const LEARNINGPATH_LISTING = "/learningpaths/" export const LEARNINGPATH_VIEW = "/learningpaths/:id" export const learningPathsView = (id: number) => generatePath(LEARNINGPATH_VIEW, { id: String(id) }) -export const USERLIST_LISTING = "/userlists/" -export const USERLIST_VIEW = "/userlists/:id" -export const userListView = (id: number) => - generatePath(USERLIST_VIEW, { id: String(id) }) export const PROGRAMLETTER_VIEW = "/program_letter/:id/view/" export const programLetterView = (id: string) => generatePath(PROGRAMLETTER_VIEW, { id: String(id) }) @@ -70,7 +66,16 @@ export const login = ({ return `${LOGIN}?next=${next}` } -export const DASHBOARD = "/dashboard/" +export const DASHBOARD_HOME = "/dashboard/" + +export const MY_LISTS = "/dashboard/my-lists/" +export const USERLIST_VIEW = "/dashboard/my-lists/:id" +export const userListView = (id: number) => + generatePath(USERLIST_VIEW, { id: String(id) }) + +export const PROFILE = "/dashboard/profile/" + +export const SETTINGS = "/dashboard/settings/" export const SEARCH = "/search/" diff --git a/frontends/mit-open/src/page-components/Header/UserMenu.tsx b/frontends/mit-open/src/page-components/Header/UserMenu.tsx index 2259371395..3c35c4395b 100644 --- a/frontends/mit-open/src/page-components/Header/UserMenu.tsx +++ b/frontends/mit-open/src/page-components/Header/UserMenu.tsx @@ -107,7 +107,7 @@ const UserMenu: React.FC = ({ variant }) => { label: "Dashboard", key: "dashboard", allow: !!user?.is_authenticated, - href: urls.DASHBOARD, + href: urls.DASHBOARD_HOME, }, { label: "Learning Paths", diff --git a/frontends/mit-open/src/page-components/ItemsListing/ItemsListingComponent.tsx b/frontends/mit-open/src/page-components/ItemsListing/ItemsListingComponent.tsx index b413343d01..13627b7bf0 100644 --- a/frontends/mit-open/src/page-components/ItemsListing/ItemsListingComponent.tsx +++ b/frontends/mit-open/src/page-components/ItemsListing/ItemsListingComponent.tsx @@ -1,20 +1,11 @@ import React from "react" -import { Grid, Button, Typography, styled } from "ol-components" -import { RiPencilFill, RiArrowUpDownLine } from "@remixicon/react" +import { Grid, Button, Typography, styled, Link } from "ol-components" +import { RiArrowLeftLine, RiArrowUpDownLine } from "@remixicon/react" import { useToggle, pluralize } from "ol-utilities" import { GridColumn, GridContainer } from "@/components/GridLayout/GridLayout" import ItemsListing from "./ItemsListing" import type { LearningResourceListItem } from "./ItemsListing" - -const Container = styled(GridContainer)` - margin-top: 30px; - margin-bottom: 100px; -` - -const TitleContainer = styled(Grid)` - margin-top: 10px; - margin-bottom: 20px; -` +import { MY_LISTS } from "@/common/urls" type OnEdit = () => void type ListData = { @@ -35,6 +26,48 @@ type ItemsListingComponentProps = { condensed?: boolean } +const HeaderText = styled.div(({ theme }) => ({ + h3: { + ...theme.typography.h3, + [theme.breakpoints.down("sm")]: { + marginBottom: "24px", + ...theme.typography.h5, + }, + }, +})) + +const DescriptionText = styled(Typography)(({ theme }) => ({ + color: theme.custom.colors.silverGrayDark, + ...theme.typography.body1, + [theme.breakpoints.down("sm")]: { + display: "none", + }, +})) + +const HeaderGrid = styled(Grid)(({ theme }) => ({ + gap: "24px", + [theme.breakpoints.down("sm")]: { + width: "100%", + }, +})) + +const EditButton = styled(Button)(({ theme }) => ({ + [theme.breakpoints.down("sm")]: { + width: "100%", + }, +})) + +const ReorderGrid = styled(Grid)(({ theme }) => ({ + [theme.breakpoints.down("sm")]: { + display: "none", + }, +})) + +const CountText = styled(Typography)(({ theme }) => ({ + color: theme.custom.colors.silverGrayDark, + ...theme.typography.buttonSmall, +})) + const ItemsListingComponent: React.FC = ({ listType, list, @@ -51,54 +84,71 @@ const ItemsListingComponent: React.FC = ({ const count = list?.item_count return ( - + - - - {list?.title} - - {list?.description &&

{list.description}

} - - {showSort && !!items.length && ( - - )} - {count !== undefined && count > 0 - ? `${count} ${pluralize("item", count)}` - : null} + + + + + - {canEdit ? ( - - ) : null} + + + {list?.title} + + {list?.description && ( + {list.description} + )} + + + {canEdit ? ( + + Edit List + + ) : null} + + + + {showSort && !!items.length && ( + + )} + + + {count !== undefined && count > 0 ? ( + {`${count} ${pluralize("item", count)}`} + ) : null} + + = ({ condensed={condensed} /> - + ) } diff --git a/frontends/mit-open/src/page-components/UserListCardTemplate/UserListCardTemplate.test.tsx b/frontends/mit-open/src/page-components/UserListCard/UserListCardCondensed.test.tsx similarity index 58% rename from frontends/mit-open/src/page-components/UserListCardTemplate/UserListCardTemplate.test.tsx rename to frontends/mit-open/src/page-components/UserListCard/UserListCardCondensed.test.tsx index 8af919565d..06168e2b31 100644 --- a/frontends/mit-open/src/page-components/UserListCardTemplate/UserListCardTemplate.test.tsx +++ b/frontends/mit-open/src/page-components/UserListCard/UserListCardCondensed.test.tsx @@ -1,20 +1,19 @@ import React from "react" -import { render, screen } from "@testing-library/react" -import UserListCardTemplate from "./UserListCardTemplate" +import { screen } from "@testing-library/react" +import UserListCardCondensed from "./UserListCardCondensed" import * as factories from "api/test-utils/factories" -import { makeImgConfig } from "ol-utilities/test-utils/factories" +import { userListView } from "@/common/urls" +import { renderWithProviders } from "@/test-utils" const userListFactory = factories.userLists describe("UserListCard", () => { it("renders title and cover image", () => { const userList = userListFactory.userList() - const imgConfig = makeImgConfig() - render( - , ) const heading = screen.getByRole("heading", { name: userList.title }) diff --git a/frontends/mit-open/src/page-components/UserListCard/UserListCardCondensed.tsx b/frontends/mit-open/src/page-components/UserListCard/UserListCardCondensed.tsx new file mode 100644 index 0000000000..9fa5debd43 --- /dev/null +++ b/frontends/mit-open/src/page-components/UserListCard/UserListCardCondensed.tsx @@ -0,0 +1,75 @@ +import React from "react" +import { UserList } from "api" +import { pluralize } from "ol-utilities" +import { RiListCheck3 } from "@remixicon/react" +import { ListCardCondensed, styled, theme, Typography } from "ol-components" + +const StyledCard = styled(ListCardCondensed)({ + display: "flex", + alignItems: "center", + padding: "16px", + margin: "0", + gap: "16px", + width: "100%", +}) + +const TextContainer = styled.div({ + display: "flex", + flexDirection: "column", + alignItems: "flex-start", + gap: "8px", + flex: "1 0 0", +}) + +const ItemsText = styled(Typography)(({ theme }) => ({ + color: theme.custom.colors.silverGrayDark, +})) + +const IconContainer = styled.div(({ theme }) => ({ + display: "flex", + alignItems: "center", + gap: "8px", + borderRadius: "4px", + color: theme.custom.colors.silverGrayDark, + background: theme.custom.colors.lightGray1, + [theme.breakpoints.down("sm")]: { + display: "none", + }, +})) + +type UserListCardCondensedProps = { + userList: U + href?: string + className?: string +} + +const UserListCardCondensed = ({ + userList, + href, + className, +}: UserListCardCondensedProps) => { + return ( + + + + + {userList.title} + + + {userList.item_count} {pluralize("item", userList.item_count)} + + + + + + + + ) +} + +export default UserListCardCondensed +export type { UserListCardCondensedProps } diff --git a/frontends/mit-open/src/page-components/UserListCardTemplate/UserListCardTemplate.tsx b/frontends/mit-open/src/page-components/UserListCardTemplate/UserListCardTemplate.tsx deleted file mode 100644 index 91da95becc..0000000000 --- a/frontends/mit-open/src/page-components/UserListCardTemplate/UserListCardTemplate.tsx +++ /dev/null @@ -1,69 +0,0 @@ -import React, { useCallback } from "react" -import CardTemplate from "../CardTemplate/CardTemplate" -import { UserList } from "api" -import { EmbedlyConfig, pluralize } from "ol-utilities" -import { styled } from "ol-components" - -const TypeRow = styled.div` - display: flex; - flex-direction: row; - justify-content: space-between; - align-items: center; - min-height: 1.5em; /* ensure consistent height even if no certificate */ -` - -type CardVariant = "column" | "row" | "row-reverse" -type OnActivateCard = (userList: UserList) => void -type UserListCardTemplateProps = { - /** - * Whether the course picture and info display as a column or row. - */ - variant: CardVariant - userList: U - sortable?: boolean - className?: string - imgConfig: EmbedlyConfig - onActivate?: OnActivateCard - footerActionSlot?: React.ReactNode -} - -const UserListCardTemplate = ({ - variant, - userList, - className, - imgConfig, - sortable, - onActivate, - footerActionSlot, -}: UserListCardTemplateProps) => { - const handleActivate = useCallback( - () => onActivate?.(userList), - [userList, onActivate], - ) - const extraDetails = ( - - {userList.description} - - ) - return ( - - {userList.item_count} {pluralize("item", userList.item_count)} - - } - footerActionSlot={footerActionSlot} - > - ) -} - -export default UserListCardTemplate -export type { UserListCardTemplateProps } diff --git a/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx b/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx index acc6a70d58..777aee60fb 100644 --- a/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx +++ b/frontends/mit-open/src/pages/DashboardPage/Dashboard.test.tsx @@ -7,7 +7,11 @@ import { } from "../../test-utils" import { factories, urls } from "api/test-utils" import { Permissions } from "@/common/permissions" -import { DashboardPage, DashboardTabLabels } from "./DashboardPage" +import { + DashboardPage, + DashboardTabKeys, + DashboardTabLabels, +} from "./DashboardPage" import { faker } from "@faker-js/faker/locale/en" import { CourseResource, @@ -16,6 +20,7 @@ import { } from "api" import { ControlledPromise } from "ol-test-utilities" import React from "react" +import { DASHBOARD_HOME, MY_LISTS, PROFILE } from "@/common/urls" describe("DashboardPage", () => { const makeSearchResponse = ( @@ -244,13 +249,15 @@ describe("DashboardPage", () => { test("Renders the expected tab links", async () => { setupAPIs() renderWithProviders() - Object.keys(DashboardTabLabels).forEach((key) => { + const urls = [DASHBOARD_HOME, MY_LISTS, PROFILE] + urls.forEach((url: string) => { + const key = DashboardTabKeys[url as keyof typeof DashboardTabKeys] const desktopTab = screen.getByTestId(`desktop-tab-${key}`) const mobileTab = screen.getByTestId(`mobile-tab-${key}`) expect(desktopTab).toBeInTheDocument() expect(mobileTab).toBeInTheDocument() - expect(desktopTab).toHaveAttribute("href", `/#${key}`) - expect(mobileTab).toHaveAttribute("href", `/#${key}`) + expect(desktopTab).toHaveAttribute("href", url) + expect(mobileTab).toHaveAttribute("href", url) }) }) diff --git a/frontends/mit-open/src/pages/DashboardPage/DashboardPage.tsx b/frontends/mit-open/src/pages/DashboardPage/DashboardPage.tsx index 002da268bd..930173ab59 100644 --- a/frontends/mit-open/src/pages/DashboardPage/DashboardPage.tsx +++ b/frontends/mit-open/src/pages/DashboardPage/DashboardPage.tsx @@ -1,4 +1,4 @@ -import React, { useCallback, useState } from "react" +import React from "react" import { RiAccountCircleFill, RiDashboardLine, @@ -23,9 +23,8 @@ import { import { MetaTags } from "ol-utilities" import { Link } from "react-router-dom" import { useUserMe } from "api/hooks/user" -import { useLocation } from "react-router" -import { UserListListingComponent } from "../UserListListingPage/UserListListingPage" -import { UserList } from "api" +import { useLocation, useParams } from "react-router" +import UserListListingComponent from "../UserListListingComponent/UserListListingComponent" import { ProfileEditForm } from "./ProfileEditForm" import { useProfileMeQuery } from "api/hooks/profile" @@ -40,6 +39,7 @@ import { import ResourceCarousel from "@/page-components/ResourceCarousel/ResourceCarousel" import UserListDetailsTab from "./UserListDetailsTab" import { SettingsPage } from "./SettingsPage" +import { DASHBOARD_HOME, MY_LISTS, PROFILE, SETTINGS } from "@/common/urls" /** * @@ -253,22 +253,37 @@ const StyledResourceCarousel = styled(ResourceCarousel)(({ theme }) => ({ }, })) +const TabKeys = { + [DASHBOARD_HOME]: "home", + [MY_LISTS]: "my-lists", + [PROFILE]: "profile", + [SETTINGS]: "settings", +} + +const TabLabels = { + [DASHBOARD_HOME]: "Home", + [MY_LISTS]: "My Lists", + [PROFILE]: "Profile", + [SETTINGS]: "Settings", +} + interface UserMenuTabProps { icon: React.ReactNode text: string + tabKey: string value: string currentValue: string onClick?: () => void } const UserMenuTab: React.FC = (props) => { - const { icon, text, value, currentValue, onClick } = props + const { icon, text, tabKey, value, currentValue, onClick } = props const selected = value === currentValue return ( = (props) => { ) } -enum TabValues { - HOME = "home", - MY_LISTS = "my-lists", - SETTINGS = "settings", - PROFILE = "profile", -} - -const TabLabels = { - [TabValues.HOME]: "Home", - [TabValues.MY_LISTS]: "My Lists", - [TabValues.PROFILE]: "Profile", - [TabValues.SETTINGS]: "Settings", -} -const keyFromHash = (hash: string) => { - const keys = Object.values(TabValues) - const match = keys.find((key) => `#${key}` === hash) - return match ?? "home" +type RouteParams = { + id: string } const DashboardPage: React.FC = () => { const { isLoading: isLoadingUser, data: user } = useUserMe() const { isLoading: isLoadingProfile, data: profile } = useProfileMeQuery() - const { hash } = useLocation() - const tabValue = keyFromHash(hash) - const [userListAction, setUserListAction] = useState("list") - const [userListId, setUserListId] = useState(0) + const { pathname } = useLocation() + const id = Number(useParams().id) || -1 + const showUserListDetail = pathname.includes(MY_LISTS) && id !== -1 + const tabValue = showUserListDetail + ? MY_LISTS + : [DASHBOARD_HOME, MY_LISTS, PROFILE, SETTINGS].includes(pathname) + ? pathname + : DASHBOARD_HOME const topics = profile?.preference_search_filters.topic const certification = profile?.preference_search_filters.certification - const handleActivateUserList = useCallback((userList: UserList) => { - setUserListId(userList.id) - setUserListAction("detail") - }, []) - const desktopMenu = ( @@ -339,27 +338,30 @@ const DashboardPage: React.FC = () => { > } - text={TabLabels[TabValues.HOME]} - value={TabValues.HOME} + text={TabLabels[DASHBOARD_HOME]} + tabKey={TabKeys[DASHBOARD_HOME]} + value={DASHBOARD_HOME} currentValue={tabValue} /> } - text={TabLabels[TabValues.MY_LISTS]} - value={TabValues.MY_LISTS} + text={TabLabels[MY_LISTS]} + tabKey={TabKeys[MY_LISTS]} + value={MY_LISTS} currentValue={tabValue} - onClick={() => setUserListAction("list")} /> } - text={TabLabels[TabValues.PROFILE]} - value={TabValues.PROFILE} + text={TabLabels[PROFILE]} + tabKey={TabKeys[PROFILE]} + value={PROFILE} currentValue={tabValue} /> } - text={TabLabels[TabValues.SETTINGS]} - value={TabValues.SETTINGS} + text={TabLabels[SETTINGS]} + tabKey={TabKeys[SETTINGS]} + value={SETTINGS} currentValue={tabValue} /> @@ -370,28 +372,27 @@ const DashboardPage: React.FC = () => { const mobileMenu = ( setUserListAction("list")} /> @@ -408,108 +409,94 @@ const DashboardPage: React.FC = () => { {mobileMenu} {desktopMenu} - - - - - - Your MIT Learning Journey - - - A customized course list based on your preferences. - - - - - Edit Profile - - - - - {topics?.map((topic, index) => ( + {showUserListDetail ? ( + + + + ) : ( + + + + + + Your MIT Learning Journey + + + A customized course list based on your preferences. + + + + + Edit Profile + + + - ))} - {certification === true ? ( + {topics?.map((topic, index) => ( + + ))} + {certification === true ? ( + + ) : ( + + )} - ) : ( - )} - - - - - {userListAction === "list" ? ( -
- -
- ) : ( -
- -
- )} - - - Profile - {isLoadingProfile || typeof profile === "undefined" ? ( - - ) : ( -
- -
- )} - - - Settings - {isLoadingProfile || !profile ? ( - - ) : ( -
- -
- )} - - + + + + + + Profile + {isLoadingProfile || !profile ? ( + + ) : ( +
+ +
+ )} + + + Settings + {isLoadingProfile || !profile ? ( + + ) : ( +
+ +
+ )} + + + )} @@ -518,4 +505,8 @@ const DashboardPage: React.FC = () => { ) } -export { DashboardPage, TabLabels as DashboardTabLabels } +export { + DashboardPage, + TabKeys as DashboardTabKeys, + TabLabels as DashboardTabLabels, +} diff --git a/frontends/mit-open/src/pages/HomePage/HomePage.test.tsx b/frontends/mit-open/src/pages/HomePage/HomePage.test.tsx index d36f46d073..2c4e7e4651 100644 --- a/frontends/mit-open/src/pages/HomePage/HomePage.test.tsx +++ b/frontends/mit-open/src/pages/HomePage/HomePage.test.tsx @@ -293,7 +293,7 @@ describe("Home Page personalize section", () => { expect(link).toHaveAttribute( "href", routes.login({ - pathname: routes.DASHBOARD, + pathname: routes.DASHBOARD_HOME, }), ) }) diff --git a/frontends/mit-open/src/pages/HomePage/PersonalizeSection.tsx b/frontends/mit-open/src/pages/HomePage/PersonalizeSection.tsx index 34f380e2cc..515edd1f41 100644 --- a/frontends/mit-open/src/pages/HomePage/PersonalizeSection.tsx +++ b/frontends/mit-open/src/pages/HomePage/PersonalizeSection.tsx @@ -65,7 +65,7 @@ const AUTH_TEXT_DATA = { text: "Ready to keep learning? Choose from your saved courses, find personalized recommendations, and see what's trending on your dashboard.", linkProps: { children: "My Dashboard", - href: urls.DASHBOARD, + href: urls.DASHBOARD_HOME, }, }, anonymous: { @@ -75,7 +75,7 @@ const AUTH_TEXT_DATA = { children: "Sign Up for Free", reloadDocument: true, href: urls.login({ - pathname: urls.DASHBOARD, + pathname: urls.DASHBOARD_HOME, }), }, }, diff --git a/frontends/mit-open/src/pages/ListDetailsPage/ListDetailsPage.test.ts b/frontends/mit-open/src/pages/ListDetailsPage/ListDetailsPage.test.ts index b8ac2ccee5..663a188f97 100644 --- a/frontends/mit-open/src/pages/ListDetailsPage/ListDetailsPage.test.ts +++ b/frontends/mit-open/src/pages/ListDetailsPage/ListDetailsPage.test.ts @@ -100,7 +100,7 @@ describe("ListDetailsPage", () => { setup({ path, userSettings }) await screen.findByRole("heading", { name: path.title }) - const editButton = screen.queryByRole("button", { name: "Edit" }) + const editButton = screen.queryByRole("button", { name: "Edit List" }) expect(!!editButton).toBe(canEdit) }, ) @@ -168,7 +168,7 @@ describe("ListDetailsPage", () => { test("Edit buttons opens editing dialog", async () => { const path = factories.learningResources.learningPath() setup({ path, userSettings: { is_learning_path_editor: true } }) - const editButton = await screen.findByRole("button", { name: "Edit" }) + const editButton = await screen.findByRole("button", { name: "Edit List" }) const editList = jest.spyOn(manageListDialogs, "upsertLearningPath") editList.mockImplementationOnce(jest.fn()) diff --git a/frontends/mit-open/src/pages/ListDetailsPage/ListDetailsPage.tsx b/frontends/mit-open/src/pages/ListDetailsPage/ListDetailsPage.tsx index f0efbbcbeb..1764734acc 100644 --- a/frontends/mit-open/src/pages/ListDetailsPage/ListDetailsPage.tsx +++ b/frontends/mit-open/src/pages/ListDetailsPage/ListDetailsPage.tsx @@ -1,9 +1,13 @@ import React from "react" -import { Container, BannerPage } from "ol-components" +import { Container, BannerPage, styled } from "ol-components" import { MetaTags } from "ol-utilities" import ItemsListingComponent from "@/page-components/ItemsListing/ItemsListingComponent" import type { ItemsListingComponentProps } from "@/page-components/ItemsListing/ItemsListingComponent" +const StyledContainer = styled(Container)({ + paddingTop: "24px", +}) + const ListDetailsPage: React.FC = ({ listType, list, @@ -20,7 +24,7 @@ const ListDetailsPage: React.FC = ({ className="learningpaths-page" > - + = ({ isFetching={isFetching} handleEdit={handleEdit} /> - + ) } diff --git a/frontends/mit-open/src/pages/OnboardingPage/OnboardingPage.tsx b/frontends/mit-open/src/pages/OnboardingPage/OnboardingPage.tsx index f0ccc2a73b..9dcdbf4e7b 100644 --- a/frontends/mit-open/src/pages/OnboardingPage/OnboardingPage.tsx +++ b/frontends/mit-open/src/pages/OnboardingPage/OnboardingPage.tsx @@ -20,7 +20,7 @@ import { import { MetaTags } from "ol-utilities" import { RiArrowRightLine, RiArrowLeftLine } from "@remixicon/react" import { useProfileMeMutation, useProfileMeQuery } from "api/hooks/profile" -import { DASHBOARD } from "@/common/urls" +import { DASHBOARD_HOME } from "@/common/urls" import { useFormik } from "formik" import { useLearningResourceTopics } from "api/hooks/learningResources" @@ -168,7 +168,7 @@ const OnboardingPage: React.FC = () => { if (activeStep < NUM_STEPS - 1) { setActiveStep((prevActiveStep) => prevActiveStep + 1) } else { - navigate(DASHBOARD) + navigate(DASHBOARD_HOME) } }, validateOnChange: false, diff --git a/frontends/mit-open/src/pages/PrivacyPage/PrivacyPage.tsx b/frontends/mit-open/src/pages/PrivacyPage/PrivacyPage.tsx index 0080d9aa38..f3eb87bc1d 100644 --- a/frontends/mit-open/src/pages/PrivacyPage/PrivacyPage.tsx +++ b/frontends/mit-open/src/pages/PrivacyPage/PrivacyPage.tsx @@ -87,6 +87,10 @@ const PrivacyPage: React.FC = () => { Biographic information – name, email address, education level and other demographic info +
  • + Demographics and Interests - Affinity categories, Product Purchase + Interests, and Other Categories of interest +
  • IP addresses
  • Course progress and performance
    • 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/frontends/mit-open/src/pages/UserListListingComponent/UserListListingComponent.test.tsx b/frontends/mit-open/src/pages/UserListListingComponent/UserListListingComponent.test.tsx new file mode 100644 index 0000000000..c35e645070 --- /dev/null +++ b/frontends/mit-open/src/pages/UserListListingComponent/UserListListingComponent.test.tsx @@ -0,0 +1,107 @@ +import React from "react" +import { faker } from "@faker-js/faker/locale/en" +import { factories, urls } from "api/test-utils" +import { + screen, + renderWithProviders, + setMockResponse, + user, + expectProps, + within, +} from "../../test-utils" +import type { User } from "../../test-utils" + +import UserListListingComponent from "./UserListListingComponent" +import { manageListDialogs } from "@/page-components/ManageListDialogs/ManageListDialogs" +import UserListCardCondensed from "@/page-components/UserListCard/UserListCardCondensed" +import { userListView } from "@/common/urls" + +jest.mock("../../page-components/UserListCard/UserListCardCondensed", () => { + const actual = jest.requireActual( + "../../page-components/UserListCard/UserListCardCondensed", + ) + return { + __esModule: true, + ...actual, + default: jest.fn(actual.default), + } +}) +const spyULCardCondensed = jest.mocked(UserListCardCondensed) + +/** + * Set up the mock API responses for lists pages. + */ +const setup = ({ + listsCount = faker.number.int({ min: 2, max: 5 }), + user = { is_learning_path_editor: false }, +}: { + user?: Partial + listsCount?: number +} = {}) => { + const paths = factories.userLists.userLists({ count: listsCount }) + setMockResponse.get(urls.userLists.list(), paths) + + setMockResponse.get(urls.userSubscription.check(), factories.percolateQueries) + + const { location } = renderWithProviders( + , + { + user, + }, + ) + + return { paths, location } +} + +describe("UserListListingPage", () => { + it("Has heading 'User Lists' and correct page title", async () => { + setup() + screen.getByRole("heading", { name: "My Lists" }) + }) + + it("Renders a card for each user list", async () => { + const { paths } = setup() + const titles = paths.results.map((userList) => userList.title) + const headings = await screen.findAllByRole("heading", { + name: (value) => titles.includes(value), + }) + + // for sanity + expect(headings.length).toBeGreaterThan(0) + expect(titles.length).toBe(headings.length) + + paths.results.forEach((userList) => { + expectProps(spyULCardCondensed, { + href: userListView(userList.id), + userList: userList, + }) + }) + }) + + test("Clicking new list opens the creation dialog", async () => { + const createList = jest + .spyOn(manageListDialogs, "upsertUserList") + .mockImplementationOnce(jest.fn()) + setup() + const newListButton = await screen.findByRole("button", { + name: "Create new list", + }) + + expect(createList).not.toHaveBeenCalled() + await user.click(newListButton) + + // Check details of this dialog elsewhere + expect(createList).toHaveBeenCalledWith() + }) + + test("Clicking on the card shows the list detail view", async () => { + const { paths, location } = setup() + const card = await screen.findByTestId( + `user-list-card-condensed-${paths.results[0].id}`, + ) + const cardLink = within(card).getByRole("link") + + await user.click(cardLink) + expect(location.current.pathname).toBe(userListView(paths.results[0].id)) + }) +}) diff --git a/frontends/mit-open/src/pages/UserListListingComponent/UserListListingComponent.tsx b/frontends/mit-open/src/pages/UserListListingComponent/UserListListingComponent.tsx new file mode 100644 index 0000000000..aaa3daaef2 --- /dev/null +++ b/frontends/mit-open/src/pages/UserListListingComponent/UserListListingComponent.tsx @@ -0,0 +1,75 @@ +import React, { useCallback } from "react" +import { + Button, + LoadingSpinner, + styled, + Typography, + PlainList, +} from "ol-components" + +import { useUserListList } from "api/hooks/learningResources" + +import { GridColumn, GridContainer } from "@/components/GridLayout/GridLayout" + +import { manageListDialogs } from "@/page-components/ManageListDialogs/ManageListDialogs" +import { userListView } from "@/common/urls" +import UserListCardCondensed from "@/page-components/UserListCard/UserListCardCondensed" + +const Header = styled(Typography)({ + marginBottom: "16px", +}) + +const NewListButton = styled(Button)(({ theme }) => ({ + marginTop: "24px", + width: "200px", + [theme.breakpoints.down("sm")]: { + width: "100%", + }, +})) + +type UserListListingComponentProps = { + title?: string +} + +const UserListListingComponent: React.FC = ( + props, +) => { + const { title } = props + const listingQuery = useUserListList() + const handleCreate = useCallback(() => { + manageListDialogs.upsertUserList() + }, []) + + return ( + + +
    {title}
    +
    + + {listingQuery.data && ( + + {listingQuery.data.results?.map((list) => { + return ( +
  • + +
    • + ) + })} +     + )} + + Create new list + +
    +     +     + ) +} + +export default UserListListingComponent diff --git a/frontends/mit-open/src/pages/UserListListingPage/UserListListingPage.test.tsx b/frontends/mit-open/src/pages/UserListListingPage/UserListListingPage.test.tsx deleted file mode 100644 index 753b9e9c39..0000000000 --- a/frontends/mit-open/src/pages/UserListListingPage/UserListListingPage.test.tsx +++ /dev/null @@ -1,146 +0,0 @@ -import React from "react" -import { faker } from "@faker-js/faker/locale/en" -import { factories, urls } from "api/test-utils" -import { - screen, - renderWithProviders, - setMockResponse, - user, - expectProps, - waitFor, -} from "../../test-utils" -import type { User } from "../../test-utils" - -import { UserListListingPage } from "./UserListListingPage" -import UserListCardTemplate from "@/page-components/UserListCardTemplate/UserListCardTemplate" -import { manageListDialogs } from "@/page-components/ManageListDialogs/ManageListDialogs" - -jest.mock( - "../../page-components/UserListCardTemplate/UserListCardTemplate", - () => { - const actual = jest.requireActual( - "../../page-components/UserListCardTemplate/UserListCardTemplate", - ) - return { - __esModule: true, - ...actual, - default: jest.fn(actual.default), - } - }, -) -const spyULCardTemplate = jest.mocked(UserListCardTemplate) - -/** - * Set up the mock API responses for lists pages. - */ -const setup = ({ - listsCount = faker.number.int({ min: 2, max: 5 }), - user = { is_learning_path_editor: false }, -}: { - user?: Partial - listsCount?: number -} = {}) => { - const paths = factories.userLists.userLists({ count: listsCount }) - - setMockResponse.get(urls.userLists.list(), paths) - - setMockResponse.get(urls.userSubscription.check(), factories.percolateQueries) - - const { location } = renderWithProviders(, { - user, - }) - - return { paths, location } -} - -describe("UserListListingPage", () => { - it("Has heading 'User Lists' and correct page title", async () => { - setup() - screen.getByRole("heading", { name: "User Lists" }) - await waitFor(() => expect(document.title).toBe("My Lists | MIT Open")) - }) - - it("Renders a card for each user list", async () => { - const { paths } = setup() - const titles = paths.results.map((userList) => userList.title) - const headings = await screen.findAllByRole("heading", { - name: (value) => titles.includes(value), - }) - - // for sanity - expect(headings.length).toBeGreaterThan(0) - expect(titles.length).toBe(headings.length) - - paths.results.forEach((userList) => { - expectProps(spyULCardTemplate, { userList: userList }) - }) - }) - - test("Clicking edit -> Edit on opens the editing dialog", async () => { - const editList = jest - .spyOn(manageListDialogs, "upsertUserList") - .mockImplementationOnce(jest.fn()) - - const { paths } = setup() - const path = faker.helpers.arrayElement(paths.results) - - const menuButton = await screen.findByRole("button", { - name: `Edit list ${path.title}`, - }) - await user.click(menuButton) - const editButton = screen.getByRole("menuitem", { name: "Edit" }) - await user.click(editButton) - - expect(editList).toHaveBeenCalledWith(path) - }) - - test("Clicking edit -> Delete opens the deletion dialog", async () => { - const deleteList = jest - .spyOn(manageListDialogs, "destroyUserList") - .mockImplementationOnce(jest.fn()) - - const { paths } = setup() - const path = faker.helpers.arrayElement(paths.results) - - const menuButton = await screen.findByRole("button", { - name: `Edit list ${path.title}`, - }) - await user.click(menuButton) - const deleteButton = screen.getByRole("menuitem", { name: "Delete" }) - - await user.click(deleteButton) - - // Check details of this dialog elsewhere - expect(deleteList).toHaveBeenCalledWith(path) - }) - - test("Clicking new list opens the creation dialog", async () => { - const createList = jest - .spyOn(manageListDialogs, "upsertUserList") - .mockImplementationOnce(jest.fn()) - setup() - const newListButton = await screen.findByRole("button", { - name: "Create new list", - }) - - expect(createList).not.toHaveBeenCalled() - await user.click(newListButton) - - // Check details of this dialog elsewhere - expect(createList).toHaveBeenCalledWith() - }) - - test("Clicking on list title navigates to list page", async () => { - const { location, paths } = setup() - const path = faker.helpers.arrayElement(paths.results) - const listTitle = await screen.findByRole("heading", { name: path.title }) - await user.click(listTitle) - expect(location.current).toEqual( - expect.objectContaining({ - pathname: `/userlists/${path.id}`, - search: "", - hash: "", - }), - ) - }) -}) diff --git a/frontends/mit-open/src/pages/UserListListingPage/UserListListingPage.tsx b/frontends/mit-open/src/pages/UserListListingPage/UserListListingPage.tsx deleted file mode 100644 index 1b5e85ddc9..0000000000 --- a/frontends/mit-open/src/pages/UserListListingPage/UserListListingPage.tsx +++ /dev/null @@ -1,161 +0,0 @@ -import React, { useCallback, useMemo } from "react" -import { - Button, - Grid, - LoadingSpinner, - BannerPage, - Container, - styled, - SimpleMenuItem, - SimpleMenu, - ActionButton, - Typography, - PlainList, - imgConfigs, -} from "ol-components" -import { RiPencilFill, RiMore2Fill, RiDeleteBin7Fill } from "@remixicon/react" - -import { MetaTags } from "ol-utilities" -import type { UserList } from "api" -import { useUserListList } from "api/hooks/learningResources" - -import { GridColumn, GridContainer } from "@/components/GridLayout/GridLayout" - -import UserListCardTemplate from "@/page-components/UserListCardTemplate/UserListCardTemplate" -import { useNavigate } from "react-router" -import * as urls from "@/common/urls" -import { manageListDialogs } from "@/page-components/ManageListDialogs/ManageListDialogs" - -const PageContainer = styled(Container)({ - marginTop: "1rem", -}) - -const ListHeaderGrid = styled(Grid)({ - marginBottom: "1rem", -}) - -type EditUserListMenuProps = { - userList: UserList -} - -const EditUserListMenu: React.FC = ({ userList }) => { - const items: SimpleMenuItem[] = useMemo( - () => [ - { - key: "edit", - label: "Edit", - icon: , - onClick: () => manageListDialogs.upsertUserList(userList), - }, - { - key: "delete", - label: "Delete", - icon: , - onClick: () => manageListDialogs.destroyUserList(userList), - }, - ], - [userList], - ) - return ( - - - - } - items={items} - /> - ) -} - -type UserListListingComponentProps = { - title?: string - onActivate: (userList: UserList) => void -} - -const UserListListingComponent: React.FC = ( - props, -) => { - const { title, onActivate } = props - const listingQuery = useUserListList() - const handleCreate = useCallback(() => { - manageListDialogs.upsertUserList() - }, []) - - return ( - - - - - - {title} - - - - - - -
    - - {listingQuery.data && ( - - {listingQuery.data.results?.map((list) => { - return ( -
  • - } - /> -
    • - ) - })} -     - )} -
    -     -     - ) -} - -const UserListListingPage: React.FC = () => { - const navigate = useNavigate() - const handleActivate = useCallback( - (userList: UserList) => { - const path = urls.userListView(userList.id) - navigate(path) - }, - [navigate], - ) - return ( - - - - - - - ) -} - -export { UserListListingComponent, UserListListingPage } diff --git a/frontends/mit-open/src/routes.tsx b/frontends/mit-open/src/routes.tsx index 626daad097..dd92134a55 100644 --- a/frontends/mit-open/src/routes.tsx +++ b/frontends/mit-open/src/routes.tsx @@ -7,7 +7,6 @@ import LearningPathListingPage from "@/pages/LearningPathListingPage/LearningPat import ChannelPage from "@/pages/ChannelPage/ChannelPage" import EditChannelPage from "@/pages/ChannelPage/EditChannelPage" -import { UserListListingPage } from "./pages/UserListListingPage/UserListListingPage" import ArticleDetailsPage from "@/pages/ArticleDetailsPage/ArticleDetailsPage" import { ArticleCreatePage, ArticleEditPage } from "@/pages/ArticleUpsertPages" import ProgramLetterPage from "@/pages/ProgramLetterPage/ProgramLetterPage" @@ -21,7 +20,6 @@ import Header from "@/page-components/Header/Header" import Footer from "@/page-components/Footer/Footer" import { Permissions } from "@/common/permissions" import SearchPage from "./pages/SearchPage/SearchPage" -import UserListDetailsPage from "./pages/ListDetailsPage/UserListDetailsPage" import LearningPathDetailsPage from "./pages/ListDetailsPage/LearningPathDetailsPage" import LearningResourceDrawer from "./page-components/LearningResourceDrawer/LearningResourceDrawer" import DepartmentListingPage from "./pages/DepartmentListingPage/DepartmentListingPage" @@ -84,10 +82,18 @@ const routes: RouteObject[] = [ element: , }, { - path: urls.USERLIST_LISTING, + path: urls.DASHBOARD_HOME, element: ( - + + + ), + }, + { + path: urls.MY_LISTS, + element: ( + + ), }, @@ -95,12 +101,20 @@ const routes: RouteObject[] = [ path: urls.USERLIST_VIEW, element: ( - + + + ), + }, + { + path: urls.PROFILE, + element: ( + + ), }, { - path: urls.DASHBOARD, + path: urls.SETTINGS, element: ( diff --git a/frontends/mit-open/src/types/settings.d.ts b/frontends/mit-open/src/types/settings.d.ts index d5c99ea243..83da0d2a88 100644 --- a/frontends/mit-open/src/types/settings.d.ts +++ b/frontends/mit-open/src/types/settings.d.ts @@ -12,12 +12,3 @@ export type PostHogSettings = { timeout?: int bootstrap_flags?: Record } - -export declare global { - const APP_SETTINGS: { - SENTRY_DSN?: string - VERSION?: string - ENVIRONMENT?: string - posthog?: PostHogSettings - } -} diff --git a/frontends/mit-open/webpack.config.js b/frontends/mit-open/webpack.config.js index 63900495d2..3a1910404d 100644 --- a/frontends/mit-open/webpack.config.js +++ b/frontends/mit-open/webpack.config.js @@ -1,7 +1,7 @@ /* eslint-disable @typescript-eslint/no-var-requires */ const path = require("path") -if (process.env.ENVIRONMENT === "local") { +if (process.env.LOAD_ENV_FILES?.toLowerCase() === "true") { console.info("Loading environment from .env files") require("dotenv").config({ path: [ @@ -26,7 +26,6 @@ const ReactRefreshWebpackPlugin = require("@pmmmwh/react-refresh-webpack-plugin" const { cleanEnv, str, bool, port } = require("envalid") const { - ENVIRONMENT, NODE_ENV, PORT, VERSION, @@ -41,10 +40,6 @@ const { CKEDITOR_UPLOAD_URL, SENTRY_DSN, } = cleanEnv(process.env, { - ENVIRONMENT: str({ - choices: ["local", "docker", "production"], - default: "production", - }), NODE_ENV: str({ choices: ["development", "production", "test"], default: "production", @@ -94,6 +89,10 @@ const { desc: "Location of the CKEditor uploads handler", default: "", }), + SENTRY_ENV: str({ + desc: "A label for the environment used for grouping errors in Sentry", + default: "", + }), SENTRY_DSN: str({ desc: "Sentry Data Source Name", default: "", @@ -216,7 +215,6 @@ module.exports = (env, argv) => { MITOPEN_API_BASE_URL: JSON.stringify(MITOPEN_API_BASE_URL), EMBEDLY_KEY: JSON.stringify(EMBEDLY_KEY), CKEDITOR_UPLOAD_URL: JSON.stringify(CKEDITOR_UPLOAD_URL), - ENVIRONMENT: JSON.stringify(ENVIRONMENT), VERSION: JSON.stringify(VERSION), SENTRY_DSN: JSON.stringify(SENTRY_DSN), POSTHOG: getPostHogSettings(), @@ -238,7 +236,7 @@ module.exports = (env, argv) => { : [new ReactRefreshWebpackPlugin()], ) .concat( - ENVIRONMENT !== "local" && WEBPACK_ANALYZE === "True" + WEBPACK_ANALYZE ? [ new BundleAnalyzerPlugin({ analyzerMode: "static", diff --git a/frontends/ol-components/src/index.ts b/frontends/ol-components/src/index.ts index da0757a32f..0ebac874ff 100644 --- a/frontends/ol-components/src/index.ts +++ b/frontends/ol-components/src/index.ts @@ -164,6 +164,7 @@ export * from "./components/Alert/Alert" export * from "./components/BannerPage/BannerPage" export * from "./components/Breadcrumbs/Breadcrumbs" export * from "./components/Card/Card" +export * from "./components/Card/ListCardCondensed" export * from "./components/Carousel/Carousel" export * from "./components/Checkbox/Checkbox" export * from "./components/Checkbox/CheckboxChoiceField" diff --git a/frontends/ol-utilities/src/types/settings.d.ts b/frontends/ol-utilities/src/types/settings.d.ts index c07998375d..a5da5cd142 100644 --- a/frontends/ol-utilities/src/types/settings.d.ts +++ b/frontends/ol-utilities/src/types/settings.d.ts @@ -14,7 +14,7 @@ export declare global { CKEDITOR_UPLOAD_URL?: string SENTRY_DSN?: string VERSION?: string - ENVIRONMENT?: string + SENTRY_ENV?: string POSTHOG?: PostHogSettings SITE_NAME: string MITOPEN_SUPPORT_EMAIL: string diff --git a/learning_resources/data/README-topics.md b/learning_resources/data/README-topics.md index e8a354c897..1b73b287fd 100644 --- a/learning_resources/data/README-topics.md +++ b/learning_resources/data/README-topics.md @@ -29,9 +29,16 @@ topics: - offeror topic - other offeror topic children: [] + parent: optional ID for the parent (read the Updating section before using this) ``` -Children use the same format as the items under `topics`. +Children use the same format as the items under `topics`. Topic IDs are UUIDs. + +## Updating topics (and the `parent` element) + +A given topic's parent/child relationship is usually defined by their position in the yaml file. Children are in the parent's `children` list and the upsert function uses this when it creates the topics in the database. + +There is a slight problem with this - if you want to _modify_ a topic, you now have to specify the whole tree, or the upsert code won't know what they belong to. This is where the `parent` element comes in. You can set this to the UUID of the parent that it should have, and the upserter will load that topic and use it as the parent so you don't have to specify the whole tree. ## Transforming topic maps 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..0174b3d263 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): @@ -486,6 +487,7 @@ def _walk_topic_map(topics: list, parent: None | LearningResourceTopic = None) - - icon - the icon we should display for the topic (a Remixicon, generally) - mappings - mappings for topics found in offeror data - children - child topics (records in this same format) + - parent - specific parent for the topic A more detailed definition of this is in data/README-topics.md. Args: @@ -502,6 +504,22 @@ def _walk_topic_map(topics: list, parent: None | LearningResourceTopic = None) - "icon": topic["icon"] or "", } + if not parent and "parent" in topic: + # Topic specifies a parent, so let's try to grab it. + try: + defaults["parent"] = LearningResourceTopic.objects.filter( + topic_uuid=topic["parent"] + ).get() + except LearningResourceTopic.DoesNotExist: + log.warning( + ( + "_walk_topic_map: topic %s specified a parent %s but" + " the parent did not exist" + ), + topic["name"], + topic["parent"], + ) + if topic["id"]: defaults["topic_uuid"] = topic["id"] diff --git a/learning_resources/utils_test.py b/learning_resources/utils_test.py index 34c748aad4..abbc9d4a30 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 ) @@ -384,6 +388,103 @@ def test_modify_topic_data_string(mocker): ) +def test_modify_topic_with_parent(mocker): + """Test that the parent option is processed correctly when upserting topics""" + + mock_pluggy = mocker.patch("learning_resources.utils.topic_upserted_actions") + + base_topic_file = """ +--- +topics: + - icon: RiRobot2Line + id: c06109bf-cff8-4873-b04b-f5e66e3e1764 + mappings: + mitx: + - Electronics + ocw: + - Technology + name: Data Science, Analytics & Computer Technology + children: + - children: [] + icon: RiRobot2Line + id: 4cd6156e-51a0-4da4-add4-6f81e106cd43 + mappings: + ocw: + - Programming Languages + - Software Design and Engineering + mitx: + - Computer Science + name: Programming & Coding +""" + + new_topic_file = """ +--- +topics: + - children: [] + icon: RiRobot2Line + id: d335c250-1292-4391-a7cb-3181f803e0f3 + mappings: [] + name: Debugging + parent: 4cd6156e-51a0-4da4-add4-6f81e106cd43 +""" + + new_topic_nested_parent_file = """ +topics: + - icon: RiRobot2Line + id: c06109bf-cff8-4873-b04b-f5e66e3e1764 + mappings: + mitx: + - Electronics + ocw: + - Technology + name: Data Science, Analytics & Computer Technology + children: + - children: [] + icon: RiRobot2Line + id: ea647bfc-cc83-42c7-b685-b5c2088b30af + mappings: [] + name: Google Analytics +""" + + upsert_topic_data_string(base_topic_file) + + assert mock_pluggy.called + + main_topic = LearningResourceTopic.objects.filter( + topic_uuid="c06109bf-cff8-4873-b04b-f5e66e3e1764" + ) + + assert main_topic.exists() + main_topic = main_topic.get() + + sub_topic = LearningResourceTopic.objects.filter(parent=main_topic) + + assert sub_topic.exists() + sub_topic = sub_topic.get() + + upsert_topic_data_string(new_topic_file) + + new_topic = LearningResourceTopic.objects.filter( + topic_uuid="d335c250-1292-4391-a7cb-3181f803e0f3" + ) + + assert new_topic.exists() + assert new_topic.get().parent == sub_topic + + upsert_topic_data_string(new_topic_nested_parent_file) + + main_topic.refresh_from_db() + + sub_topic = LearningResourceTopic.objects.filter(parent=main_topic) + assert sub_topic.count() == 2 + + for topic in sub_topic.all(): + assert str(topic.topic_uuid) in [ + "ea647bfc-cc83-42c7-b685-b5c2088b30af", + "4cd6156e-51a0-4da4-add4-6f81e106cd43", + ] + + def test_add_parent_topics_to_learning_resource(fixture_resource): """Ensure the parent topics get added to the 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/models.py b/learning_resources_search/models.py index 5a37efebc6..058381b2af 100644 --- a/learning_resources_search/models.py +++ b/learning_resources_search/models.py @@ -4,7 +4,6 @@ from django.db import models from django.db.models import JSONField -from channels.constants import ChannelType from channels.models import Channel from main.models import TimestampedModel @@ -29,11 +28,25 @@ class PercolateQuery(TimestampedModel): ) users = models.ManyToManyField(User, related_name="percolate_queries") - def __str__(self): - return f"Percolate query {self.id}: {self.query}" + def source_label(self): + source_channel = self.source_channel() + if source_channel: + return source_channel.channel_type + else: + return "saved_search" - class Meta: - unique_together = (("source_type", "original_query"),) + def source_description(self): + channel = self.source_channel() + if channel: + return channel.title + return self.original_url_params() + + def source_channel(self): + original_query_params = self.original_url_params() + channels_filtered = Channel.objects.filter(search_filter=original_query_params) + if channels_filtered.exists(): + return channels_filtered.first() + return None def original_url_params(self): ignore_params = ["endpoint"] @@ -43,19 +56,8 @@ def original_url_params(self): } return urlencode(defined_params, doseq=True) - def source_label(self): - original_query_params = self.original_url_params() - channels_filtered = Channel.objects.filter(search_filter=original_query_params) - if channels_filtered.exists(): - return channels_filtered.first().channel_type - else: - return "saved_search" - - def source_description(self): - original_query_params = self.original_url_params() - source_label = self.source_label() + def __str__(self): + return f"Percolate query {self.id}: {self.query}" - if source_label in ChannelType: - channel = Channel.objects.get(search_filter=original_query_params) - return channel.title - return self.original_url_params() + class Meta: + unique_together = (("source_type", "original_query"),) 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/learning_resources_search/serializers_test.py b/learning_resources_search/serializers_test.py index 254217d6aa..8ce934e30f 100644 --- a/learning_resources_search/serializers_test.py +++ b/learning_resources_search/serializers_test.py @@ -557,19 +557,19 @@ def test_serialize_bulk_learning_resources(mocker): every existing learning resource """ # NOTE: explicitly creating a fixed number per type for a deterministic query count - resources = [ - *factories.LearningResourceFactory.create_batch(5, is_course=True), - *factories.LearningResourceFactory.create_batch(5, is_program=True), - *factories.LearningResourceFactory.create_batch(5, is_podcast=True), - *factories.LearningResourceFactory.create_batch(5, is_podcast_episode=True), - *factories.LearningResourceFactory.create_batch(5, is_video=True), - *factories.LearningResourceFactory.create_batch(5, is_video_playlist=True), - ] - resources = LearningResource.objects.for_serialization() - results = list( + factories.LearningResourceFactory.create_batch(5, is_course=True) + factories.LearningResourceFactory.create_batch(5, is_program=True) + factories.LearningResourceFactory.create_batch(5, is_podcast=True) + factories.LearningResourceFactory.create_batch(5, is_podcast_episode=True) + factories.LearningResourceFactory.create_batch(5, is_video=True) + factories.LearningResourceFactory.create_batch(5, is_video_playlist=True) + + resources = LearningResource.objects.order_by("id").for_serialization() + results = sorted( serializers.serialize_bulk_learning_resources( [resource.id for resource in resources] - ) + ), + key=lambda x: x["id"], ) expected = [] diff --git a/learning_resources_search/tasks.py b/learning_resources_search/tasks.py index 1d94d7b935..f4344d7aa8 100644 --- a/learning_resources_search/tasks.py +++ b/learning_resources_search/tasks.py @@ -12,7 +12,9 @@ from django.conf import settings from django.contrib.auth import get_user_model from django.db.models import Q +from django.template.defaultfilters import pluralize from opensearchpy.exceptions import NotFoundError, RequestError +from requests.models import PreparedRequest from learning_resources.etl.constants import RESOURCE_FILE_ETL_SOURCES from learning_resources.models import ( @@ -120,6 +122,8 @@ def _infer_percolate_group(percolate_query): Infer the heading name for the percolate query to be grouped under in the email """ + if percolate_query.source_label() != "saved_search": + return percolate_query.source_description() group_keys = ["department", "topic", "offered_by"] original_query = OrderedDict(percolate_query.original_query) for key, val in original_query.items(): @@ -132,10 +136,13 @@ def _infer_percolate_group(percolate_query): return None -def _infer_search_url(percolate_query): +def _infer_percolate_group_url(percolate_query): """ Infer the search URL for the percolate query """ + source_channel = percolate_query.source_channel() + if source_channel: + return frontend_absolute_url(source_channel.channel_url) original_query = OrderedDict(percolate_query.original_query) query_string_params = {k: v for k, v in original_query.items() if v} if "endpoint" in query_string_params: @@ -178,14 +185,27 @@ def _get_percolated_rows(resources, subscription_type): percolated_users = set(percolated.values_list("users", flat=True)) all_users.update(percolated_users) query = percolated.first() + search_url = _infer_percolate_group_url(query) + req = PreparedRequest() + req.prepare_url(search_url, {"resource": resource.id}) + resource_url = req.url + source_channel = query.source_channel() rows.extend( [ { - "resource_url": resource.url, + "resource_url": resource_url, "resource_title": resource.title, + "resource_image_url": resource.image.url + if resource.image + else "", + "resource_type": resource.resource_type, "user_id": user, + "source_label": query.source_label(), + "source_channel_type": source_channel.channel_type + if source_channel + else "saved_search", "group": _infer_percolate_group(query), - "search_url": _infer_search_url(query), + "search_url": search_url, } for user in percolated_users ] @@ -209,7 +229,6 @@ def send_subscription_emails(self, subscription_type, period="daily"): ) rows = _get_percolated_rows(new_learning_resources, subscription_type) template_data = _group_percolated_rows(rows) - email_tasks = celery.group( [ attempt_send_digest_email_batch.si(user_template_items) @@ -816,6 +835,30 @@ def finish_recreate_index(results, backing_indices): log.info("recreate_index has finished successfully!") +def _generate_subscription_digest_subject( + sample_course, source_name, unique_resource_types, total_count, shortform +): + prefix = "" if shortform else "MIT Learn: " + + if sample_course["source_channel_type"] == "saved_search": + return ( + f"{prefix}New" + f' "{source_name}" ' + f"{unique_resource_types.pop().capitalize()}{pluralize(total_count)}" + ) + preposition = "from" + if sample_course["source_channel_type"] == "topic": + preposition = "in" + + suffix = "" if shortform else f": {sample_course['resource_title']}" + + return ( + f"{prefix}New" + f" {unique_resource_types.pop().capitalize()}{pluralize(total_count)} " + f"{preposition} {source_name}{suffix}" + ) + + @app.task( acks_late=True, reject_on_worker_lost=True, @@ -824,16 +867,40 @@ def finish_recreate_index(results, backing_indices): def attempt_send_digest_email_batch(user_template_items): for user_id, template_data in user_template_items: log.info("Sending email to user %s", user_id) + if not user_id: + continue user = User.objects.get(id=user_id) - total_count = sum([len(template_data[group]) for group in template_data]) - subject = f"{settings.MITOPEN_TITLE} New Learning Resources for You" - send_template_email( - [user.email], - subject, - "email/subscribed_channel_digest.html", - context={ - "documents": template_data, - "total_count": total_count, - "subject": subject, - }, - ) + + unique_resource_types = set() + for group in template_data: + total_count = len(template_data[group]) + unique_resource_types.update( + [resource["resource_type"] for resource in template_data[group]] + ) + subject = _generate_subscription_digest_subject( + template_data[group][0], + group, + list(unique_resource_types), + total_count, + shortform=False, + ) + # generate a shorter subject for use in the template + short_subject = _generate_subscription_digest_subject( + template_data[group][0], + group, + list(unique_resource_types), + total_count, + shortform=True, + ) + send_template_email( + [user.email], + subject, + "email/subscribed_channel_digest.html", + context={ + "documents": template_data[group], + "total_count": total_count, + "subject": subject, + "resource_group": group, + "short_subject": short_subject, + }, + ) diff --git a/learning_resources_search/tasks_test.py b/learning_resources_search/tasks_test.py index a65ac7d0e9..612e3d557b 100644 --- a/learning_resources_search/tasks_test.py +++ b/learning_resources_search/tasks_test.py @@ -35,6 +35,7 @@ serialize_learning_resource_for_update, ) from learning_resources_search.tasks import ( + _generate_subscription_digest_subject, _get_percolated_rows, _group_percolated_rows, _infer_percolate_group, @@ -929,3 +930,49 @@ def get_percolator(res): assert user.id == task_args[0] for topic in topics: assert topic in template_data + + +def test_subscription_digest_subject(): + """ + Test that email generates a dynamic subject based + on the unique resource types included + """ + resource_types = {"program"} + sample_course = {"source_channel_type": "topic", "resource_title": "robotics"} + + subject_line = _generate_subscription_digest_subject( + sample_course, + "electronics", + resource_types, + total_count=1, + shortform=False, + ) + assert subject_line == "MIT Learn: New Program in electronics: robotics" + + sample_course = {"source_channel_type": "podcast", "resource_title": "robotics"} + resource_types = {"program"} + + subject_line = _generate_subscription_digest_subject( + sample_course, + "xpro", + resource_types, + total_count=9, + shortform=False, + ) + assert subject_line == "MIT Learn: New Programs from xpro: robotics" + + resource_types = {"podcast"} + subject_line = _generate_subscription_digest_subject( + sample_course, + "engineering", + resource_types, + total_count=19, + shortform=False, + ) + assert subject_line == "MIT Learn: New Podcasts from engineering: robotics" + + resource_types = {"course"} + subject_line = _generate_subscription_digest_subject( + sample_course, "management", resource_types, 19, shortform=True + ) + assert subject_line == "New Courses from management" diff --git a/main/settings.py b/main/settings.py index b5a6dbce1b..bd66e52f93 100644 --- a/main/settings.py +++ b/main/settings.py @@ -33,7 +33,7 @@ from main.settings_pluggy import * # noqa: F403 from openapi.settings_spectacular import open_spectacular_settings -VERSION = "0.15.0" +VERSION = "0.15.1" log = logging.getLogger() diff --git a/main/templates/email/email_base.html b/main/templates/email/email_base.html index 623ab1978c..e3cbc446b0 100644 --- a/main/templates/email/email_base.html +++ b/main/templates/email/email_base.html @@ -167,7 +167,6 @@ } .button-td-primary:hover, .button-a-primary:hover { - background: #ffffff !important; border-color: #a31f34 !important; } @@ -231,17 +230,20 @@ margin: 0; padding: 0 !important; mso-line-height-rule: exactly; - background-color: #ffffff; + background-color: #f3f4f8; " > -
    +
    -