add to list dialog updates #1463

gumaerc · 2024-08-23T21:59:32Z

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/5275

Description (What does it do?)

This PR alters the Add to List dialog used to add Learning Resources to both Learning Paths and User Lists. Instead of adding the resource to lists immediately upon checking the box, the user's changes are saved upon clicking the new "Save" button. New API endpoints were added to accomplish this, sending all the lists that the resource should be a part of as one request. The styling of the modal was also updated to reflect the current designs in Figma.

Screenshots (if appropriate):

How can this be tested?

Spin up mit-learn on this branch
Make sure you are logged in as a user with is_staff or is_superuser set to true
Make sure you have backpopulated at least some data into your local database and generated a search index that will give you some search results
Go to the search page at http://localhost:8063/search
On any search result, click the bookmark button to display the Add to User List modal
Use the Create New List button to create a few lists
Check some boxes and click the x in the upper right of the modal
Verify that your changes were not saved
Open the modal again and this time select a few lists and hit the save button
Verify that the bookmark icon on the resource you selected is now highlighted
When you click the bookmark icon again, your selections should come back up
Repeat this process with the list button next to the bookmark button, which should open the Add to Learning Path dialog

ChristopherChudzicki

The new dialog is UI is looking good. I did notice a few functionality issues.

An issue with Featured courses ont he homepage
A permissioning issue... Currently, if one user updates the resource membership, that seems to be affecting all other users.

A suggestion for the first issue is in the review. Overall suggestion for the API below.

This PR adds a variety of new APIs:

GET, POST /api/v1/learning_resources/{learning_resource_id}/relationships/
GET, POST, PATCH, DELETE /api/v1/learning_resources/{learning_resource_id}/relationships/{id}
PATCH /api/v1/learning_resources/{learning_resource_id}/relationships/set_learning_path_relationships/
PATCH /api/v1/learning_resources/{learning_resource_id}/relationships/set_user_list_relationships/

Did you intend to add these all?

Request/Suggestion: IMO, we should only add two new API endpoints:

# more similar to what you have now
PATCH /api/v1/learning_resource/{resource_id}/userlists/
PATCH /api/v1/learning_resource/{resource_id}/userlists/

OR 

# needs resource_id in body
POST /api/v1/userlists/modify_items/
POST /api/v1/learningpaths/modify_items/

We'll have to check ownership for all userlists, and users should only be able to edit their own userlist. We definitely want tests for the permissioning / user specificity, too.

ChristopherChudzicki · 2024-08-26T18:27:02Z

frontends/api/src/hooks/learningResources/index.ts

+    onSuccess: (response, _vars) => {
+      queryClient.setQueriesData<PaginatedLearningResourceList>(
+        learningResources.featured({}).queryKey,
+        (old) => updateListParentsOnAdd(response.data, old),


Bug: On the homepage featured carousel list (applies to learning paths, too), try

added a resource w/o any userlists to a userlist

Expected: Bookmark becomes filled

Actual: no change

removing all userlists from a resource w/ userlist

Expected: Bookmark becomes unfilled

Actual: No change

I think the issue is that in the old way, response.data was a single UserListRelationship. Now it's Array< UserListRelationship>.

Probably easiest to tweak by changing changing updateListParentsOnAdd/Remove. Actually, maybe a new function updateListParentsOnChange... since your API returns ALL the list parents, I think it could be much simpler.

ChristopherChudzicki · 2024-08-26T18:32:21Z

frontends/api/src/hooks/learningResources/index.ts

+    },
+    onSettled: (_response, _err, vars) => {
+      invalidateResourceQueries(queryClient, vars.learning_resource_id, {
+        skipFeatured: true,


This should be false, and you can remove the onSucccess hook.

See

Filled vs Unfilled Bookmarks #1180 (comment)
(particularly the last bullet)

ChristopherChudzicki · 2024-08-26T18:44:45Z

learning_resources/views.py

+    @action(detail=False, methods=["patch"], name="Set User List Relationships")
+    def set_user_list_relationships(self, request, *args, **kwargs):  # noqa: ARG002


If you look at the generated OpenAPI spec, you'll see

Aside: This bug is why Typescript didn't realize the usage of updateListParentsOnAdd mentioned above was an error.

See tfranzel/drf-spectacular#190 (comment) for more on this, but the tldr is:

detail=False only affects the URL route. /set_user_list_relationships/ NOT set_user_list_relationships/:id

in particular, it has no affect on the logic of what the route returns, and DRF-spectacular can't predict that, so we need annotate it.

ChristopherChudzicki · 2024-08-27T18:23:53Z

learning_resources/views.py



+@extend_schema(
+    parameters=[


There are 3 parameters here... 2 are relevant for one endpoint, 2 are relevant for the other.

I would try moving the annotation to the methods themselves.

I also think you can skip the annotation for the path parameter. I think DRF-spectacular will pick that up automatically.

You're right in that you can usually omit the id path param, but in this case drf-spectacular mislabels it so I'm just specifying it to fix that:

"A unique integer value identifying this learning resource relationship." vs "id of the learning resource"

ChristopherChudzicki · 2024-08-27T18:25:34Z

learning_resources/views.py

+    permission_classes = (AnonymousAccessReadonlyPermission,)
+    filter_backends = [MultipleOptionsFilterBackend]
+    queryset = LearningResourceRelationship.objects.select_related("parent", "child")
+    http_method_names = ["patch"]


Minor comment: PUT still seems like a mildly better fit to me since this replaces the existing relationshisps (for that resource). When I briefly tried, I had an issue making PUT work with detail=false, but wince you're using detail=true, I think it would work.

ChristopherChudzicki · 2024-08-27T18:27:03Z

learning_resources/views.py

+    queryset = LearningResourceRelationship.objects.select_related("parent", "child")
+    http_method_names = ["patch"]
+
+    def get_serializer_class(self):


Do you know if get_serializer_class matters here? I would expect not since both of your methods call a specific serializer + have DRF-spectacular annotations. (More of a django question for my own education.)

Without it you get this error during the OpenAPI spec check:

/home/runner/work/mit-open/mit-open/learning_resources/views.py:381: Error [LearningResourceListRelationshipViewSet]: exception raised while getting serializer. Hint: Is get_serializer_class() returning None or is get_queryset() not working without a request? Ignoring the view for now. (Exception: 'LearningResourceListRelationshipViewSet' should either include a `serializer_class` attribute, or override the `get_serializer_class()` method.)

Ah, that makes sense. DRF-spectacular uses get_serializer_class for introspection, which it can't do with an explicitly coded serializer in the action method.

ChristopherChudzicki · 2024-08-27T18:32:04Z

learning_resources/views.py

+        if not is_learning_path_editor(request):
+            return Response(
+                {"error": "You do not have permission to edit learning paths"},
+                status=403,
+            )


I'm fairly certain you can pass permission_classes to an action decorator, so you could pass permission_classes=[HasLearningPathPermissions] to the decorator, but this is good too.

ChristopherChudzicki

The behavior and implementation all look good 👍 👍 I made one request regarding a test that I think should be added.

ChristopherChudzicki · 2024-08-28T13:46:05Z

learning_resources/views.py

+                child_id=learning_resource_id,
+                position=last_index + 1,
+            )
+        serializer = UserListRelationshipSerializer(current_relationships, many=True)


minor I guess you could do Serializer = self.get_serializer_class() in the action method rather than using it explicitly. Right now there could be a conflict between the two.

ChristopherChudzicki · 2024-08-28T14:40:39Z

learning_resources/views.py

+        Set User List relationships for a given Learning Resource
+        """
+        learning_resource_id = kwargs.get("pk")
+        user_list_ids = request.query_params.getlist("userlist_id")


Suggestion: Throw a 403 here if one of the userlists does not belong to user
I think something like

if (UserList.objects.filter(pk__in=user_list_ids).exclude(author=request.user).exists()): msg = f"You do not have permission to edit all of {user_list_ids}" raise PermissionDenied(msg)

and maybe add a test for the 403 error.

I think this is worthwhile and doesn't seem too complicated

ChristopherChudzicki · 2024-08-28T14:52:07Z

learning_resources/views_userlist_test.py

+
+
+def test_set_userlist_relationships(client, user):
+    """Test the userlists endpoint for setting multiple userlist relationships"""


Request: I think it's definitely worth testing "User X changes relationship does not affect other users' relationships" (earlier behavior in this PR.)

Suggestion: To this test, add something like

userlist_other_author = factories.UserListFactory.create() assert userlist_other_author.author != userlists[0].author # I like this for sanity 🤷 factories.UserListRelationshipFactory.create( parent=userlist_other_author, child=course.learning_resource ) # Then at the end, assert child still belongs to userlist_other_author

gumaerc · 2024-08-28T19:35:04Z

@ChristopherChudzicki all set

…rebuild position index

… that don't belong to you

ChristopherChudzicki · 2024-08-29T14:36:50Z

learning_resources/views_userlist_test.py

+    assert (
+        UserListRelationship.objects.filter(child=course.learning_resource).count() == 3
+    )


👍 👍 This fails if the parent__author=request.user filter is missing. That's what I was hoping for. Ty!

ChristopherChudzicki

👍

I did just notice that all of our dialogs use 28px on the bottom, but Dialog.tsx is using 40px on the bottom.

We could add to this PR. Otherwise I'll make a followup immediately after you merge this.

gumaerc added Work in Progress Needs Review An open Pull Request that is ready for review and removed Work in Progress labels Aug 23, 2024

ChristopherChudzicki self-assigned this Aug 26, 2024

gumaerc force-pushed the cg/add-to-user-list-checkboxes-and-form branch from a416f6d to 55a069f Compare August 26, 2024 20:02

ChristopherChudzicki requested changes Aug 26, 2024

View reviewed changes

ChristopherChudzicki added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Aug 26, 2024

ChristopherChudzicki reviewed Aug 27, 2024

View reviewed changes

gumaerc added Needs Review An open Pull Request that is ready for review and removed Waiting on author labels Aug 27, 2024

ChristopherChudzicki requested changes Aug 28, 2024

View reviewed changes

gumaerc added 16 commits August 28, 2024 18:00

WIP

722d73f

rework api endpoints and implement them

55aad9f

remove the modal after we're done with it

6336383

await setting new list parents before dismissing modal

18de402

remove circularprogress for now

5197513

fix tests

95ff8e5

try and fix warning about api parameter

c688787

try and fix python tests again

edaac97

remove id parameter because it broke everything

9eeb25b

remove extraneous API endpoints

02e15f8

simplify api signature

d51a33a

remove extraneous debugging line

3a210af

filter user list manipulation based on the current user and properly …

5e5a50a

…rebuild position index

restrict learning path editing to learning path editor

25b66e5

add reordering logic to learning path setting too

879a2e1

fix js tests

816877d

gumaerc added 15 commits August 28, 2024 18:01

fix schema annotations

fef3827

fix annotations and response types

fd7f945

override get_serializer_class to satisfy checks

ef893bc

regenerate spec

775d43d

fix invalidations

7147102

revise invalidations based on feedback

9f9a655

only specify parameters where necessary

f5f798b

fix backwards classes

432aa2b

add learning path permission class instead of manual check

c38a252

add some tests for the new API endpoints

c9af277

use get_serializer_class

128b7c5

properly use get_serializer_class

852129b

throw an unauthorized error if you try to assign courses to userlists…

15274d9

… that don't belong to you

fix test by checking for raising of permission error

037eba1

fix rebase issue yet again

4d70856

gumaerc force-pushed the cg/add-to-user-list-checkboxes-and-form branch from 735c4f3 to 4d70856 Compare August 28, 2024 22:02

gumaerc added 2 commits August 29, 2024 10:20

add a test to ensure accidental unassignment does not happen

904549d

pass in course to userlist assignment function

950dbc1

ChristopherChudzicki reviewed Aug 29, 2024

View reviewed changes

ChristopherChudzicki approved these changes Aug 29, 2024

View reviewed changes

gumaerc merged commit 87fc79b into main Aug 29, 2024

odlbot mentioned this pull request Aug 29, 2024

Release 0.17.12 #1483

Merged

3 tasks

ChristopherChudzicki mentioned this pull request Aug 29, 2024

Fix dialog spacing + reset AddToListDialog #1484

Merged

ChristopherChudzicki mentioned this pull request Nov 21, 2024

Refactor user lists and learning paths hooks out of learning resources #1842

Merged

rhysyngsun deleted the cg/add-to-user-list-checkboxes-and-form branch February 7, 2025 20:35

		@action(detail=False, methods=["patch"], name="Set User List Relationships")
		def set_user_list_relationships(self, request, args, *kwargs): # noqa: ARG002



		def test_set_userlist_relationships(client, user):
		"""Test the userlists endpoint for setting multiple userlist relationships"""



		@extend_schema(
		parameters=[

add to list dialog updates #1463

add to list dialog updates #1463

Uh oh!

Conversation

gumaerc commented Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What are the relevant tickets?

Description (What does it do?)

Screenshots (if appropriate):

How can this be tested?

Uh oh!

ChristopherChudzicki left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristopherChudzicki left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristopherChudzicki Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gumaerc commented Aug 28, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristopherChudzicki left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gumaerc commented Aug 23, 2024 •

edited

Loading

ChristopherChudzicki left a comment •

edited

Loading

ChristopherChudzicki Aug 28, 2024 •

edited

Loading