add sorting options for labs, default viewing options for labs by MarisaPLee · Pull Request #992 · Submitty/Submitty

MarisaPLee · 2017-05-25T21:29:29Z

sorting by first name is using legal name and by not preferred name
closes #899

MasterOdin · 2017-05-26T13:39:07Z

Before I do my review, one thing that stands out is that you're only applying the ability to sort differently when we're grading by registration section. Why is this also not applied for rotating sections?

MasterOdin · 2017-05-28T15:32:33Z

Talking to Barb, we want to apply sorting to both registration and rotating section grading.

MasterOdin · 2017-05-26T13:31:01Z

TAGradingServer/account/print/print_checkpoints_gradeable.php

+}
+else{
+    $query = "SELECT * FROM users WHERE rotating_section=? AND user_group=? ORDER BY user_id";
+}


This should be something like:

$query = "SELECT * FROM users WHERE "; $query .= ($grade_by_reg_section) ? "registration_section=?" : "rotating_section=?"; $query .= " AND user_group=? ORDER BY " if ($sort_by === 0) { $query .= "user_firstname, user_lastname, user_id"; } elseif ($sort_by === 1) { $query .= "user_lastname, user_id"; } else { $query .= "user_id"; }

and dynamically build the query making it easier for us to go back and change things about the query without having to apply the changes to 4+ versions of it.

MasterOdin · 2017-05-26T13:32:33Z

TAGradingServer/account/print/print_checkpoints_gradeable.php

 $g_id = $_GET['g_id'];
 $section = intval($_GET['section_id']);
 $grade_by_reg_section = $_GET['grade_by_reg_section'];
+$sort_by = $_GET['sort_by'];


If we're only expecting a number to be passed on this, wrap with intval($_GET['sort_by']), especially if you're doing numeric comparisons later against it. Otherwise, this will be a string and then you're doing '0' == 0, '1' == 0, '1' == 1, etc which is a potential source of an error on implicit type coercion.

MasterOdin · 2017-05-28T15:44:53Z

site/app/libraries/database/DatabaseQueriesPostgresql.php

            }
        }
-        $keys = array("registration_section", "rotating_section");
+        $keys = array("registration_section", "rotating_section", "registration_section_by_first", "registration_section_by_last");


I'd prefer you add a new argument to the function $sort_key or something and not overload $section_key since it'll lead to a lot of duplicate stuff as we apply the same sorts to both registration and rotating sections (and make it harder to add new sorting methods in the future).

MasterOdin · 2017-05-28T15:52:14Z

site/app/libraries/database/DatabaseQueriesPostgresql.php

+          else{
            $query .= "
 ORDER BY u.{$section_key}, u.user_id";
+          }


Going off the above comments, it'd be nice to see this be something like:

$order_by = array(); $order_by[] = "u.{$section_key}; if ($sort_key === "by_first") { $order_by[] = "u.user_firstname"; $order_by[] = "u.user_lastname"; } else if ($sort_key === "by_last") { $order_by[] = "u.user_lastname"; } $order_by[] = "u.user_id"; $query .= "ORDER BY ".implode(", ", $order_by);

MasterOdin · 2017-05-28T15:58:11Z

site/app/views/grading/ElectronicGraderView.php

    </div>
 </div>
 HTML;
+        }


Fix the indentation on this curly brace so it aligns with what ever opened it. That way, we can scroll up in the file and find the associated if or else or whatever that opened it.

MasterOdin · 2017-05-28T15:59:08Z

site/app/views/grading/ElectronicGraderView.php

    <div style="float: right; margin-bottom: 10px">
 HTML;
            if (!isset($_GET['view']) || $_GET['view'] !== 'all') {
+                if ($this->core->getUser()->accessFullGrading()){


indent this line since the line above opens a new if block.

MasterOdin · 2017-05-28T15:59:46Z

site/app/views/grading/ElectronicGraderView.php

+                }
            }
            else {
+                if(count($this->core->getUser()->getGradingRegistrationSections()) !== 0){


indent as the line above opens a new if block.

MasterOdin · 2017-05-28T16:01:04Z

site/app/views/grading/SimpleGraderView.php

    public function checkpointForm($gradeable, $rows, $graders) {
        $return = <<<HTML
 <div class="content">
+            <div style="float: right; margin-bottom: 10px; margin-left: 20px">


Unindent as it it should be just one tab (4 spaces). The HTML and PHP content follow different indentation schemes (bit confusing at first, but you get used to it) which makes it easy to see where things start/end for various PHP blocks and HTML blocks.

…into Simple_Grader

wangj35

checked that sorting worked for instructor, full access grader (ta), and limited access grader (grader) for both sectional/rotating grading
sorting works and applies to the print sheet
if grader doesn't have access to a lab, no students show up and there is just a message
functionality & code all looks good to me

MasterOdin · 2017-06-06T17:15:45Z

TAGradingServer/account/print/print_checkpoints_gradeable.php

+else{
+    $query .= "user_id";
+}
+


We don't want to sort just on firstname or just one lastname or just on id as this can lead to unpredictable behavior if we have students who have matching first names and last names (it'll be sorted by insertion order which given manual additions might lead to some slight confusion on order).

I'd like to see this be:

$sort = array(); if ($sort_by === 'first') { $sort[] = 'user_firstname'; $sort[] = 'user_lastname'; } else if ($sort_by === 'last') { $sort[] = 'user_lastname'; } $sort[] = 'user_id'; $query .= implode(', ', $sort);

MasterOdin · 2017-06-06T17:16:41Z

site/app/libraries/database/DatabaseQueriesPostgresql.php

-            $query .= "
-ORDER BY u.{$section_key}, u.user_id";
+          $query .= "
+ORDER BY u.{$section_key}, {$sort_key}";


Same comment as above that we should not just be sorting by first name or last name (though just by id is fine as that's unique).

* add sorting options for labs, default viewing options for labs * sorting for registration and rotation grading * sort by more than just id

* Added directories to path in execute.cpp. Added gfx installs and cmake permissions in install_system.sh * added php zip file fix * Added very rough version of window_utils * Added support for screenshotting. Separated window code into window_utils Author: Evan Maicus <emaicus@outlook.com> * Added support for delays and typed commands * Added detection of window name based on child pid * Added click and drag support. Added mouse support. * Added error checking and documentation * Large overhaul of click and drag. * Added comments, added alt button support in click and drag, tested in prep for pull request. * Added support for decimal delays. * add sorting options for labs, default viewing options for labs (#992) * add sorting options for labs, default viewing options for labs * sorting for registration and rotation grading * sort by more than just id * Progress on revising navigation page with submission status (#962) progress on issue #674 * Allow user to specify whether they should stay logged in (past browser closure), defaults to true (#1014) * Ambersands no longer get truncated (#1022) * Gradeable layout with only numerics (#1023) * Looks better when there are zero numeric fields * Looks better when there are zero numeric fields * Electronic grader, changes names of functions and default for if instructor has grading (#1026) * add sorting options for labs, default viewing options for labs * sorting for registration and rotation grading * sort by more than just id * changed function names * now user id not left aligned * remove unneeded code * Creating a new Numeric Gradeable has a total points row (#1025) * Added a row for total points. * total row is functional * is updated when you make a gradeable from a template * made suggested changes and fixed a typo. * all updates made * corrected some unnecessary changes * First draft of check_everything.py -- checks directory owner/group/permissions (#1030) * just starting * progress * a bit more * edits to check_everything script * more checks * change requests * rearrange logic * add to travis after_script * Extra credit colors (#1024) * Penalty, extra, and normal credit look diffirent now * Extra credit and penalty have colors and icons * The icons are now on the left * Create Gradeable: check for invalid dates and gradeable_ids (#1021) * Added checks for the form for the user * updated with requested changes * updated with requested changes * check for hwcron user at the top of grade_students.sh (#1044) * Update sample apache configuration for Ubuntu 16.04 configuration (#1020) * Improved checking of autograding execution of local and system programs (#1050) replace 'assignment' with 'gradeable' in grade_students.sh (improves consistency) better error message for attempted use of system programs that are not on the whitelist now scrapes the config.json for names of compiled_executables to improve the check for local programs if the config.json uses an unacceptable local, now it complains at BUILD time (not delaying to submission debugging as before) fix autograding test failures disabled seccomp because it's broken :( will make a new issue & fix that next, in a new PR #1051. * remove PHP 5.5 (#1054) * Added very rough version of window_utils * Added support for screenshotting. Separated window code into window_utils Author: Evan Maicus <emaicus@outlook.com> * Added support for delays and typed commands * Added detection of window name based on child pid * Added click and drag support. Added mouse support. * Added error checking and documentation * Large overhaul of click and drag. * Added comments, added alt button support in click and drag, tested in prep for pull request. * Added support for decimal delays. * Refactored comments, fixed spelling issues. * Fixed formatting, hacked seccomp * Fixed mislabeled allowed system programs

… in Electronic Gradeable Uploads (#1209) * Added directories to path in execute.cpp. Added gfx installs and cmake permissions in install_system.sh * added php zip file fix * Added very rough version of window_utils * Added support for screenshotting. Separated window code into window_utils Author: Evan Maicus <emaicus@outlook.com> * Added support for delays and typed commands * Added detection of window name based on child pid * Added click and drag support. Added mouse support. * Added error checking and documentation * Large overhaul of click and drag. * Added comments, added alt button support in click and drag, tested in prep for pull request. * Added support for decimal delays. * add sorting options for labs, default viewing options for labs (#992) * add sorting options for labs, default viewing options for labs * sorting for registration and rotation grading * sort by more than just id * Progress on revising navigation page with submission status (#962) progress on issue #674 * Allow user to specify whether they should stay logged in (past browser closure), defaults to true (#1014) * Ambersands no longer get truncated (#1022) * Gradeable layout with only numerics (#1023) * Looks better when there are zero numeric fields * Looks better when there are zero numeric fields * Electronic grader, changes names of functions and default for if instructor has grading (#1026) * add sorting options for labs, default viewing options for labs * sorting for registration and rotation grading * sort by more than just id * changed function names * now user id not left aligned * remove unneeded code * Creating a new Numeric Gradeable has a total points row (#1025) * Added a row for total points. * total row is functional * is updated when you make a gradeable from a template * made suggested changes and fixed a typo. * all updates made * corrected some unnecessary changes * First draft of check_everything.py -- checks directory owner/group/permissions (#1030) * just starting * progress * a bit more * edits to check_everything script * more checks * change requests * rearrange logic * add to travis after_script * Extra credit colors (#1024) * Penalty, extra, and normal credit look diffirent now * Extra credit and penalty have colors and icons * The icons are now on the left * Create Gradeable: check for invalid dates and gradeable_ids (#1021) * Added checks for the form for the user * updated with requested changes * updated with requested changes * check for hwcron user at the top of grade_students.sh (#1044) * Update sample apache configuration for Ubuntu 16.04 configuration (#1020) * Improved checking of autograding execution of local and system programs (#1050) replace 'assignment' with 'gradeable' in grade_students.sh (improves consistency) better error message for attempted use of system programs that are not on the whitelist now scrapes the config.json for names of compiled_executables to improve the check for local programs if the config.json uses an unacceptable local, now it complains at BUILD time (not delaying to submission debugging as before) fix autograding test failures disabled seccomp because it's broken :( will make a new issue & fix that next, in a new PR #1051. * remove PHP 5.5 (#1054) * Added very rough version of window_utils * Added support for screenshotting. Separated window code into window_utils Author: Evan Maicus <emaicus@outlook.com> * Added support for delays and typed commands * Added detection of window name based on child pid * Added click and drag support. Added mouse support. * Added error checking and documentation * Large overhaul of click and drag. * Added comments, added alt button support in click and drag, tested in prep for pull request. * Added support for decimal delays. * Refactored comments, fixed spelling issues. * Fixed formatting, hacked seccomp * Fixed mislabeled allowed system programs * Added support for image diff. * added support to view expected image file. * fixed a regex bug in windowutils, finalized image diff functionality. * fixed a regex bug in windowutils, finalized image diff functionality. * checkpoint * Got text box images working * finished textbox images. finished image diff. fixed ta grading page image issues. fixed ta grading page error display issues. * added borders around images. * Added support for image resizing for textboxes. Resolved local test errors. * formatted. * added blue info messages and per image resizing. * Added new autograding examples for image diffing and textbox images. * updated travis test. * Quick fix. * hotfix

pleaseDefine and others added 2 commits May 25, 2017 17:25

add sorting options for labs, default viewing options for labs

73d592d

Merge branch 'master' into Simple_Grader

c115666

Merge branch 'master' into Simple_Grader

6bf7299

MasterOdin requested changes May 28, 2017

View reviewed changes

pleaseDefine added 3 commits June 5, 2017 09:20

merge with master

b248788

sorting for registration and rotation grading

c3579e0

Merge branch 'Simple_Grader' of https://github.com/Submitty/Submitty …

c510d7c

…into Simple_Grader

bmcutler requested a review from wangj35 June 5, 2017 21:40

wangj35 approved these changes Jun 6, 2017

View reviewed changes

MasterOdin requested changes Jun 6, 2017

View reviewed changes

pleaseDefine and others added 2 commits June 6, 2017 17:32

sort by more than just id

ce854d7

Merge branch 'master' into Simple_Grader

4ae3090

bmcutler merged commit 530fd58 into master Jun 7, 2017

bmcutler deleted the Simple_Grader branch June 7, 2017 20:32

emaicus pushed a commit that referenced this pull request Jun 19, 2017

add sorting options for labs, default viewing options for labs (#992)

71239ed

* add sorting options for labs, default viewing options for labs * sorting for registration and rotation grading * sort by more than just id

Conversation

MarisaPLee commented May 25, 2017 • edited by bmcutler Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MasterOdin commented May 26, 2017

Uh oh!

MasterOdin commented May 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangj35 left a comment

Choose a reason for hiding this comment

Uh oh!

MasterOdin Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MarisaPLee commented May 25, 2017 •

edited by bmcutler

Loading

MasterOdin Jun 6, 2017 •

edited

Loading