-
Notifications
You must be signed in to change notification settings - Fork 3
fix list duplication when editing lists #1749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| ) | ||
|
|
||
| # Add new items as necessary | ||
| if learning_path_id not in list(current_parent_lists): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spend a while trying to debug why my conditional was not working, and it ended up being because (originally) learning_path_id was a string, whereas current_parent_lists was a list of integers.
That motivated me to make the two new serializers for validating request data.
2fa2a07 to
20a6ddf
Compare
20a6ddf to
03370f1
Compare
mbertrand
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great, just add some missing docstrings to the tests, and, only if you feel like it, make the suggested change to reduce code duplication.
learning_resources/serializers.py
Outdated
| ) | ||
| learning_resource_id = serializers.IntegerField() | ||
|
|
||
| def validate_learning_resource_id(self, learning_resource_id): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal, but to avoid some code duplication, you could have a BaseListItemsRequestsSerializer with the validate_learrning_resource_id function defined, then have SetUserRequestSerializer and SetLearningPathRequestSerializer inherit from that.
| ) | ||
|
|
||
|
|
||
| def test_set_learning_path_request_serializer(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add. docstring
| assert "learning_resource_id" in invalid.errors | ||
|
|
||
|
|
||
| def test_set_userlist_request_serializer(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add. docstring
| ) | ||
|
|
||
|
|
||
| def test_adding_to_userlist_not_effect_existing_membership(client, user): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing docstring
What are the relevant tickets?
Fixes https://github.com/mitodl/hq/issues/5858
Description (What does it do?)
When assigning resources to lists (learning paths, userlists), the resource was being added again to lists it was already in. This fixes that.
How can this be tested?
main, L1 now has two copies of the resource