From ce88aa1efaa286c8bcab801ab30ff6bc73eba2ba Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Thu, 24 Oct 2024 16:53:52 -0400 Subject: [PATCH 1/4] fix list duplication when editing lists --- learning_resources/serializers.py | 67 +++++++++++++++++++ learning_resources/serializers_test.py | 52 ++++++++++++++ learning_resources/views.py | 55 ++++++++++----- learning_resources/views_learningpath_test.py | 32 +++++++++ learning_resources/views_userlist_test.py | 27 ++++++++ 5 files changed, 217 insertions(+), 16 deletions(-) diff --git a/learning_resources/serializers.py b/learning_resources/serializers.py index 96e5576a7b..1db0a6c803 100644 --- a/learning_resources/serializers.py +++ b/learning_resources/serializers.py @@ -888,3 +888,70 @@ class Meta: model = models.UserListRelationship extra_kwargs = {"position": {"required": False}} exclude = COMMON_IGNORED_FIELDS + + +class SetLearningPathsRequestSerializer(serializers.Serializer): + """ + Validate request parameters for setting learning paths for a learning resource + """ + + learning_path_ids = serializers.ListField( + child=serializers.IntegerField(), allow_empty=False + ) + learning_resource_id = serializers.IntegerField() + + def validate_learning_resource_id(self, learning_resource_id): + """Ensure that the learning resource exists""" + try: + models.LearningResource.objects.get(id=learning_resource_id) + except models.LearningResource.DoesNotExist as dne: + msg = f"Invalid learning resource id: {learning_resource_id}" + raise ValidationError(msg) from dne + return learning_resource_id + + def validate_learning_path_ids(self, learning_path_ids): + """Ensure that the learning paths exist""" + valid_learning_path_ids = set( + models.LearningResource.objects.filter( + id__in=learning_path_ids, + resource_type=LearningResourceType.learning_path.name, + ).values_list("id", flat=True) + ) + missing = set(learning_path_ids).difference(valid_learning_path_ids) + if missing: + msg = f"Invalid learning path ids: {missing}" + raise ValidationError(msg) + return learning_path_ids + + +class SetUserListsRequestSerializer(serializers.Serializer): + """ + Validate request parameters for setting learning paths for a learning resource + """ + + user_list_ids = serializers.ListField( + child=serializers.IntegerField(), allow_empty=False + ) + learning_resource_id = serializers.IntegerField() + + def validate_learning_resource_id(self, learning_resource_id): + """Ensure that the learning resource exists""" + try: + models.LearningResource.objects.get(id=learning_resource_id) + except models.LearningResource.DoesNotExist as dne: + msg = f"Invalid learning resource id: {learning_resource_id}" + raise ValidationError(msg) from dne + return learning_resource_id + + def validate_user_list_ids(self, user_list_ids): + """Ensure that the learning paths exist""" + valid_user_list_ids = set( + models.UserList.objects.filter( + id__in=user_list_ids, + ).values_list("id", flat=True) + ) + missing = set(user_list_ids).difference(valid_user_list_ids) + if missing: + msg = f"Invalid learning path ids: {missing}" + raise ValidationError(msg) + return user_list_ids diff --git a/learning_resources/serializers_test.py b/learning_resources/serializers_test.py index 960c0bc622..b1222e81e5 100644 --- a/learning_resources/serializers_test.py +++ b/learning_resources/serializers_test.py @@ -550,3 +550,55 @@ def test_content_file_serializer(settings, expected_types, has_channels): ), }, ) + + +def test_set_learning_path_request_serializer(): + lists = factories.LearningPathFactory.create_batch(2) + resource = factories.LearningResourceFactory.create() + + serializer = serializers.SetLearningPathsRequestSerializer() + + data1 = { + "learning_path_ids": [ + str(lists[0].learning_resource.id), + lists[1].learning_resource.id, + ], + "learning_resource_id": str(resource.id), + } + assert serializer.to_internal_value(data1) == { + "learning_path_ids": [ + lists[0].learning_resource.id, + lists[1].learning_resource.id, + ], + "learning_resource_id": resource.id, + } + + invalid = serializers.SetLearningPathsRequestSerializer( + data={"learning_path_ids": [1, 2], "learning_resource_id": 3} + ) + assert invalid.is_valid() is False + assert "learning_path_ids" in invalid.errors + assert "learning_resource_id" in invalid.errors + + +def test_set_userlist_request_serializer(): + lists = factories.UserListFactory.create_batch(2) + resource = factories.LearningResourceFactory.create() + + serializer = serializers.SetUserListsRequestSerializer() + + data1 = { + "user_list_ids": [str(lists[0].id), lists[1].id], + "learning_resource_id": str(resource.id), + } + assert serializer.to_internal_value(data1) == { + "user_list_ids": [lists[0].id, lists[1].id], + "learning_resource_id": resource.id, + } + + invalid = serializers.SetUserListsRequestSerializer( + data={"user_list_ids": [1, 2], "learning_resource_id": 3} + ) + assert invalid.is_valid() is False + assert "user_list_ids" in invalid.errors + assert "learning_resource_id" in invalid.errors diff --git a/learning_resources/views.py b/learning_resources/views.py index 683742d110..25381e6b3e 100644 --- a/learning_resources/views.py +++ b/learning_resources/views.py @@ -74,6 +74,8 @@ PodcastEpisodeResourceSerializer, PodcastResourceSerializer, ProgramResourceSerializer, + SetLearningPathsRequestSerializer, + SetUserListsRequestSerializer, UserListRelationshipSerializer, UserListSerializer, VideoPlaylistResourceSerializer, @@ -433,8 +435,15 @@ def userlists(self, request, *args, **kwargs): # noqa: ARG002 """ Set User List relationships for a given Learning Resource """ - learning_resource_id = kwargs.get("pk") - user_list_ids = request.query_params.getlist("userlist_id") + req_data = SetUserListsRequestSerializer().to_internal_value( + { + "user_list_ids": request.query_params.getlist("user_list_id"), + "learning_resource_id": kwargs.get("pk"), + } + ) + learning_resource_id = req_data["learning_resource_id"] + user_list_ids = req_data["user_list_ids"] + if ( UserList.objects.filter(pk__in=user_list_ids) .exclude(author=request.user) @@ -446,6 +455,8 @@ def userlists(self, request, *args, **kwargs): # noqa: ARG002 parent__author=request.user, child_id=learning_resource_id ) current_relationships.exclude(parent_id__in=user_list_ids).delete() + current_parent_lists = current_relationships.values_list("parent_id", flat=True) + for userlist_id in user_list_ids: last_index = 0 for index, relationship in enumerate( @@ -456,11 +467,12 @@ def userlists(self, request, *args, **kwargs): # noqa: ARG002 relationship.position = index relationship.save() last_index = index - UserListRelationship.objects.create( - parent_id=userlist_id, - child_id=learning_resource_id, - position=last_index + 1, - ) + if userlist_id not in list(current_parent_lists): + UserListRelationship.objects.create( + parent_id=userlist_id, + child_id=learning_resource_id, + position=last_index + 1, + ) SerializerClass = self.get_serializer_class() serializer = SerializerClass(current_relationships, many=True) return Response(serializer.data) @@ -489,13 +501,22 @@ def learning_paths(self, request, *args, **kwargs): # noqa: ARG002 """ Set Learning Path relationships for a given Learning Resource """ - learning_resource_id = kwargs.get("pk") - learning_path_ids = request.query_params.getlist("learning_path_id") + req_data = SetLearningPathsRequestSerializer().to_internal_value( + { + "learning_path_ids": request.query_params.getlist("learning_path_id"), + "learning_resource_id": kwargs.get("pk"), + } + ) + learning_resource_id = req_data["learning_resource_id"] + learning_path_ids = req_data["learning_path_ids"] current_relationships = LearningResourceRelationship.objects.filter( child_id=learning_resource_id ) current_relationships.exclude(parent_id__in=learning_path_ids).delete() - for learning_path_id in learning_path_ids: + current_parent_lists = current_relationships.values_list("parent_id", flat=True) + + for learning_path_id_str in learning_path_ids: + learning_path_id = int(learning_path_id_str) last_index = 0 for index, relationship in enumerate( LearningResourceRelationship.objects.filter( @@ -505,12 +526,14 @@ def learning_paths(self, request, *args, **kwargs): # noqa: ARG002 relationship.position = index relationship.save() last_index = index - LearningResourceRelationship.objects.create( - parent_id=learning_path_id, - child_id=learning_resource_id, - relation_type=LearningResourceRelationTypes.LEARNING_PATH_ITEMS, - position=last_index + 1, - ) + + if learning_path_id not in list(current_parent_lists): + LearningResourceRelationship.objects.create( + parent_id=learning_path_id, + child_id=learning_resource_id, + relation_type=LearningResourceRelationTypes.LEARNING_PATH_ITEMS, + position=last_index + 1, + ) SerializerClass = self.get_serializer_class() serializer = SerializerClass(current_relationships, many=True) return Response(serializer.data) diff --git a/learning_resources/views_learningpath_test.py b/learning_resources/views_learningpath_test.py index 9d479771dc..1d61b8665f 100644 --- a/learning_resources/views_learningpath_test.py +++ b/learning_resources/views_learningpath_test.py @@ -425,3 +425,35 @@ def test_set_learning_path_relationships(client, staff_user): assert not course.learning_resource.learning_path_parents.filter( parent__id=previous_learning_path.learning_resource.id ).exists() + + +def test_adding_to_learning_path_not_effect_existing_membership(client, staff_user): + course = factories.CourseFactory.create() + + existing_parent = factories.LearningPathFactory.create(author=staff_user) + factories.LearningPathRelationshipFactory.create( + parent=existing_parent.learning_resource, child=course.learning_resource + ) + new_additional_parent = factories.LearningPathFactory.create(author=staff_user) + + prev_parent_count = existing_parent.learning_resource.resources.count() + new_additional_parent_count = ( + new_additional_parent.learning_resource.resources.count() + ) + + url = reverse( + "lr:v1:learning_resource_relationships_api-learning-paths", + args=[course.learning_resource.id], + ) + client.force_login(staff_user) + lps = [existing_parent, new_additional_parent] + resp = client.patch( + f"{url}?{"".join([f"learning_path_id={lp.learning_resource.id}&" for lp in lps])}" + ) + + assert resp.status_code == 200 + assert prev_parent_count == existing_parent.learning_resource.resources.count() + assert ( + new_additional_parent_count + 1 + == new_additional_parent.learning_resource.resources.count() + ) diff --git a/learning_resources/views_userlist_test.py b/learning_resources/views_userlist_test.py index d404b19741..46e65d7a62 100644 --- a/learning_resources/views_userlist_test.py +++ b/learning_resources/views_userlist_test.py @@ -350,3 +350,30 @@ def assign_userlists(course, userlists): assert ( UserListRelationship.objects.filter(child=course.learning_resource).count() == 3 ) + + +def test_adding_to_userlist_not_effect_existing_membership(client, user): + course = factories.CourseFactory.create() + + existing_parent = factories.UserListFactory.create(author=user) + factories.UserListRelationshipFactory.create( + parent=existing_parent, child=course.learning_resource + ) + new_additional_parent = factories.UserListFactory.create(author=user) + + prev_parent_count = existing_parent.resources.count() + new_additional_parent_count = new_additional_parent.resources.count() + + url = reverse( + "lr:v1:learning_resource_relationships_api-userlists", + args=[course.learning_resource.id], + ) + client.force_login(user) + lists = [existing_parent, new_additional_parent] + resp = client.patch( + f"{url}?{"".join([f"user_list_id={userlist.id}&" for userlist in lists])}" + ) + + assert resp.status_code == 200 + assert prev_parent_count == existing_parent.resources.count() + assert new_additional_parent_count + 1 == new_additional_parent.resources.count() From 4ce5acbd06826f6752e840be6182fbe15128cd99 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Thu, 24 Oct 2024 16:57:35 -0400 Subject: [PATCH 2/4] add some comments --- learning_resources/views.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/learning_resources/views.py b/learning_resources/views.py index 25381e6b3e..d2ab7e92b3 100644 --- a/learning_resources/views.py +++ b/learning_resources/views.py @@ -454,11 +454,14 @@ def userlists(self, request, *args, **kwargs): # noqa: ARG002 current_relationships = UserListRelationship.objects.filter( parent__author=request.user, child_id=learning_resource_id ) + + # Remove the resource from lists it WAS in before but is not in now current_relationships.exclude(parent_id__in=user_list_ids).delete() current_parent_lists = current_relationships.values_list("parent_id", flat=True) for userlist_id in user_list_ids: last_index = 0 + # re-number the positions for surviving items for index, relationship in enumerate( UserListRelationship.objects.filter( parent__author=request.user, parent__id=userlist_id @@ -467,6 +470,7 @@ def userlists(self, request, *args, **kwargs): # noqa: ARG002 relationship.position = index relationship.save() last_index = index + # Add new items as necessary if userlist_id not in list(current_parent_lists): UserListRelationship.objects.create( parent_id=userlist_id, @@ -512,12 +516,14 @@ def learning_paths(self, request, *args, **kwargs): # noqa: ARG002 current_relationships = LearningResourceRelationship.objects.filter( child_id=learning_resource_id ) + # Remove the resource from lists it WAS in before but is not in now current_relationships.exclude(parent_id__in=learning_path_ids).delete() current_parent_lists = current_relationships.values_list("parent_id", flat=True) for learning_path_id_str in learning_path_ids: learning_path_id = int(learning_path_id_str) last_index = 0 + # re-number the positions for surviving items for index, relationship in enumerate( LearningResourceRelationship.objects.filter( parent__id=learning_path_id @@ -527,6 +533,7 @@ def learning_paths(self, request, *args, **kwargs): # noqa: ARG002 relationship.save() last_index = index + # Add new items as necessary if learning_path_id not in list(current_parent_lists): LearningResourceRelationship.objects.create( parent_id=learning_path_id, From 03370f1cb11bc675aed8f2c342b4c209ac1168ed Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Fri, 25 Oct 2024 10:14:20 -0400 Subject: [PATCH 3/4] fix param names, allow empty --- learning_resources/serializers.py | 16 ++++++++-------- learning_resources/serializers_test.py | 8 ++++---- learning_resources/views.py | 11 +++++------ learning_resources/views_userlist_test.py | 2 +- 4 files changed, 18 insertions(+), 19 deletions(-) diff --git a/learning_resources/serializers.py b/learning_resources/serializers.py index 1db0a6c803..4508867713 100644 --- a/learning_resources/serializers.py +++ b/learning_resources/serializers.py @@ -896,7 +896,7 @@ class SetLearningPathsRequestSerializer(serializers.Serializer): """ learning_path_ids = serializers.ListField( - child=serializers.IntegerField(), allow_empty=False + child=serializers.IntegerField(), allow_empty=True ) learning_resource_id = serializers.IntegerField() @@ -929,8 +929,8 @@ class SetUserListsRequestSerializer(serializers.Serializer): Validate request parameters for setting learning paths for a learning resource """ - user_list_ids = serializers.ListField( - child=serializers.IntegerField(), allow_empty=False + userlist_ids = serializers.ListField( + child=serializers.IntegerField(), allow_empty=True ) learning_resource_id = serializers.IntegerField() @@ -943,15 +943,15 @@ def validate_learning_resource_id(self, learning_resource_id): raise ValidationError(msg) from dne return learning_resource_id - def validate_user_list_ids(self, user_list_ids): + def validate_userlist_ids(self, userlist_ids): """Ensure that the learning paths exist""" - valid_user_list_ids = set( + valid_userlist_ids = set( models.UserList.objects.filter( - id__in=user_list_ids, + id__in=userlist_ids, ).values_list("id", flat=True) ) - missing = set(user_list_ids).difference(valid_user_list_ids) + missing = set(userlist_ids).difference(valid_userlist_ids) if missing: msg = f"Invalid learning path ids: {missing}" raise ValidationError(msg) - return user_list_ids + return userlist_ids diff --git a/learning_resources/serializers_test.py b/learning_resources/serializers_test.py index b1222e81e5..7f0b78484a 100644 --- a/learning_resources/serializers_test.py +++ b/learning_resources/serializers_test.py @@ -588,17 +588,17 @@ def test_set_userlist_request_serializer(): serializer = serializers.SetUserListsRequestSerializer() data1 = { - "user_list_ids": [str(lists[0].id), lists[1].id], + "userlist_ids": [str(lists[0].id), lists[1].id], "learning_resource_id": str(resource.id), } assert serializer.to_internal_value(data1) == { - "user_list_ids": [lists[0].id, lists[1].id], + "userlist_ids": [lists[0].id, lists[1].id], "learning_resource_id": resource.id, } invalid = serializers.SetUserListsRequestSerializer( - data={"user_list_ids": [1, 2], "learning_resource_id": 3} + data={"userlist_ids": [1, 2], "learning_resource_id": 3} ) assert invalid.is_valid() is False - assert "user_list_ids" in invalid.errors + assert "userlist_ids" in invalid.errors assert "learning_resource_id" in invalid.errors diff --git a/learning_resources/views.py b/learning_resources/views.py index d2ab7e92b3..9e1b699bc1 100644 --- a/learning_resources/views.py +++ b/learning_resources/views.py @@ -437,15 +437,14 @@ def userlists(self, request, *args, **kwargs): # noqa: ARG002 """ req_data = SetUserListsRequestSerializer().to_internal_value( { - "user_list_ids": request.query_params.getlist("user_list_id"), + "userlist_ids": request.query_params.getlist("userlist_id"), "learning_resource_id": kwargs.get("pk"), } ) learning_resource_id = req_data["learning_resource_id"] - user_list_ids = req_data["user_list_ids"] - + userlist_ids = req_data["userlist_ids"] if ( - UserList.objects.filter(pk__in=user_list_ids) + UserList.objects.filter(pk__in=userlist_ids) .exclude(author=request.user) .exists() ): @@ -456,10 +455,10 @@ def userlists(self, request, *args, **kwargs): # noqa: ARG002 ) # Remove the resource from lists it WAS in before but is not in now - current_relationships.exclude(parent_id__in=user_list_ids).delete() + current_relationships.exclude(parent_id__in=userlist_ids).delete() current_parent_lists = current_relationships.values_list("parent_id", flat=True) - for userlist_id in user_list_ids: + for userlist_id in userlist_ids: last_index = 0 # re-number the positions for surviving items for index, relationship in enumerate( diff --git a/learning_resources/views_userlist_test.py b/learning_resources/views_userlist_test.py index 46e65d7a62..50b6099f5f 100644 --- a/learning_resources/views_userlist_test.py +++ b/learning_resources/views_userlist_test.py @@ -371,7 +371,7 @@ def test_adding_to_userlist_not_effect_existing_membership(client, user): client.force_login(user) lists = [existing_parent, new_additional_parent] resp = client.patch( - f"{url}?{"".join([f"user_list_id={userlist.id}&" for userlist in lists])}" + f"{url}?{"".join([f"userlist_id={userlist.id}&" for userlist in lists])}" ) assert resp.status_code == 200 From f6a3a7e4c747468f4ce4f0869a63dccf6f0c35ba Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Fri, 25 Oct 2024 11:39:19 -0400 Subject: [PATCH 4/4] docstrings, serializer baseclass --- learning_resources/serializers.py | 32 +++++++++---------- learning_resources/serializers_test.py | 2 ++ learning_resources/views_learningpath_test.py | 4 +++ learning_resources/views_userlist_test.py | 4 +++ 4 files changed, 25 insertions(+), 17 deletions(-) diff --git a/learning_resources/serializers.py b/learning_resources/serializers.py index 4508867713..401f4f52d2 100644 --- a/learning_resources/serializers.py +++ b/learning_resources/serializers.py @@ -890,14 +890,12 @@ class Meta: exclude = COMMON_IGNORED_FIELDS -class SetLearningPathsRequestSerializer(serializers.Serializer): +class BaseRelationshipRequestSerializer(serializers.Serializer): """ - Validate request parameters for setting learning paths for a learning resource + Base class for validating requests that set relationships between + learning resources """ - learning_path_ids = serializers.ListField( - child=serializers.IntegerField(), allow_empty=True - ) learning_resource_id = serializers.IntegerField() def validate_learning_resource_id(self, learning_resource_id): @@ -909,6 +907,16 @@ def validate_learning_resource_id(self, learning_resource_id): raise ValidationError(msg) from dne return learning_resource_id + +class SetLearningPathsRequestSerializer(BaseRelationshipRequestSerializer): + """ + Validate request parameters for setting learning paths for a learning resource + """ + + learning_path_ids = serializers.ListField( + child=serializers.IntegerField(), allow_empty=True + ) + def validate_learning_path_ids(self, learning_path_ids): """Ensure that the learning paths exist""" valid_learning_path_ids = set( @@ -924,24 +932,14 @@ def validate_learning_path_ids(self, learning_path_ids): return learning_path_ids -class SetUserListsRequestSerializer(serializers.Serializer): +class SetUserListsRequestSerializer(BaseRelationshipRequestSerializer): """ - Validate request parameters for setting learning paths for a learning resource + Validate request parameters for setting userlist for a learning resource """ userlist_ids = serializers.ListField( child=serializers.IntegerField(), allow_empty=True ) - learning_resource_id = serializers.IntegerField() - - def validate_learning_resource_id(self, learning_resource_id): - """Ensure that the learning resource exists""" - try: - models.LearningResource.objects.get(id=learning_resource_id) - except models.LearningResource.DoesNotExist as dne: - msg = f"Invalid learning resource id: {learning_resource_id}" - raise ValidationError(msg) from dne - return learning_resource_id def validate_userlist_ids(self, userlist_ids): """Ensure that the learning paths exist""" diff --git a/learning_resources/serializers_test.py b/learning_resources/serializers_test.py index 7f0b78484a..4c49eee142 100644 --- a/learning_resources/serializers_test.py +++ b/learning_resources/serializers_test.py @@ -553,6 +553,7 @@ def test_content_file_serializer(settings, expected_types, has_channels): def test_set_learning_path_request_serializer(): + """Test serializer for setting learning path relationships""" lists = factories.LearningPathFactory.create_batch(2) resource = factories.LearningResourceFactory.create() @@ -582,6 +583,7 @@ def test_set_learning_path_request_serializer(): def test_set_userlist_request_serializer(): + """Test serializer for setting userlist relationships""" lists = factories.UserListFactory.create_batch(2) resource = factories.LearningResourceFactory.create() diff --git a/learning_resources/views_learningpath_test.py b/learning_resources/views_learningpath_test.py index 1d61b8665f..51709bad99 100644 --- a/learning_resources/views_learningpath_test.py +++ b/learning_resources/views_learningpath_test.py @@ -428,6 +428,10 @@ def test_set_learning_path_relationships(client, staff_user): def test_adding_to_learning_path_not_effect_existing_membership(client, staff_user): + """ + Given L1 (existing parent), L2 (new parent), and R (resource), + test that adding R to L2 does not affect L1. + """ course = factories.CourseFactory.create() existing_parent = factories.LearningPathFactory.create(author=staff_user) diff --git a/learning_resources/views_userlist_test.py b/learning_resources/views_userlist_test.py index 50b6099f5f..ff8679b09d 100644 --- a/learning_resources/views_userlist_test.py +++ b/learning_resources/views_userlist_test.py @@ -353,6 +353,10 @@ def assign_userlists(course, userlists): def test_adding_to_userlist_not_effect_existing_membership(client, user): + """ + Given L1 (existing parent), L2 (new parent), and R (resource), + test that adding R to L2 does not affect L1. + """ course = factories.CourseFactory.create() existing_parent = factories.UserListFactory.create(author=user)