diff --git a/RELEASE.rst b/RELEASE.rst index c4fb14ca63..d24eb10fd1 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,13 @@ Release Notes ============= +Version 0.20.3 +-------------- + +- add is_incomplete_or_stale (#1627) +- "unfollow" confirmation modal and "Unfollow All" button (#1628) +- raise SystemExit as a RetryError (#1635) + Version 0.20.2 (Released October 01, 2024) -------------- diff --git a/frontends/mit-learn/src/pages/DashboardPage/SettingsPage.test.tsx b/frontends/mit-learn/src/pages/DashboardPage/SettingsPage.test.tsx index 38812527f3..16bcb2e1e3 100644 --- a/frontends/mit-learn/src/pages/DashboardPage/SettingsPage.test.tsx +++ b/frontends/mit-learn/src/pages/DashboardPage/SettingsPage.test.tsx @@ -25,10 +25,15 @@ const setupApis = ({ `${urls.userSubscription.check(subscriptionRequest)}`, subscribeResponse, ) - const unsubscribeUrl = urls.userSubscription.delete(subscribeResponse[0]?.id) - setMockResponse.delete(unsubscribeUrl, subscribeResponse[0]) + const unsubscribeUrls = [] + for (const sub of subscribeResponse) { + const unsubscribeUrl = urls.userSubscription.delete(sub?.id) + unsubscribeUrls.push(unsubscribeUrl) + setMockResponse.delete(unsubscribeUrl, sub) + } + return { - unsubscribeUrl, + unsubscribeUrls, } } @@ -46,7 +51,7 @@ describe("SettingsPage", () => { }) test("Clicking 'Unfollow' removes the subscription", async () => { - const { unsubscribeUrl } = setupApis({ + const { unsubscribeUrls } = setupApis({ isAuthenticated: true, isSubscribed: true, subscriptionRequest: {}, @@ -54,13 +59,42 @@ describe("SettingsPage", () => { renderWithProviders() const followList = await screen.findByTestId("follow-list") - const unsubscribeButton = within(followList).getAllByText("Unfollow")[0] - await user.click(unsubscribeButton) + const unsubscribeLink = within(followList).getAllByText("Unfollow")[0] + await user.click(unsubscribeLink) + const unsubscribeButton = await screen.findByTestId("dialog-unfollow") + await user.click(unsubscribeButton) expect(makeRequest).toHaveBeenCalledWith( "delete", - unsubscribeUrl, + unsubscribeUrls[0], undefined, ) }) + + test("Clicking 'Unfollow All' removes all subscriptions", async () => { + const { unsubscribeUrls } = setupApis({ + isAuthenticated: true, + isSubscribed: true, + subscriptionRequest: {}, + }) + renderWithProviders() + const unsubscribeLink = await screen.findByTestId("unfollow-all") + await user.click(unsubscribeLink) + + const unsubscribeButton = await screen.findByTestId("dialog-unfollow") + await user.click(unsubscribeButton) + for (const unsubUrl of unsubscribeUrls) { + expect(makeRequest).toHaveBeenCalledWith("delete", unsubUrl, undefined) + } + }) + test("Unsubscribe from all is hidden if there are no subscriptions", async () => { + setupApis({ + isAuthenticated: true, + isSubscribed: false, + subscriptionRequest: {}, + }) + renderWithProviders() + const unfollowButton = screen.queryByText("Unfollow All") + expect(unfollowButton).not.toBeInTheDocument() + }) }) diff --git a/frontends/mit-learn/src/pages/DashboardPage/SettingsPage.tsx b/frontends/mit-learn/src/pages/DashboardPage/SettingsPage.tsx index 2a135a5178..1e201aaf98 100644 --- a/frontends/mit-learn/src/pages/DashboardPage/SettingsPage.tsx +++ b/frontends/mit-learn/src/pages/DashboardPage/SettingsPage.tsx @@ -1,11 +1,19 @@ import React from "react" -import { PlainList, Typography, Link, styled } from "ol-components" +import { + PlainList, + Typography, + Link, + styled, + Button, + Dialog, + DialogActions, +} from "ol-components" import { useUserMe } from "api/hooks/user" import { useSearchSubscriptionDelete, useSearchSubscriptionList, } from "api/hooks/searchSubscription" - +import * as NiceModal from "@ebay/nice-modal-react" const SOURCE_LABEL_DISPLAY = { topic: "Topic", unit: "MIT Unit", @@ -13,6 +21,10 @@ const SOURCE_LABEL_DISPLAY = { saved_search: "Saved Search", } +const Actions = styled(DialogActions)({ + display: "flex", + "> *": { flex: 1 }, +}) const FollowList = styled(PlainList)(({ theme }) => ({ borderRadius: "8px", background: theme.custom.colors.white, @@ -37,6 +49,29 @@ const SubTitleText = styled(Typography)(({ theme }) => ({ ...theme.typography.body2, })) +const SettingsHeader = styled.div(({ theme }) => ({ + display: "flex", + alignItems: "center", + alignSelf: "stretch", + [theme.breakpoints.down("md")]: { + paddingBottom: "8px", + }, +})) + +const SettingsHeaderLeft = styled.div({ + display: "flex", + flexDirection: "column", + alignItems: "flex-start", + flex: "1 0 0", +}) + +const SettingsHeaderRight = styled.div(({ theme }) => ({ + display: "flex", + [theme.breakpoints.down("md")]: { + display: "none", + }, +})) + const ListItem = styled.li(({ theme }) => [ { padding: "16px 32px", @@ -83,22 +118,100 @@ const ListItemBody: React.FC = ({ ) } +type UnfollowDialogProps = { + subscriptionIds?: number[] + subscriptionName?: string +} +const UnfollowDialog = NiceModal.create( + ({ subscriptionIds, subscriptionName }: UnfollowDialogProps) => { + const modal = NiceModal.useModal() + const subscriptionDelete = useSearchSubscriptionDelete() + const unsubscribe = subscriptionDelete.mutate + return ( + + + + + + } + > + {subscriptionIds?.length === 1 ? ( + <> + Are you sure you want to unfollow {subscriptionName}? + + ) : ( + <> + Are you sure you want to Unfollow All? You will stop getting + emails for all topics, academic departments, and MIT units you are + following. + + )} + + ) + }, +) + const SettingsPage: React.FC = () => { const { data: user } = useUserMe() - const subscriptionDelete = useSearchSubscriptionDelete() + const subscriptionList = useSearchSubscriptionList({ enabled: !!user?.is_authenticated, }) - const unsubscribe = subscriptionDelete.mutate if (!user || subscriptionList.isLoading) return null return ( <> - Following - - All topics, academic departments, and MIT units you are following. - + + + Following + + All topics, academic departments, and MIT units you are following. + + + {subscriptionList?.data && subscriptionList?.data?.length > 1 ? ( + + + + ) : ( + <> + )} + {subscriptionList?.data?.map((subscriptionItem) => ( @@ -111,10 +224,13 @@ const SettingsPage: React.FC = () => { } /> { - event.preventDefault() - unsubscribe(subscriptionItem.id) - }} + onClick={() => + NiceModal.show(UnfollowDialog, { + subscriptionIds: [subscriptionItem.id], + subscriptionName: subscriptionItem.source_description, + id: subscriptionItem.id.toString(), + }) + } > Unfollow diff --git a/learning_resources/factories.py b/learning_resources/factories.py index a3996cf0fc..5252d2894e 100644 --- a/learning_resources/factories.py +++ b/learning_resources/factories.py @@ -213,6 +213,7 @@ class LearningResourceFactory(DjangoModelFactory): departments = factory.PostGeneration(_post_gen_departments) topics = factory.PostGeneration(_post_gen_topics) content_tags = factory.PostGeneration(_post_gen_tags) + completeness = 1 published = True delivery = factory.List(random.choices(LearningResourceDelivery.names())) # noqa: S311 professional = factory.LazyAttribute( diff --git a/learning_resources_search/api_test.py b/learning_resources_search/api_test.py index 86b810113b..50952da6c7 100644 --- a/learning_resources_search/api_test.py +++ b/learning_resources_search/api_test.py @@ -1445,6 +1445,7 @@ def test_execute_learn_search_for_learning_resource_query(opensearch): "is_learning_material", "resource_age_date", "featured_rank", + "is_incomplete_or_stale", ] }, } @@ -1921,6 +1922,7 @@ def test_execute_learn_search_with_script_score( "is_learning_material", "resource_age_date", "featured_rank", + "is_incomplete_or_stale", ] }, } @@ -2349,6 +2351,7 @@ def test_execute_learn_search_with_min_score(mocker, settings, opensearch): "is_learning_material", "resource_age_date", "featured_rank", + "is_incomplete_or_stale", ] }, } @@ -2559,6 +2562,7 @@ def test_execute_learn_search_for_content_file_query(opensearch): "is_learning_material", "resource_age_date", "featured_rank", + "is_incomplete_or_stale", ] }, } diff --git a/learning_resources_search/constants.py b/learning_resources_search/constants.py index 6bcc47e1f0..1487edb061 100644 --- a/learning_resources_search/constants.py +++ b/learning_resources_search/constants.py @@ -115,6 +115,7 @@ class FilterConfig: }, "free": {"type": "boolean"}, "is_learning_material": {"type": "boolean"}, + "is_incomplete_or_stale": {"type": "boolean"}, "delivery": { "type": "nested", "properties": { @@ -416,6 +417,7 @@ class FilterConfig: "is_learning_material", "resource_age_date", "featured_rank", + "is_incomplete_or_stale", ] LEARNING_RESOURCE_SEARCH_SORTBY_OPTIONS = { diff --git a/learning_resources_search/serializers.py b/learning_resources_search/serializers.py index b340605169..2addd6fa4d 100644 --- a/learning_resources_search/serializers.py +++ b/learning_resources_search/serializers.py @@ -125,6 +125,9 @@ def serialize_learning_resource_for_update( dict: The serialized and transformed resource data """ + STALENESS_CUTOFF = 2010 + COMPLETENESS_CUTOFF = 0.5 + serialized_data = LearningResourceSerializer(instance=learning_resource_obj).data if learning_resource_obj.resource_type == LearningResourceType.course.name: @@ -146,15 +149,22 @@ def serialize_learning_resource_for_update( else: featured_rank = None + resource_age_date = get_resource_age_date( + learning_resource_obj, serialized_data["resource_category"] + ) + + is_incomplete_or_stale = ( + resource_age_date and resource_age_date.year <= STALENESS_CUTOFF + ) or (learning_resource_obj.completeness < COMPLETENESS_CUTOFF) + return { "resource_relations": {"name": "resource"}, "created_on": learning_resource_obj.created_on, "is_learning_material": serialized_data["resource_category"] == LEARNING_MATERIAL_RESOURCE_CATEGORY, - "resource_age_date": get_resource_age_date( - learning_resource_obj, serialized_data["resource_category"] - ), + "resource_age_date": resource_age_date, "featured_rank": featured_rank, + "is_incomplete_or_stale": is_incomplete_or_stale, **serialized_data, } diff --git a/learning_resources_search/serializers_test.py b/learning_resources_search/serializers_test.py index 92eddbe150..43d97c6154 100644 --- a/learning_resources_search/serializers_test.py +++ b/learning_resources_search/serializers_test.py @@ -615,6 +615,7 @@ def test_serialize_bulk_learning_resources(mocker): "resource_age_date": mocker.ANY, "is_learning_material": mocker.ANY, "featured_rank": None, + "is_incomplete_or_stale": mocker.ANY, **LearningResourceSerializer(instance=resource).data, } @@ -637,30 +638,47 @@ def test_serialize_bulk_learning_resources(mocker): @pytest.mark.parametrize("is_professional", [True, False]) @pytest.mark.parametrize("no_price", [True, False]) @pytest.mark.parametrize("has_featured_rank", [True, False]) -def test_serialize_learning_resource_for_bulk( - mocker, resource_type, is_professional, no_price, has_featured_rank +@pytest.mark.parametrize("is_stale", [True, False]) +@pytest.mark.parametrize("is_incomplete", [True, False]) +def test_serialize_learning_resource_for_bulk( # noqa: PLR0913 + mocker, + resource_type, + is_professional, + no_price, + has_featured_rank, + is_stale, + is_incomplete, ): """ Test that serialize_program_for_bulk yields a valid LearningResourceSerializer for resource types other than "course" The "course" resource type is tested by `test_serialize_course_numbers_for_bulk` below. """ + completeness = 0.24 if is_incomplete else 0.75 resource = factories.LearningResourceFactory.create( - resource_type=resource_type, professional=is_professional, runs=[] + resource_type=resource_type, + professional=is_professional, + runs=[], + completeness=completeness, ) + LearningResourceRunFactory.create( learning_resource=resource, prices=[Decimal(0.00 if no_price else 1.00)] ) + + resource_age_date = datetime( + 2009 if is_stale else 2024, 1, 1, 1, 1, 1, 0, tzinfo=UTC + ) + + mocker.patch( + "learning_resources_search.serializers.get_resource_age_date", + return_value=resource_age_date, + ) free_dict = { "free": resource_type not in [LearningResourceType.program.name, LearningResourceType.course.name] or (no_price and not is_professional) } - mocker.patch( - "learning_resources_search.serializers.get_resource_age_date", - return_value=datetime(2024, 1, 1, 1, 1, 1, 0, tzinfo=UTC), - ) - if has_featured_rank: mocker.patch( "learning_resources_search.serializers.random", @@ -692,8 +710,9 @@ def test_serialize_learning_resource_for_bulk( "resource_relations": {"name": "resource"}, "created_on": resource.created_on, "is_learning_material": resource.resource_type not in ["course", "program"], - "resource_age_date": datetime(2024, 1, 1, 1, 1, 1, 0, tzinfo=UTC), + "resource_age_date": resource_age_date, "featured_rank": 3.4 if has_featured_rank else None, + "is_incomplete_or_stale": is_incomplete or is_stale, **free_dict, **LearningResourceSerializer(resource).data, } @@ -818,6 +837,7 @@ def test_serialize_course_numbers_for_bulk( "is_learning_material": False, "resource_age_date": datetime(2024, 1, 1, 1, 1, 1, 0, tzinfo=UTC), "featured_rank": None, + "is_incomplete_or_stale": False, **LearningResourceSerializer(resource).data, } expected_data["course"]["course_numbers"][0] = { diff --git a/learning_resources_search/tasks.py b/learning_resources_search/tasks.py index 6a187b9c00..dc2ea778f4 100644 --- a/learning_resources_search/tasks.py +++ b/learning_resources_search/tasks.py @@ -283,7 +283,7 @@ def send_subscription_emails(self, subscription_type, period="daily"): @app.task( acks_late=True, reject_on_worker_lost=True, - autoretry_for=(RetryError, SystemExit), + autoretry_for=(RetryError,), retry_backoff=True, rate_limit="600/m", ) @@ -301,8 +301,10 @@ def index_learning_resources(ids, resource_type, index_types): try: with wrap_retry_exception(*SEARCH_CONN_EXCEPTIONS): api.index_learning_resources(ids, resource_type, index_types) - except (RetryError, Ignore, SystemExit): + except (RetryError, Ignore): raise + except SystemExit as err: + raise RetryError(SystemExit.__name__) from err except: # noqa: E722 error = "index_courses threw an error" log.exception(error) @@ -362,7 +364,7 @@ def bulk_deindex_percolators(ids): @app.task( acks_late=True, reject_on_worker_lost=True, - autoretry_for=(RetryError, SystemExit), + autoretry_for=(RetryError,), retry_backoff=True, rate_limit="600/m", ) @@ -383,8 +385,10 @@ def bulk_index_percolate_queries(percolate_ids, index_types): PERCOLATE_INDEX_TYPE, index_types, ) - except (RetryError, Ignore, SystemExit): + except (RetryError, Ignore): raise + except SystemExit as err: + raise RetryError(SystemExit.__name__) from err except: # noqa: E722 error = "bulk_index_percolate_queries threw an error" log.exception(error) @@ -417,7 +421,7 @@ def index_course_content_files(course_ids, index_types): @app.task( acks_late=True, reject_on_worker_lost=True, - autoretry_for=(RetryError, SystemExit), + autoretry_for=(RetryError,), retry_backoff=True, rate_limit="600/m", ) @@ -441,8 +445,10 @@ def index_content_files( api.index_content_files( content_file_ids, learning_resource_id, index_types=index_types ) - except (RetryError, Ignore, SystemExit): + except (RetryError, Ignore): raise + except SystemExit as err: + raise RetryError(SystemExit.__name__) from err except: # noqa: E722 error = "index_content_files threw an error" log.exception(error) diff --git a/learning_resources_search/tasks_test.py b/learning_resources_search/tasks_test.py index a285f40f24..db178b727c 100644 --- a/learning_resources_search/tasks_test.py +++ b/learning_resources_search/tasks_test.py @@ -119,6 +119,18 @@ def raise_thing(): raise_thing() +def test_system_exit_retry(mocker): + """Task should raise a retry error on system exit""" + mocker.patch( + "learning_resources_search.tasks.wrap_retry_exception", side_effect=SystemExit + ) + with pytest.raises(Retry) as exc: + index_learning_resources.delay( + [1], COURSE_TYPE, IndexestoUpdate.current_index.value + ) + assert str(exc.value.args[1]) == "SystemExit" + + @pytest.mark.parametrize( "indexes", [["course"], ["program"]], diff --git a/main/settings.py b/main/settings.py index 46fcc3c304..61a6a9113c 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.20.2" +VERSION = "0.20.3" log = logging.getLogger()