fix: handle dupe course staff mgmt command #294
Conversation
varshamenon4
changed the title
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
varshamenon4
force-pushed
the
varshamenon4/fix-handle-dupe-course-staff-mgmt-command
branch
from
9ec3f02
to
e558965
Compare
| (User( | ||
| username=row.get('username'), | ||
| email=row.get('email'), | ||
| ) for row in reader[i:i + batch_size]), |
There was a problem hiding this comment.
Will this line generate indexing errors? For example, the i+batch_size > len(reader) condition?
There was a problem hiding this comment.
Good call, let me add testing/handling for that.
There was a problem hiding this comment.
So I did add tests to test that this still works. Also, I believe that this works because of this line: for i in range(0, len(reader), batch_size). Since this is incremented by batch_size, the line for row in reader[i:i + batch_size]) won't actually get to the index error. I actually originally repurposed this from some batching examples in the platform, so there are other examples of this pattern being used. See below:
- https://github.com/search?q=repo%3Aopenedx%2Fedx-platform%20%2B%20batch_size&type=code
- https://github.com/openedx/edx-platform/blob/8d0ada54c428b0c546119338d3661980ed4d23d2/scripts/user_retirement/utils/utils.py#L22
- https://github.com/openedx/edx-platform/blob/8d0ada54c428b0c546119338d3661980ed4d23d2/openedx/core/djangoapps/notifications/utils.py#L56
There was a problem hiding this comment.
okay this was throwing me off too. reader[i:i + batch_size] is actually slicing a subset reader into a new list. If the k in list[i:k] is outside the bounds it won't matter it only pulls the subset of indexes that actually exist up to k. eg [1,2,3,4][2:20] evaluates to [3,4]
| @@ -130,3 +130,32 @@ def test_dupe_user_csv(self): | |||
| call_command(self.command, f'--csv_path={csv.name}') | |||
| self._assert_user_and_role(username, email, self.course_role, self.course_id) | |||
| self._assert_user_and_role(username, email, self.course_role, course_id_2) | |||
|
|
|||
There was a problem hiding this comment.
Could you add a test case where you set the batch size to 2 and create like 3 course_staff users?
There was a problem hiding this comment.
Adding two cases, one where the batch size is larger than the number of roles, and one where the batch size is smaller.
varshamenon4
force-pushed
the
varshamenon4/fix-handle-dupe-course-staff-mgmt-command
branch
from
ef6be73
to
973b522
Compare
| call_command(self.command, f'--csv_path={csv.name}', '--batch_size=1') | ||
|
|
||
| def test_add_course_staff_with_batch_size_larger_than_list(self): |
There was a problem hiding this comment.
@schenedx added this test case as you'd suggested! Thanks for the feedback.
| @@ -130,3 +130,32 @@ def test_dupe_user_csv(self): | |||
| call_command(self.command, f'--csv_path={csv.name}') | |||
| self._assert_user_and_role(username, email, self.course_role, self.course_id) | |||
| self._assert_user_and_role(username, email, self.course_role, course_id_2) | |||
|
|
|||
There was a problem hiding this comment.
Adding two cases, one where the batch size is larger than the number of roles, and one where the batch size is smaller.
edx_exams/apps/core/management/commands/bulk_add_course_staff.py
Outdated
Show resolved
Hide resolved
varshamenon4
force-pushed
the
varshamenon4/fix-handle-dupe-course-staff-mgmt-command
branch
from
1e27f1a
to
e763e84
Compare
varshamenon4
deleted the
varshamenon4/fix-handle-dupe-course-staff-mgmt-command
branch
JIRA: COSMO-234
Description: This PR updates the course staff role management command to use ignore_conflicts to handle duplicate/already existing users and course staff roles. This is more efficient, and more readable. I did do some research, test locally and find other usages to confirm that ignore_conflicts is supported in our environment.
Merge checklist:
Post merge: