[WIP]Optimize gradebook csv export/import #456

dzhuang · 2018-02-09T15:41:53Z

Add checks for new introduced settings items.
Add tests.

Features:

Allow define user prefered csv encoding. (So that csv containing non-latin characters can be opened by Excel directly without convertion).
Allow replace None in gradebook to 0.
Allow customized output fields in csv,
Allow enforce grades floor and ceiling for grades.
Enable round for grades.
Allow remove instructors and TAs from the output.

codecov · 2018-02-09T15:47:38Z

Codecov Report

Merging #456 into master will increase coverage by 0.95%.
The diff coverage is 99.03%.

@@            Coverage Diff             @@
##           master     #456      +/-   ##
==========================================
+ Coverage   67.36%   68.31%   +0.95%     
==========================================
  Files          46       46              
  Lines       10933    11119     +186     
  Branches     2041     2087      +46     
==========================================
+ Hits         7365     7596     +231     
+ Misses       2936     2893      -43     
+ Partials      632      630       -2

Impacted Files	Coverage Δ
relate/checks.py	`92.57% <100%> (+0.12%)`	⬆️
course/models.py	`84.1% <100%> (+0.12%)`	⬆️
course/grades.py	`71.34% <98.83%> (+3%)`	⬆️
course/utils.py	`73.03% <99.07%> (+4.86%)`	⬆️
course/page/code.py	`72.49% <0%> (+0.51%)`	⬆️
course/auth.py	`81.12% <0%> (+7.15%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22530d2...8f22e8f. Read the comment docs.

dzhuang · 2018-02-12T19:14:00Z

@inducer How do you feel about this PR? I am not confident in the naming. Thanks.

inducer · 2018-02-13T17:25:43Z

course/grades.py

+            initial=0,
+            min_value=0,
+            max_value=100 * MAX_EXTRA_CREDIT_FACTOR,
+            label=_("Minimum points"),


Could use a help text. Don't know what it does.

inducer · 2018-02-13T17:25:52Z

course/grades.py

+            min_value=0,
+            initial=0,
+            max_value=2,
+            label=_("Round Digits"),


Or maybe change label to "Round point counts to N digits" and "Round percentages to N digits". Also, why max out at 2?

I previously set that to 4, and I found (in tests) the value are all rounded to 2 digits already, had thought that's preset behavior. Since you commented on this, I'll check that.

inducer · 2018-02-13T17:27:19Z

course/grades.py

+            required=False,
+            initial=False,
+            label=_("Use 0 point for grade state `NONE`, which possibly "
+                    "means the opptunity had not been graded."),


Why "possibly"?

That's why I opened #457 . I think it's quite confusing. both not-graded-yet and graded can result in None. Also, those participations who didn't open a session for the grading opportunity also get None.

I just opened an issue #461 about NONE .

inducer · 2018-02-13T17:29:01Z

course/grades.py

+        self.fields["exclude_tas"] = forms.BooleanField(
+            required=False,
+            initial=True,
+            label=_("Exclude Grades of Teaching assistants"),


"Teaching assistant" and "Instructor" are not concepts in Relate. That means you may not use them in any UI. (They used to be, but they no longer are.) Right now, there are just arbitrarily-defined roles. The only thing available is a "permission" that says whether a role's/participant's grades are to be included in the grades.

inducer · 2018-02-13T17:29:54Z

course/utils.py

@@ -1320,4 +1324,215 @@ def _render_notebook_from_source(

 # }}}

+
+class RelateCsvHandler(object):
+    # Tests ref tests.template_backends.test_django.DjangoTemplatesTests


I have no idea what this does and why it belongs here.

Why is this not in checks.py?

It could stand renaming to RelateCSVSettingsValidator or some such maybe?

I have no idea what this does and why it belongs here.

It's something that make the a csv configuration (currently for csv export) from settings.RelateCSVSetting, by check and add some default value if not configured, and packaged some methods like generate the user attribute choice fields (in the ExportCsvForm) for csv export.

Why is this not in checks.py?

As mentioned above, it's functionalities are not limited to check only, do you prefer to put that in checks.py? Won't it look weird to import a function from checks.py?.

It could stand renaming to RelateCSVSettingsValidator or some such maybe?

What about RelateCSVSettingsInitializer

docstring added.

dzhuang · 2018-02-14T10:39:23Z

Updated.

inducer · 2018-02-15T20:20:25Z

course/grades.py

        )

+csv_settings = RelateCSVSettingsInitializer()


Can we do without this global variable somehow?

(I'd much prefer a function, for example.)

In fact, the csv_setting object created here is intended to be an app global object, since I am creating the object with its properties to be cached_properties, each of which need to be calculated only once.

However, I agree it's not elegant to use it here in this way. What about initialize the csv_settings object, i.e.,

csv_settings = RelateCSVSettingsInitializer()

in utils.py, and then import it in grades.py. Will that be better?

inducer · 2018-02-15T20:22:18Z

course/grades.py

+            label=_("0 percentage for graded 'NONE'"),
+            help_text=string_concat(
+                _("Use 0 percentage for graded opportunities which have "
+                  "`NONE` points."),


"NONE points" reads awkwardly. Maybe "that do not currently have a defined point count."

That's fair. I think then we should notice user if False, it will result in NONE in the csv.

inducer · 2018-02-15T20:23:51Z

course/grades.py

+            help_text=_(
+                "If selected, grades of course staff (participations who "
+                "have '%(permission)s' permissions) will be excluded."
+                % {"permission": pperm.assign_grade}),


Why not base this on the existing pperm.included_in_grade_statistics?

That's fair.

dzhuang · 2018-02-16T14:52:06Z

I felt this PR should be WIP before #461 is resolved, so that we can full test grade status and gradebook behavior.

dzhuang force-pushed the allow_user_defined_csv_codec branch from f204428 to 9d8bc55 Compare February 9, 2018 16:15

dzhuang added local_settings items added need tests labels Feb 9, 2018

dzhuang force-pushed the allow_user_defined_csv_codec branch 20 times, most recently from f6d99b5 to 7c54fc0 Compare February 12, 2018 18:42

dzhuang removed the need tests label Feb 12, 2018

dzhuang changed the title ~~[WIP] Optimize gradebook csv exportation~~ Optimize gradebook csv exportation Feb 12, 2018

dzhuang force-pushed the allow_user_defined_csv_codec branch from 7c54fc0 to ded9cbe Compare February 12, 2018 18:54

dzhuang changed the title ~~Optimize gradebook csv exportation~~ Optimize gradebook csv export/import Feb 12, 2018

dzhuang force-pushed the allow_user_defined_csv_codec branch from ded9cbe to 99e8f6f Compare February 12, 2018 19:03

dzhuang force-pushed the allow_user_defined_csv_codec branch 5 times, most recently from 842d8ca to 2c6a17f Compare February 13, 2018 05:12

Improve & test gradebook csv export/import.

d08c457

dzhuang force-pushed the allow_user_defined_csv_codec branch from 2c6a17f to d08c457 Compare February 13, 2018 06:40

inducer reviewed Feb 13, 2018

View reviewed changes

dzhuang mentioned this pull request Feb 14, 2018

More informative grade status #461

Open

dzhuang added a commit to dzhuang/relate that referenced this pull request Feb 14, 2018

Better descriptions and namings for inducer#456.

af9bd54

Better descriptions and namings for inducer#456.

8f22e8f

dzhuang force-pushed the allow_user_defined_csv_codec branch from af9bd54 to 8f22e8f Compare February 14, 2018 10:03

inducer reviewed Feb 15, 2018

View reviewed changes

dzhuang changed the title ~~Optimize gradebook csv export/import~~ [WIP]Optimize gradebook csv export/import Feb 16, 2018

dzhuang force-pushed the master branch from bc55a62 to d8817f3 Compare February 18, 2018 10:12

dzhuang force-pushed the master branch from 4efa222 to b780917 Compare March 4, 2018 19:18

dzhuang force-pushed the master branch from d08b9e0 to 8b30595 Compare April 20, 2018 20:53

Base automatically changed from master to main March 8, 2021 02:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP]Optimize gradebook csv export/import #456

[WIP]Optimize gradebook csv export/import #456

dzhuang commented Feb 9, 2018 •

edited

Loading

codecov bot commented Feb 9, 2018 •

edited

Loading

dzhuang commented Feb 12, 2018 •

edited

Loading

inducer Feb 13, 2018

dzhuang Feb 14, 2018

inducer Feb 13, 2018

inducer Feb 13, 2018

dzhuang Feb 14, 2018 •

edited

Loading

inducer Feb 13, 2018

dzhuang Feb 14, 2018 •

edited

Loading

dzhuang Feb 14, 2018

inducer Feb 13, 2018

dzhuang Feb 14, 2018

inducer Feb 13, 2018

inducer Feb 13, 2018

inducer Feb 13, 2018

dzhuang Feb 14, 2018 •

edited

Loading

dzhuang Feb 14, 2018

dzhuang Feb 14, 2018

dzhuang Feb 14, 2018

dzhuang commented Feb 14, 2018

inducer Feb 15, 2018

inducer Feb 15, 2018

dzhuang Feb 16, 2018 •

edited

Loading

inducer Feb 15, 2018

dzhuang Feb 16, 2018 •

edited

Loading

inducer Feb 15, 2018

dzhuang Feb 16, 2018

dzhuang commented Feb 16, 2018

[WIP]Optimize gradebook csv export/import #456

Are you sure you want to change the base?

[WIP]Optimize gradebook csv export/import #456

Conversation

dzhuang commented Feb 9, 2018 • edited Loading

codecov bot commented Feb 9, 2018 • edited Loading

Codecov Report

dzhuang commented Feb 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzhuang Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzhuang Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzhuang Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzhuang commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzhuang Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzhuang Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzhuang commented Feb 16, 2018

dzhuang commented Feb 9, 2018 •

edited

Loading

codecov bot commented Feb 9, 2018 •

edited

Loading

dzhuang commented Feb 12, 2018 •

edited

Loading

dzhuang Feb 14, 2018 •

edited

Loading

dzhuang Feb 14, 2018 •

edited

Loading

dzhuang Feb 14, 2018 •

edited

Loading

dzhuang Feb 16, 2018 •

edited

Loading

dzhuang Feb 16, 2018 •

edited

Loading