-
Notifications
You must be signed in to change notification settings - Fork 10
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
EDUCATOR-4618 | Prevent csv processor objects from having duplicate columns #31
Conversation
11d3a96
to
619ae6e
Compare
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.
Looks good. General question: when is it good to use mixin vs a common base class?
aaa6e01
to
a4dec92
Compare
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.
wrote these comments yesterday but forgot to hit send; sorry
tests/test_api.py
Outdated
@@ -216,3 +224,16 @@ def test_export(self, mocked_api, mocked_course_grade_factory): | |||
processor = api.InterventionCSVProcessor(course_id=self.course_id) | |||
rows = list(processor.get_iterator()) | |||
assert len(rows) == 2 | |||
|
|||
def test_columns_not_duplicated_during_init(self): |
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 would delete this test; we don't really have a user story for why this behavior would be important, since the interventions report isn't uploaded after being downloaded
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.
Ah, thanks, that's a good point.
bulk_grades/api.py
Outdated
self._course_key = CourseKey.from_string(self.course_id) | ||
self._subsection = UsageKey.from_string(self.subsection) if self.subsection else None | ||
self._subsections = self._get_graded_subsections( | ||
self._course_key, | ||
filter_subsection=self._subsection, | ||
filter_assignment_type=self.assignment_type | ||
filter_assignment_type=kwargs.get('assignment_type', None), |
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.
Especially when you've left a big honkin' comment about what super's init does, this seems a little unnecessary
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.
Thanks, that's some copypasta from the change in GradeCSVProcessor
.
a4dec92
to
3e8dd5e
Compare
https://openedx.atlassian.net/browse/EDUCATOR-4618
GradesCSVProcessor
andInterventionCSVProcessor
manage subsection column names in a sane way.pydocstyle
.Merge checklist:
Post merge:
finished.