Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-18229

Add grade item and student grades quick edit feature to the grader report

    Details

    • Testing Instructions:
      Hide

      Test Setup:

      • Atleast 2 courses. 1 of size M.
      • Gradable items with at least 1x point, 1x scale set in grade settings.
      • 1 or more categories.

      [Test one] Navigation of Singleview is consistent with multiple courses.

      • Have > 1 course and many users.
      • With the second and third courses make sure that clicking the name of an activity in the Single view loads the activities page correctly.
      • Clicking a users name should send you to their profile page.

      [Test two] Change Singleview filtering.

      • Have many users and activities in a course.
      • Click the pen (Grades for ...) icon next to a users name in the Single view, should load all assignments for that user.
      • Click the pen icon (Grades for ...) next to an activity and load all users for that activity.

      [Test three] Gradereport links to Singleview on a per user or per activity basis.

      • Have many users and activities in a course.
      • From the Grade Report, make sure there is a pen (Single view for ...) icon next to the user-grades icon.
      • Clicking this icon should send you to the Single view and load the assignments and their grades for that user..
      • Back at the Grade Report, find the same pen (Single view for ...) icon next to each activity name.
      • Clicking the icon should take you to the Single view and load each user and their grades for that activity.
      • The Single view icons shouldn't be visible if you do not have the capability to view grades.

      [Test four] Inter-Singleview navigation between users and activities.

      • Have many users and activities in a course.
      • Go into the Single view, select a activity.
      • If at the first activity, there should be navigation links to go to the next activity, if not the first, a link to go to the previous activity.
      • Navigation links duplicated at the bottom.
      • Same functionality for users.

      [Test five] Bulk Insert.

      • Given a course of atleast size M.
      • Select a Singleview for an activity.
      • Select All in the override column.
      • Select the checkbox to the left of 'Insert' at the bottom of the page.
      • Enter in 50.00.
      • Make sure for "Empty grades" is selected.
      • Hit enter or update.
      • Ensure all the users grades for that activity have been updated.
      • Repeat for a user.
      • The activities for that user should all be updated.

      Note for test five: Not all grades get updated. Only the first 100 presented on the Singleview. You'll need to go back and choose the next page to update the next 100. Please confirm this is the case.

      Show
      Test Setup: Atleast 2 courses. 1 of size M. Gradable items with at least 1x point, 1x scale set in grade settings. 1 or more categories. [Test one] Navigation of Singleview is consistent with multiple courses. Have > 1 course and many users. With the second and third courses make sure that clicking the name of an activity in the Single view loads the activities page correctly. Clicking a users name should send you to their profile page. [Test two] Change Singleview filtering. Have many users and activities in a course. Click the pen (Grades for ...) icon next to a users name in the Single view, should load all assignments for that user. Click the pen icon (Grades for ...) next to an activity and load all users for that activity. [Test three] Gradereport links to Singleview on a per user or per activity basis. Have many users and activities in a course. From the Grade Report, make sure there is a pen (Single view for ...) icon next to the user-grades icon. Clicking this icon should send you to the Single view and load the assignments and their grades for that user.. Back at the Grade Report, find the same pen (Single view for ...) icon next to each activity name. Clicking the icon should take you to the Single view and load each user and their grades for that activity. The Single view icons shouldn't be visible if you do not have the capability to view grades. [Test four] Inter-Singleview navigation between users and activities. Have many users and activities in a course. Go into the Single view, select a activity. If at the first activity, there should be navigation links to go to the next activity, if not the first, a link to go to the previous activity. Navigation links duplicated at the bottom. Same functionality for users. [Test five] Bulk Insert. Given a course of atleast size M. Select a Singleview for an activity. Select All in the override column. Select the checkbox to the left of 'Insert' at the bottom of the page. Enter in 50.00. Make sure for "Empty grades" is selected. Hit enter or update. Ensure all the users grades for that activity have been updated. Repeat for a user. The activities for that user should all be updated. Note for test five: Not all grades get updated. Only the first 100 presented on the Singleview. You'll need to go back and choose the next page to update the next 100. Please confirm this is the case.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_28_STABLE
    • Epic Link:
    • Pull from Repository:
    • Pull Master Branch:
      MDL-18229-master
    • Sprint:
      FRONTEND Sprint 14
    • Story Points (Obsolete):
      100
    • Sprint:
      FRONTEND Sprint 14

      Description

      Add grade item and student grades quick edit feature to the grader report to enable editing for each grade item and for each student:

      • Grades and feedback
      • Grade overrides
      • Excluded grades

      Thanks to developers at the LSU for this feature - see http://moodle.org/mod/forum/discuss.php?d=101449 for more details.

        Gliffy Diagrams

        1. Activity View.png
          184 kB
        2. Grade Report.png
          187 kB
        3. Select View.png
          133 kB
        4. User View.png
          184 kB

          Issue Links

            Activity

            Hide
            joshuarbholden Joshua Holden added a comment -

            As far as I can tell, this has been incorporated into Moodle 2.x for grades and feedback, but not overrides and excluded grades. It would be really useful to have those also! Or is there a setting somewhere I'm missing?

            Show
            joshuarbholden Joshua Holden added a comment - As far as I can tell, this has been incorporated into Moodle 2.x for grades and feedback, but not overrides and excluded grades. It would be really useful to have those also! Or is there a setting somewhere I'm missing?
            Hide
            andyjdavis Andrew Davis added a comment -

            This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

            For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

            Show
            andyjdavis Andrew Davis added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
            Hide
            tsala Helen Foster added a comment -

            Thanks to everyone who has voted for this suggested new feature. It seems it is half implemented - with grades and feedback but not grade overrides or excluded grades.

            Andrew Davis please could you advise whether it will be part of the gradebook improvements for 2.8?

            Show
            tsala Helen Foster added a comment - Thanks to everyone who has voted for this suggested new feature. It seems it is half implemented - with grades and feedback but not grade overrides or excluded grades. Andrew Davis please could you advise whether it will be part of the gradebook improvements for 2.8?
            Hide
            mcka0074 Mark McKay added a comment -

            I think this got missed on the "Gradebook Push", but it is an important part of the vision for the 2.8 gradebook.

            Show
            mcka0074 Mark McKay added a comment - I think this got missed on the "Gradebook Push", but it is an important part of the vision for the 2.8 gradebook.
            Hide
            moodle.com moodle.com added a comment -

            If this is implemented, we may be able to do away with the AJAX editing on the grader report, which is covered by MDL-46732.

            Show
            moodle.com moodle.com added a comment - If this is implemented, we may be able to do away with the AJAX editing on the grader report, which is covered by MDL-46732 .
            Hide
            zac Zachary Durber added a comment -

            Potential concerns.

            • Repurposing of the User Grades icon to filter the Grade Report instead of navigating you to the User Report.
            • The grades icon and and it's placement next to the item-name in the Grade Report.
            • Turning editing on/off reverts filters.
            Show
            zac Zachary Durber added a comment - Potential concerns. Repurposing of the User Grades icon to filter the Grade Report instead of navigating you to the User Report. The grades icon and and it's placement next to the item-name in the Grade Report. Turning editing on/off reverts filters.
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-18229 using repository: git://github.com/zbdd/moodle.git

            • master (branch: MDL-18229-master | CI Job)
              • Error: The MDL-18229-master branch at git://github.com/zbdd/moodle.git exceeds the maximum number of commits ( 25 > 15)

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-18229 using repository: git://github.com/zbdd/moodle.git master (branch: MDL-18229-master | CI Job ) Error: The MDL-18229 -master branch at git://github.com/zbdd/moodle.git exceeds the maximum number of commits ( 25 > 15) More information about this report
            Hide
            zac Zachary Durber added a comment -

            I believe we are searching for similar functionality as:
            http://docs.moodle.org/en/grade/report/simple_grader/index
            http://docs.moodle.org/en/grade/report/simple_grader/quick_edit
            http://docs.moodle.org/en/grade/edit/simple_tree/index

            Adding in some screenshots of what my implementation looks like.

            Show
            zac Zachary Durber added a comment - I believe we are searching for similar functionality as: http://docs.moodle.org/en/grade/report/simple_grader/index http://docs.moodle.org/en/grade/report/simple_grader/quick_edit http://docs.moodle.org/en/grade/edit/simple_tree/index Adding in some screenshots of what my implementation looks like.
            Hide
            rex Rex Lorenzo added a comment - - edited

            Zachary Durber Looking at your screenshots, I am not sure you have actually tried out LSU's QuickEdit, which this task is suppose to be integrating or emulate its behavior:

            https://github.com/lsuits/quick_edit

            We want a page that a teacher can quickly grade or view all students for a particular grade item. Or grade or view all grade items for a particular student.

            What you have in your screenshots appear to be what is currently possible in the grader report with quick editing turned on, which does not scale well with thousands of items.

            Show
            rex Rex Lorenzo added a comment - - edited Zachary Durber Looking at your screenshots, I am not sure you have actually tried out LSU's QuickEdit, which this task is suppose to be integrating or emulate its behavior: https://github.com/lsuits/quick_edit We want a page that a teacher can quickly grade or view all students for a particular grade item. Or grade or view all grade items for a particular student. What you have in your screenshots appear to be what is currently possible in the grader report with quick editing turned on, which does not scale well with thousands of items.
            Hide
            rrusso Robert Russo added a comment -

            Here is the real LSU quick edit.

            https://github.com/lsuits/quick_edit

            Show
            rrusso Robert Russo added a comment - Here is the real LSU quick edit. https://github.com/lsuits/quick_edit
            Hide
            rrusso Robert Russo added a comment -

            Quick Edit is a simple report that you install as with any grader report. Hacks are no longer necessary.

            Show
            rrusso Robert Russo added a comment - Quick Edit is a simple report that you install as with any grader report. Hacks are no longer necessary.
            Hide
            rrusso Robert Russo added a comment - - edited

            Here is the patch that allows for integration into the grader report for ultra quick grading of either student grades or graded items.

            https://raw.githubusercontent.com/lsuits/gradebook_patches/master/quick_edit_integration.patch

            This patch is for 2.7.

            Show
            rrusso Robert Russo added a comment - - edited Here is the patch that allows for integration into the grader report for ultra quick grading of either student grades or graded items. https://raw.githubusercontent.com/lsuits/gradebook_patches/master/quick_edit_integration.patch This patch is for 2.7.
            Hide
            bobpuffer Bob Puffer added a comment -

            This is a total departure from the plan with no good outcomes. Let's get back to LSU's quickedit, used by schools all over the world, please.

            Show
            bobpuffer Bob Puffer added a comment - This is a total departure from the plan with no good outcomes. Let's get back to LSU's quickedit, used by schools all over the world, please.
            Hide
            dougiamas Martin Dougiamas added a comment - - edited

            I believe there is confusion here between:

            • "quick editing" items directly in the grader report with editing on and
            • LSU's Quick Edit which is actually "individual editing of one row or column at a time".

            This issue is supposed to be the second one, but I think Zac is working on the first (should be a different issue, part of MDL-25544 ).

            We'll sort this out tomorrow.

            Show
            dougiamas Martin Dougiamas added a comment - - edited I believe there is confusion here between: "quick editing" items directly in the grader report with editing on and LSU's Quick Edit which is actually "individual editing of one row or column at a time". This issue is supposed to be the second one, but I think Zac is working on the first (should be a different issue, part of MDL-25544 ). We'll sort this out tomorrow.
            Hide
            salvetore Michael de Raadt added a comment - - edited

            It's good that we now have the Quick edit report code.

            Noting that this code was made available on this issue's Epic (MDL-44673). It's a shame that wasn't indicated here.

            Show
            salvetore Michael de Raadt added a comment - - edited It's good that we now have the Quick edit report code. Noting that this code was made available on this issue's Epic ( MDL-44673 ). It's a shame that wasn't indicated here.
            Hide
            salvetore Michael de Raadt added a comment -

            It's also worth keeping in mind the requirements set out in the original spec (https://docs.moodle.org/dev/Grader_report_2014#Single_row.2Fcolumn_editing)

            Show
            salvetore Michael de Raadt added a comment - It's also worth keeping in mind the requirements set out in the original spec ( https://docs.moodle.org/dev/Grader_report_2014#Single_row.2Fcolumn_editing )
            Hide
            zac Zachary Durber added a comment -

            Thanks for the updates all, Martin was correct regarding the confusion.
            Looking at the Quick Edit code now.

            Show
            zac Zachary Durber added a comment - Thanks for the updates all, Martin was correct regarding the confusion. Looking at the Quick Edit code now.
            Hide
            zac Zachary Durber added a comment -

            Putting this up for peer review to get some other opinions and insights on what we have accomplished thus far.

            • We are aware the commits need to be cleaned up.
            Show
            zac Zachary Durber added a comment - Putting this up for peer review to get some other opinions and insights on what we have accomplished thus far. We are aware the commits need to be cleaned up.
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-18229 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-18229 using repository: git://github.com/zbdd/moodle.git master (branch: MDL-18229-master | CI Job ) Coding style problems found More information about this report
            Hide
            damyon Damyon Wiese added a comment -

            This branch looks in pretty good shape - most of the things that are needed are just tidying - but overall it's pretty good.

            Here are a list if things I spotted while reviewing - this is just things I thought cibot had not already covered:

            Remove Readme.md - thats for plugins in the plugins DB.

            The package for these files should be gradereport_singleview.

            The classes could be split into one class per file and make use of autoloading. As it is - all the class names have the wrong prefix (should be same as package name above). This strict enforcement of classnames is because php has a just one scope for all these classes and we need to ensure plugins do not collide with other plugins). A nicer alternative would be to put all these classes in a namespace.

            There is a general lack of php docs in this code.

            All files will need to define the copyright owner for the code.

            Strings files should be alphabetical

            There should be AMOS commits for any strings that are the same as existing strings.

            $string['for'] = 'for'; - This string looks sus - I bet its used for concatination (bad).

            All of this type of code could be replaced with automatic class loading (need to make a call if this can be done in the time - probably not).
            + dirname(_FILE_) . '/screens';
            +
            + $is_valid = function($filename) use ($screendir) {
            + if (preg_match('/^\./', $filename)){

            This events trigger code (qe_events_trigger) looks suspect and can probably be stripped out.

            This is should be using

            get_role_users(
            + $roleids, $this->context, false, '',
            + 'u.lastname, u.firstname', null, $this->groupid,
            + $this->perpage * $this->page, $this->perpage
            + );

            The CSS is quite good.

            This new plugin needs to be added to the list of standard plugins in core_plugin_manager:;standard_plugins_list()

            This needs some behat test at least.

            The amount of work remaining seems managable - go go go!

            Show
            damyon Damyon Wiese added a comment - This branch looks in pretty good shape - most of the things that are needed are just tidying - but overall it's pretty good. Here are a list if things I spotted while reviewing - this is just things I thought cibot had not already covered: Remove Readme.md - thats for plugins in the plugins DB. The package for these files should be gradereport_singleview. The classes could be split into one class per file and make use of autoloading. As it is - all the class names have the wrong prefix (should be same as package name above). This strict enforcement of classnames is because php has a just one scope for all these classes and we need to ensure plugins do not collide with other plugins). A nicer alternative would be to put all these classes in a namespace. There is a general lack of php docs in this code. All files will need to define the copyright owner for the code. Strings files should be alphabetical There should be AMOS commits for any strings that are the same as existing strings. $string ['for'] = 'for'; - This string looks sus - I bet its used for concatination (bad). All of this type of code could be replaced with automatic class loading (need to make a call if this can be done in the time - probably not). + dirname(_ FILE _) . '/screens'; + + $is_valid = function($filename) use ($screendir) { + if (preg_match('/^\./', $filename)){ This events trigger code (qe_events_trigger) looks suspect and can probably be stripped out. This is should be using get_role_users( + $roleids, $this->context, false, '', + 'u.lastname, u.firstname', null, $this->groupid, + $this->perpage * $this->page, $this->perpage + ); The CSS is quite good. This new plugin needs to be added to the list of standard plugins in core_plugin_manager:;standard_plugins_list() This needs some behat test at least. The amount of work remaining seems managable - go go go!
            Hide
            rex Rex Lorenzo added a comment -

            Just a note regarding the event trigger code "qe_events_trigger". It was a clever hack to get inter-plugin communication using the older event system api.

            See this presentation by LSU a couple of years back: https://docs.google.com/presentation/d/1lmkuHHDY6stvNMT3YI_edXtN_pv7z2--WmKPKqyepPk/edit#slide=id.p

            Hopefully we can get a real plugin hook system into Moodle sooner than later (MDL-44078).

            Show
            rex Rex Lorenzo added a comment - Just a note regarding the event trigger code "qe_events_trigger". It was a clever hack to get inter-plugin communication using the older event system api. See this presentation by LSU a couple of years back: https://docs.google.com/presentation/d/1lmkuHHDY6stvNMT3YI_edXtN_pv7z2--WmKPKqyepPk/edit#slide=id.p Hopefully we can get a real plugin hook system into Moodle sooner than later ( MDL-44078 ).
            Hide
            zac Zachary Durber added a comment - - edited

            Updated the code based off the review. New labels to assist in accessibility as well as a suite of behat testing.

            Could I get another set of eyes on this please?

            EDIT: Patch incoming for the Bulk Update, noticed it's not working atm.
            EDIT 2: This was because we removed pagination and the forms could only handle so much data at once. So for a assignment with > 300 or so users the form data would break and the bulk insert values were being missed. Reinstated pagination maxed at 100 users per page.

            Show
            zac Zachary Durber added a comment - - edited Updated the code based off the review. New labels to assist in accessibility as well as a suite of behat testing. Could I get another set of eyes on this please? EDIT: Patch incoming for the Bulk Update, noticed it's not working atm. EDIT 2: This was because we removed pagination and the forms could only handle so much data at once. So for a assignment with > 300 or so users the form data would break and the bulk insert values were being missed. Reinstated pagination maxed at 100 users per page.
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-18229 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-18229 using repository: git://github.com/zbdd/moodle.git master (branch: MDL-18229-master | CI Job ) Coding style problems found More information about this report
            Hide
            fred Frédéric Massart added a comment -

            Hi Zac,

            Thanks for working on this issue, please find a few comments:

            grade/report/grader/lib.php

            1. L814 Shouldn't that be checking the capability of the singleview report?

            grade/report/singleview/classes/lib.php

            1. L216 $oldvalue might be used at L227 but not undefined.
            2. L356 White space

            grade/report/singleview/classes/uilib.php

            1. L60 You are adding $label but it is not a defined class property.
            2. L289 I don't think there is a need to rename is_checked(), a method can contain underscores. And that would be consistent with is_disabled().
            3. You are not using the label in dropdown.
            4. L316 I am not sure if closures are allowed/recommended in Moodle code, that would be good to check. This one does not seem really helpful anyway...
            5. L541 Few coding style issues in the if conditions.

            grade/report/singleview/index.php

            1. L114 $report::classname() is better called using the class name: grade_report_singleview::classname()

            grade/report/singleview/screens/grade/lib.php

            1. L87, could you please fix the indentation?

            grade/report/singleview/screens/user/lib.php

            1. L37 Closure again.
            2. L108 Coding style (white spaces, missing curly brackets)
            3. L155-159 I think this should be using fullname() all the time. String concatenation is not advised and I'm not sure I understand what that is supposed to be for.

            Behat

            1. I think the tag @singleview should be @gradereport_singleview
            2. A scenario and its background should start with Given, then have 1 When, and 1 Then.
            3. You are missing a 2 space indendation for the points under the feature (Background, Scenario, @javascript)
            4. You are setting the course groupmode to 1, is that relevant to the tests?
            5. Do you need that many activities to check that grades can be updated?
            6. Scenario: 'Single view quick links work on grade report' is not checking if you're landing on the right page
            7. Scenario: 'Navigation works in the Single view.' is not checking if you're landing on the right page

            JS

            1. Should we consider a proper YUI module using shifter?

            CSS

            1. Should the margin be adjusted in RTL?

            Damyon's comments

            1. What about the readme file?
            2. What about the autoloading and the file per class?
            3. What about the missing PHP Docs?
            4. The strings in the language file are not alphabetically ordered
            5. What about the AMOS commands to copy strings from single_view to singleview?
            6. What about $string['for']?
            7. What about adding the plugin to the list of standard plugins?

            I will stop the review here, I am not sure what had to be reviewd but I focussed on the changes and not the code that those changes are based on. I noticed a few things that Damyon pointed out, if they do not need to be fixed could you please just comment about them here.

            PS: I did not do any testing, did not run behat, and did not even checkout the branch due to the lack of time I only looked at the diff on gitub. My sincere apologies if I have missed many obvious things .

            Good work Zac!

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Hi Zac, Thanks for working on this issue, please find a few comments: grade/report/grader/lib.php L814 Shouldn't that be checking the capability of the singleview report? grade/report/singleview/classes/lib.php L216 $oldvalue might be used at L227 but not undefined. L356 White space grade/report/singleview/classes/uilib.php L60 You are adding $label but it is not a defined class property. L289 I don't think there is a need to rename is_checked() , a method can contain underscores. And that would be consistent with is_disabled() . You are not using the label in dropdown . L316 I am not sure if closures are allowed/recommended in Moodle code, that would be good to check. This one does not seem really helpful anyway... L541 Few coding style issues in the if conditions. grade/report/singleview/index.php L114 $report::classname() is better called using the class name: grade_report_singleview::classname() grade/report/singleview/screens/grade/lib.php L87, could you please fix the indentation? grade/report/singleview/screens/user/lib.php L37 Closure again. L108 Coding style (white spaces, missing curly brackets) L155-159 I think this should be using fullname() all the time. String concatenation is not advised and I'm not sure I understand what that is supposed to be for. Behat I think the tag @singleview should be @gradereport_singleview A scenario and its background should start with Given, then have 1 When, and 1 Then. You are missing a 2 space indendation for the points under the feature (Background, Scenario, @javascript) You are setting the course groupmode to 1, is that relevant to the tests? Do you need that many activities to check that grades can be updated? Scenario: 'Single view quick links work on grade report' is not checking if you're landing on the right page Scenario: 'Navigation works in the Single view.' is not checking if you're landing on the right page JS Should we consider a proper YUI module using shifter? CSS Should the margin be adjusted in RTL? Damyon's comments What about the readme file? What about the autoloading and the file per class? What about the missing PHP Docs? The strings in the language file are not alphabetically ordered What about the AMOS commands to copy strings from single_view to singleview? What about $string ['for'] ? What about adding the plugin to the list of standard plugins? I will stop the review here, I am not sure what had to be reviewd but I focussed on the changes and not the code that those changes are based on. I noticed a few things that Damyon pointed out, if they do not need to be fixed could you please just comment about them here. PS: I did not do any testing, did not run behat, and did not even checkout the branch due to the lack of time I only looked at the diff on gitub. My sincere apologies if I have missed many obvious things . Good work Zac! Cheers, Fred
            Hide
            zac Zachary Durber added a comment - - edited

            Thank you Fred, majority of this barring the following have been addressed.

            grade/report/singleview/screens/user/lib.php - the closure complaint.
            JS and CSS not touched.
            No PHP Docs.
            No Autoloading.

            Behat tests were updated and expanded, then run again.

            Show
            zac Zachary Durber added a comment - - edited Thank you Fred, majority of this barring the following have been addressed. grade/report/singleview/screens/user/lib.php - the closure complaint. JS and CSS not touched. No PHP Docs. No Autoloading. Behat tests were updated and expanded, then run again.
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-18229 using repository: git://github.com/zbdd/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-18229 using repository: git://github.com/zbdd/moodle.git master (branch: MDL-18229-master | CI Job ) Coding style problems found More information about this report
            Hide
            damyon Damyon Wiese added a comment -

            Hi Zac,

            Taking a look at this now to guage how much work is left.

            I am not covering everything here - just the stuff that seems like a real blocker.

            1. The interfaces in grade/report/singleview/classes/lib.php need prefixing.

            2. On a clean site I went to grade a single student and I get debugging:
            Notice: Undefined property: grade_item::$cmid in /home/damyonw/Documents/Moodle/integration/master/moodle/grade/report/singleview/screens/user/lib.php on line 112

            3. There is trailing white space:
            grade/report/singleview/classes/lib.php 2 of them
            grade/report/singleview/db/access.php first line
            grade/report/singleview/index.php 2
            grade/report/singleview/lib.php 1
            grade/report/singleview/screens/grade/lib.php 1
            grade/report/singleview/tests/behat/singleview.feature 3

            4. Behat features need 1 given, 1 when, and 1 then each - no more no less.

            5. Not all the GPL copyright sections are identical - they should be

            6. The copyright owner of each file seems to be different all over - this needs to be decided and applied consistently.

            7. This style rule is not specific enough:
            input[name^="finalgrade"]

            { + width: 50px; +}

            8. This check has_capability('gradereport/'.$CFG->grade_profilereport.':view'
            should be checking has_capability('gradereport/singleview:view'

            There are many more points that have been made above, but are not fixed yet in the branch - I guess the points in this review would be enough to send it to integration for a look.

            Thanks - Damo

            Show
            damyon Damyon Wiese added a comment - Hi Zac, Taking a look at this now to guage how much work is left. I am not covering everything here - just the stuff that seems like a real blocker. 1. The interfaces in grade/report/singleview/classes/lib.php need prefixing. 2. On a clean site I went to grade a single student and I get debugging: Notice: Undefined property: grade_item::$cmid in /home/damyonw/Documents/Moodle/integration/master/moodle/grade/report/singleview/screens/user/lib.php on line 112 3. There is trailing white space: grade/report/singleview/classes/lib.php 2 of them grade/report/singleview/db/access.php first line grade/report/singleview/index.php 2 grade/report/singleview/lib.php 1 grade/report/singleview/screens/grade/lib.php 1 grade/report/singleview/tests/behat/singleview.feature 3 4. Behat features need 1 given, 1 when, and 1 then each - no more no less. 5. Not all the GPL copyright sections are identical - they should be 6. The copyright owner of each file seems to be different all over - this needs to be decided and applied consistently. 7. This style rule is not specific enough: input [name^="finalgrade"] { + width: 50px; +} 8. This check has_capability('gradereport/'.$CFG->grade_profilereport.':view' should be checking has_capability('gradereport/singleview:view' There are many more points that have been made above, but are not fixed yet in the branch - I guess the points in this review would be enough to send it to integration for a look. Thanks - Damo
            Hide
            fred Frédéric Massart added a comment -

            Zac, could you please answer to every point that was made during the reviews? Even if it's just to state that you won't fix it, as long as there is an explanation. That makes it easier to track what was (not) done and why. Cheers!

            Show
            fred Frédéric Massart added a comment - Zac, could you please answer to every point that was made during the reviews? Even if it's just to state that you won't fix it, as long as there is an explanation. That makes it easier to track what was (not) done and why. Cheers!
            Hide
            zac Zachary Durber added a comment - - edited

            I'll edit this once I get a chance to look at Damyon's latest review.

            EDIT: Updates at the bottom.

            grade/report/grader/lib.php
            L814 Shouldn't that be checking the capability of the singleview report? -> Haven't looked at, there is no singleview capability atm.

            grade/report/singleview/classes/lib.php
            L216 $oldvalue might be used at L227 but not undefined. -> fixed this up. Merged the two calls into a isset and whether it's been posted.
            L356 White space -> fixed.

            grade/report/singleview/classes/uilib.php
            L60 You are adding $label but it is not a defined class property. -> defined as a public variable for gradereport_singelview_ui_element now.
            L289 I don't think there is a need to rename is_checked(), a method can contain underscores. And that would be consistent with is_disabled(). -> fixed.
            You are not using the label in dropdown. -> removed it except on the parent::construct where it's still required.
            L316 I am not sure if closures are allowed/recommended in Moodle code, that would be good to check. This one does not seem really helpful anyway... -> removed and fixed.
            L541 Few coding style issues in the if conditions. -> fixed.

            grade/report/singleview/index.php
            L114 $report::classname() is better called using the class name: grade_report_singleview::classname() -> fixed.

            grade/report/singleview/screens/grade/lib.php
            L87, could you please fix the indentation? -> fixed.

            grade/report/singleview/screens/user/lib.php
            L37 Closure again. -> not touched, haven't had time to investigate. investigated with Andrew, will not fix
            L108 Coding style (white spaces, missing curly brackets) -> fixed.
            L155-159 I think this should be using fullname() all the time. String concatenation is not advised and I'm not sure I understand what that is supposed to be for. -> I agree, fixed.

            Behat
            I think the tag @singleview should be @gradereport_singleview -> updated.
            A scenario and its background should start with Given, then have 1 When, and 1 Then. -> Reviewed with Andrew and updated.
            You are missing a 2 space indendation for the points under the feature (Background, Scenario, @javascript) -> fixed.
            You are setting the course groupmode to 1, is that relevant to the tests? -> removed.
            Do you need that many activities to check that grades can be updated? -> removed extra activities.
            Scenario: 'Single view quick links work on grade report' is not checking if you're landing on the right page -> added in additional checks.
            Scenario: 'Navigation works in the Single view.' is not checking if you're landing on the right page -> added in additional checks.

            JS
            Should we consider a proper YUI module using shifter? -> not looked at.

            CSS
            Should the margin be adjusted in RTL? -> not looked at.

            Damyon's comments
            What about the readme file? -> removed.
            What about the autoloading and the file per class? -> looked at, updated the names of all functions to be prefixed with gradereport_, nothing else done here though.
            What about the missing PHP Docs? -> not looked into.
            The strings in the language file are not alphabetically ordered -> reordered.
            What about the AMOS commands to copy strings from single_view to singleview? -> added in AMOS into last commit for override string. (also in mod_quiz)
            What about $string['for']? -> renamed to bulkappliesto, will need to revisit this I believe.
            What about adding the plugin to the list of standard plugins? -> added to gradereport component.

            Damyon Review 2

            1. The interfaces in grade/report/singleview/classes/lib.php need prefixing. -> Prefixed, sorry I missed the interface* ones with my find/sed script.
            2. On a clean site I went to grade a single student and I get debugging:
              Notice: Undefined property: grade_item::$cmid in /home/damyonw/Documents/Moodle/integration/master/moodle/grade/report/singleview/screens/user/lib.php on line 112 -> have added a check for that property before using it.
            3. There is trailing white space: -> ran a script to clean this up.
              grade/report/singleview/classes/lib.php 2 of them
              grade/report/singleview/db/access.php first line
              grade/report/singleview/index.php 2
              grade/report/singleview/lib.php 1
              grade/report/singleview/screens/grade/lib.php 1
              grade/report/singleview/tests/behat/singleview.feature 3
            4. Behat features need 1 given, 1 when, and 1 then each - no more no less. -> updated during review with Andrew.
            5. Not all the GPL copyright sections are identical - they should be -> updated.
            6. The copyright owner of each file seems to be different all over - this needs to be decided and applied consistently. -> should be consistent now, a couple of pages differed.
            7. This style rule is not specific enough: -> is now inline with the rest of the styling.
              input[name^="finalgrade"] { + width: 50px; +}
            8. This check has_capability('gradereport/'.$CFG->grade_profilereport.':view'
              should be checking has_capability('gradereport/singleview:view' -> fixed, also checks to make sure it renders the link+icon only on a grade item. Not a course or category header.
            Show
            zac Zachary Durber added a comment - - edited I'll edit this once I get a chance to look at Damyon's latest review. EDIT: Updates at the bottom. grade/report/grader/lib.php L814 Shouldn't that be checking the capability of the singleview report? -> Haven't looked at, there is no singleview capability atm. grade/report/singleview/classes/lib.php L216 $oldvalue might be used at L227 but not undefined. -> fixed this up. Merged the two calls into a isset and whether it's been posted. L356 White space -> fixed. grade/report/singleview/classes/uilib.php L60 You are adding $label but it is not a defined class property. -> defined as a public variable for gradereport_singelview_ui_element now. L289 I don't think there is a need to rename is_checked(), a method can contain underscores. And that would be consistent with is_disabled(). -> fixed. You are not using the label in dropdown. -> removed it except on the parent::construct where it's still required. L316 I am not sure if closures are allowed/recommended in Moodle code, that would be good to check. This one does not seem really helpful anyway... -> removed and fixed. L541 Few coding style issues in the if conditions. -> fixed. grade/report/singleview/index.php L114 $report::classname() is better called using the class name: grade_report_singleview::classname() -> fixed. grade/report/singleview/screens/grade/lib.php L87, could you please fix the indentation? -> fixed. grade/report/singleview/screens/user/lib.php L37 Closure again. -> not touched, haven't had time to investigate. investigated with Andrew, will not fix L108 Coding style (white spaces, missing curly brackets) -> fixed. L155-159 I think this should be using fullname() all the time. String concatenation is not advised and I'm not sure I understand what that is supposed to be for. -> I agree, fixed. Behat I think the tag @singleview should be @gradereport_singleview -> updated. A scenario and its background should start with Given, then have 1 When, and 1 Then. -> Reviewed with Andrew and updated. You are missing a 2 space indendation for the points under the feature (Background, Scenario, @javascript) -> fixed. You are setting the course groupmode to 1, is that relevant to the tests? -> removed. Do you need that many activities to check that grades can be updated? -> removed extra activities. Scenario: 'Single view quick links work on grade report' is not checking if you're landing on the right page -> added in additional checks. Scenario: 'Navigation works in the Single view.' is not checking if you're landing on the right page -> added in additional checks. JS Should we consider a proper YUI module using shifter? -> not looked at. CSS Should the margin be adjusted in RTL? -> not looked at. Damyon's comments What about the readme file? -> removed. What about the autoloading and the file per class? -> looked at, updated the names of all functions to be prefixed with gradereport_, nothing else done here though. What about the missing PHP Docs? -> not looked into. The strings in the language file are not alphabetically ordered -> reordered. What about the AMOS commands to copy strings from single_view to singleview? -> added in AMOS into last commit for override string. (also in mod_quiz) What about $string ['for'] ? -> renamed to bulkappliesto, will need to revisit this I believe. What about adding the plugin to the list of standard plugins? -> added to gradereport component. Damyon Review 2 The interfaces in grade/report/singleview/classes/lib.php need prefixing. -> Prefixed, sorry I missed the interface* ones with my find/sed script. On a clean site I went to grade a single student and I get debugging: Notice: Undefined property: grade_item::$cmid in /home/damyonw/Documents/Moodle/integration/master/moodle/grade/report/singleview/screens/user/lib.php on line 112 -> have added a check for that property before using it. There is trailing white space: -> ran a script to clean this up. grade/report/singleview/classes/lib.php 2 of them grade/report/singleview/db/access.php first line grade/report/singleview/index.php 2 grade/report/singleview/lib.php 1 grade/report/singleview/screens/grade/lib.php 1 grade/report/singleview/tests/behat/singleview.feature 3 Behat features need 1 given, 1 when, and 1 then each - no more no less. -> updated during review with Andrew. Not all the GPL copyright sections are identical - they should be -> updated. The copyright owner of each file seems to be different all over - this needs to be decided and applied consistently. -> should be consistent now, a couple of pages differed. This style rule is not specific enough: -> is now inline with the rest of the styling. input [name^="finalgrade"] { + width: 50px; +} This check has_capability('gradereport/'.$CFG->grade_profilereport.':view' should be checking has_capability('gradereport/singleview:view' -> fixed, also checks to make sure it renders the link+icon only on a grade item. Not a course or category header.
            Hide
            nebgor Aparup Banerjee added a comment -

            The navigation across grade items broke, here is a fix updated with the new class name.
            git fetch git@github.com:nebgor/moodle.git MDL-18229-master; git cherry-pick e83474026ec63902c44e50b9fdf89e3ec3db0ba4;

            Show
            nebgor Aparup Banerjee added a comment - The navigation across grade items broke, here is a fix updated with the new class name. git fetch git@github.com:nebgor/moodle.git MDL-18229 -master; git cherry-pick e83474026ec63902c44e50b9fdf89e3ec3db0ba4;
            Hide
            zac Zachary Durber added a comment -

            Added in, thanks Aparup.

            Show
            zac Zachary Durber added a comment - Added in, thanks Aparup.
            Hide
            nebgor Aparup Banerjee added a comment - - edited

            reviewing current status @ https://github.com/zbdd/moodle/compare/moodle:master...MDL-18229-master?diff=split

            1. https://github.com/zbdd/moodle/compare/moodle:master...MDL-18229-master?diff=split#diff-1026048e589fad53a8c7c559ce6fc3e6R822
              not quite sure why we're testing using preg_match here for '^item' instead of simply == , 'item course' , 'item grade' expected perhaps? (not sure).
            2. due to bulk updates being possible, with zeros or even blank '-', please add RISK_DATALOSS to the permissions risk mask for a 'write' cap type capability , so RISK_DATALOSS | RISK_PERSONAL. This probably deserves the implied additional capability and not 'grade report/singleview:view'. Viewing shouldn't allow writing, perhaps something with 'grade' will be more meaningful here. The 'view' permission is used to display a direct link to single view. 'moodle/grade:edit' has the necessary details to control writing to grades. Basically we need consistency here, use one report permission and use it in deciding to display the edit icons and to allow edits too. The report does allow bulk dataloss after examining the database changes to table grade_grades. Best to look at how grader report is handling these permissions. Also see singleview/lib.php : gradereport_singleview_profilereport()'s $can_use. (grr underscored variables..)
            3. submitting from 'update' in singleview goes back to grader report which can be slow to load. did we want this or did we want to come back to the same page and perhaps display a nice green 'changes saved' box ?
            4. All process_data() or screens' process($data) should be checked for permission checks before writing, i can't seem to find any. The $DB inserts are in the set() calls within singleview/classes/uilib.php : gradereport_singleview_*_ui classes. This level seems the best to implant the permission checks prior to writing.
            5. https://github.com/zbdd/moodle/compare/moodle:master...MDL-18229-master?diff=split#diff-fa6dc3738442cd750f86eb883bfafaf5R41 contains reference to old and now unused phrase 'quick edit'. ( also use of colon in 'singleview:view' in line 48 is new to me)
            6. https://github.com/zbdd/moodle/compare/moodle:master...MDL-18229-master?diff=split#diff-8984520654c7ce9fd8f7376803bfbc59R116 i think concatenations are preferred, did cibot mention this?

            ok thats the peer review for me.

            Show
            nebgor Aparup Banerjee added a comment - - edited reviewing current status @ https://github.com/zbdd/moodle/compare/moodle:master...MDL-18229-master?diff=split https://github.com/zbdd/moodle/compare/moodle:master...MDL-18229-master?diff=split#diff-1026048e589fad53a8c7c559ce6fc3e6R822 not quite sure why we're testing using preg_match here for '^item' instead of simply == , 'item course' , 'item grade' expected perhaps? (not sure). due to bulk updates being possible, with zeros or even blank '-', please add RISK_DATALOSS to the permissions risk mask for a 'write' cap type capability , so RISK_DATALOSS | RISK_PERSONAL. This probably deserves the implied additional capability and not 'grade report/singleview:view'. Viewing shouldn't allow writing, perhaps something with 'grade' will be more meaningful here. The 'view' permission is used to display a direct link to single view. 'moodle/grade:edit' has the necessary details to control writing to grades. Basically we need consistency here, use one report permission and use it in deciding to display the edit icons and to allow edits too. The report does allow bulk dataloss after examining the database changes to table grade_grades. Best to look at how grader report is handling these permissions. Also see singleview/lib.php : gradereport_singleview_profilereport()'s $can_use. (grr underscored variables..) submitting from 'update' in singleview goes back to grader report which can be slow to load. did we want this or did we want to come back to the same page and perhaps display a nice green 'changes saved' box ? All process_data() or screens' process($data) should be checked for permission checks before writing , i can't seem to find any. The $DB inserts are in the set() calls within singleview/classes/uilib.php : gradereport_singleview_*_ui classes. This level seems the best to implant the permission checks prior to writing. https://github.com/zbdd/moodle/compare/moodle:master...MDL-18229-master?diff=split#diff-fa6dc3738442cd750f86eb883bfafaf5R41 contains reference to old and now unused phrase 'quick edit'. ( also use of colon in 'singleview:view' in line 48 is new to me) https://github.com/zbdd/moodle/compare/moodle:master...MDL-18229-master?diff=split#diff-8984520654c7ce9fd8f7376803bfbc59R116 i think concatenations are preferred, did cibot mention this? ok thats the peer review for me.
            Hide
            zac Zachary Durber added a comment - - edited
            1. Review with Apu, we are preg_matching for NOT starting with category. So the Singleview link will show for gradable items. (including Course total)
            2. Response to this and Number 4 (From above). Adding in additional has_capability check for editing grades during processing. There are already 3 capability checks on loading the Singleview.
            3. I prefer to change it as you have suggested but haven't had the go ahead.
            4. Answered in 2
            5. Reworded old usage of Quick Edit.
            6. Corrected.

            Apu and I are currently looking into a bug with the navigation and pagination in the user screen.
            EDIT: Fixed, pagination never was implemented for the User screen. Made sure it doesn't try to paginated by default.

            Show
            zac Zachary Durber added a comment - - edited Review with Apu, we are preg_matching for NOT starting with category. So the Singleview link will show for gradable items. (including Course total) Response to this and Number 4 (From above). Adding in additional has_capability check for editing grades during processing. There are already 3 capability checks on loading the Singleview. I prefer to change it as you have suggested but haven't had the go ahead. Answered in 2 Reworded old usage of Quick Edit. Corrected. Apu and I are currently looking into a bug with the navigation and pagination in the user screen. EDIT: Fixed, pagination never was implemented for the User screen. Made sure it doesn't try to paginated by default.
            Hide
            damyon Damyon Wiese added a comment -

            I have another look at the final patch and the things I mentioned before look fixed. Sending for integration. Thanks everyone.

            Show
            damyon Damyon Wiese added a comment - I have another look at the final patch and the things I mentioned before look fixed. Sending for integration. Thanks everyone.
            Hide
            marina Marina Glancy added a comment -

            Zac, please don't forget the missing 'notavailable' string!

            Show
            marina Marina Glancy added a comment - Zac, please don't forget the missing 'notavailable' string!
            Hide
            poltawski Dan Poltawski added a comment - - edited

            [Edit: Dan was an idiot]

            Show
            poltawski Dan Poltawski added a comment - - edited [Edit: Dan was an idiot]
            Hide
            poltawski Dan Poltawski added a comment -

            Hi All,

            Sorry, I appreciate that the initial code came from a LSU and might not have been meeting our coding standards, but I am extremely disappointed that we have not made more of an effort to get it to our standards in this issue.

            1. Please can you reduce the amount of issues reported by codechecker on this issue, 1 in 10 lines havign a coding style problem is not a good ratio (and there are other weirdeties not reported by codechecker like global variable definitions in random places).
            2. Same for phpdocs, I think we can do better.
            3. Are those copyrights assigning these classes to Moodle Pty correct? Doesn't seem like it to me.
            4. There are various places with too much 'clever' coding going on. I think this is not going to be good for maintance and i'm not keen to see this kind of stuff landing. Examples:

             public function __construct() {
                   $args = func_get_args();
                   $this->get_arg_or_nothing($args, 0, 'grade');
                   $this->get_arg_or_nothing($args, 1, 'tabindex');
            }
            

            list($___, $item) = explode('singleview_', get_class($this));
            

            5. Simple changes to Moodle APis like:

             $can_use = (
            +        has_capability('gradereport/singleview:view', $context) and
            +        has_capability('moodle/grade:viewall', $context) and
            +        has_capability('moodle/grade:edit', $context)
            +    );
            

            To has_any_capability()
            6. Regarding;

            1. Review with Apu, we are preg_matching for NOT starting with category. So the Singleview link will show for gradable items. (including Course total)

            Looking at 830 of grade/report/grader/lib.php it looks to me that you should using !== instead. In that code path it seems hat that code path $type is set to simply 'category', not some match like you are doing in other parts of the code. Since its not doing any matching it definitely should not be preg_match and should be changed because its confusing (see these two questions).
            7. Shouldn't the capability match gradereport/grader:view and also copy permissions from there?
            8. Unused code like:

            function _s($key, $a = null) {
               return get_string($key, 'gradereport_singleview', $a);
            }
            

            9. Messy commit history with conflict markers and fixup commits. https://docs.moodle.org/dev/Coding_style#Git_commits

            I know that people are keen on this feature for this release and I acknowledge that at this late stage I might be overruled on my objections, but applying the rules i've used to accept or reject other issues landing in the past it would not be fair for me to simply ignore all these issues and let it through. Other people spend a significant amount of their time reworking their code to meet our guidelines and we have not set an example and done this here.

            Show
            poltawski Dan Poltawski added a comment - Hi All, Sorry, I appreciate that the initial code came from a LSU and might not have been meeting our coding standards, but I am extremely disappointed that we have not made more of an effort to get it to our standards in this issue. 1. Please can you reduce the amount of issues reported by codechecker on this issue, 1 in 10 lines havign a coding style problem is not a good ratio (and there are other weirdeties not reported by codechecker like global variable definitions in random places). 2. Same for phpdocs, I think we can do better. 3. Are those copyrights assigning these classes to Moodle Pty correct? Doesn't seem like it to me. 4. There are various places with too much 'clever' coding going on. I think this is not going to be good for maintance and i'm not keen to see this kind of stuff landing. Examples: public function __construct() { $args = func_get_args(); $this->get_arg_or_nothing($args, 0, 'grade'); $this->get_arg_or_nothing($args, 1, 'tabindex'); } list($___, $item) = explode('singleview_', get_class($this)); 5. Simple changes to Moodle APis like: $can_use = ( + has_capability('gradereport/singleview:view', $context) and + has_capability('moodle/grade:viewall', $context) and + has_capability('moodle/grade:edit', $context) + ); To has_any_capability() 6. Regarding; 1. Review with Apu, we are preg_matching for NOT starting with category. So the Singleview link will show for gradable items. (including Course total) Looking at 830 of grade/report/grader/lib.php it looks to me that you should using !== instead. In that code path it seems hat that code path $type is set to simply 'category', not some match like you are doing in other parts of the code. Since its not doing any matching it definitely should not be preg_match and should be changed because its confusing (see these two questions). 7. Shouldn't the capability match gradereport/grader:view and also copy permissions from there? 8. Unused code like: function _s($key, $a = null) { return get_string($key, 'gradereport_singleview', $a); } 9. Messy commit history with conflict markers and fixup commits. https://docs.moodle.org/dev/Coding_style#Git_commits I know that people are keen on this feature for this release and I acknowledge that at this late stage I might be overruled on my objections, but applying the rules i've used to accept or reject other issues landing in the past it would not be fair for me to simply ignore all these issues and let it through. Other people spend a significant amount of their time reworking their code to meet our guidelines and we have not set an example and done this here.
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            damyon Damyon Wiese added a comment -

            I will have a go at cleaning this up today...

            Show
            damyon Damyon Wiese added a comment - I will have a go at cleaning this up today...
            Hide
            damyon Damyon Wiese added a comment -

            I had a go - it's much cleaner now (I put one mega commit at the end of the branch).

            Dan: All of your points above melted away as I cleaned anything I thought was too smelly.

            The final thing is the copyright, which I see has always been assigned to Moodle in this code, so I left it.

            Sending for peer review.

            Show
            damyon Damyon Wiese added a comment - I had a go - it's much cleaner now (I put one mega commit at the end of the branch). Dan: All of your points above melted away as I cleaned anything I thought was too smelly. The final thing is the copyright, which I see has always been assigned to Moodle in this code, so I left it. Sending for peer review.
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-18229 using repository: git://github.com/damyon/moodle.git

            • master (branch: MDL-18229-master | CI Job)
              • Error: The MDL-18229-master branch at git://github.com/damyon/moodle.git exceeds the maximum number of commits ( 261 > 100)

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-18229 using repository: git://github.com/damyon/moodle.git master (branch: MDL-18229-master | CI Job ) Error: The MDL-18229 -master branch at git://github.com/damyon/moodle.git exceeds the maximum number of commits ( 261 > 100) More information about this report
            Hide
            zac Zachary Durber added a comment - - edited
            • Left-Right navigation between activities or users missing.
            • User bulk edit (and bulk exclude) causes:

              Fatal error: Class 'gradereport_singleview\local\screen\grade_item' not found in /home/vagrant/moodles/integration_master/moodle/grade/report/singleview/classes/local/screen/tablelike.php on line 143

            • Fixing that by adding use grade_item at the top of that file causes.

              Notice: Trying to get property of non-object in /home/vagrant/moodles/integration_master/moodle/grade/report/singleview/classes/local/screen/tablelike.php on line 149
               
              Notice: Trying to get property of non-object in /home/vagrant/moodles/integration_master/moodle/grade/report/singleview/classes/local/screen/tablelike.php on line 152
               
              Fatal error: Class 'gradereport_singleview\local\screen\grade_grade' not found in /home/vagrant/moodles/integration_master/moodle/grade/report/singleview/classes/local/screen/tablelike.php on line 157

            • Excluding en-masse for users on a per-activity basis:

              Fatal error: Class 'gradereport_singleview\local\ui\grade_grade' not found in /home/vagrant/moodles/integration_master/moodle/grade/report/singleview/classes/local/ui/exclude.php on line 99

            Show
            zac Zachary Durber added a comment - - edited Left-Right navigation between activities or users missing. User bulk edit (and bulk exclude) causes: Fatal error: Class 'gradereport_singleview\local\screen\grade_item' not found in /home/vagrant/moodles/integration_master/moodle/grade/report/singleview/classes/local/screen/tablelike.php on line 143 Fixing that by adding use grade_item at the top of that file causes. Notice: Trying to get property of non-object in /home/vagrant/moodles/integration_master/moodle/grade/report/singleview/classes/local/screen/tablelike.php on line 149   Notice: Trying to get property of non-object in /home/vagrant/moodles/integration_master/moodle/grade/report/singleview/classes/local/screen/tablelike.php on line 152   Fatal error: Class 'gradereport_singleview\local\screen\grade_grade' not found in /home/vagrant/moodles/integration_master/moodle/grade/report/singleview/classes/local/screen/tablelike.php on line 157 Excluding en-masse for users on a per-activity basis: Fatal error: Class 'gradereport_singleview\local\ui\grade_grade' not found in /home/vagrant/moodles/integration_master/moodle/grade/report/singleview/classes/local/ui/exclude.php on line 99
            Hide
            zac Zachary Durber added a comment -

            Pagination errors. When clicking between pages in an M sized course.

            Fatal error: Call to a member function is_manual_item() on a non-object in /home/vagrant/moodles/integration_master/moodle/grade/report/singleview/classes/local/screen/grade.php on line 149
            

            Show
            zac Zachary Durber added a comment - Pagination errors. When clicking between pages in an M sized course. Fatal error: Call to a member function is_manual_item() on a non-object in /home/vagrant/moodles/integration_master/moodle/grade/report/singleview/classes/local/screen/grade.php on line 149
            Hide
            damyon Damyon Wiese added a comment -

            Fixed all those issues Zac - this is ready for another look.

            Show
            damyon Damyon Wiese added a comment - Fixed all those issues Zac - this is ready for another look.
            Hide
            zac Zachary Durber added a comment -

            Thanks Damyon. Looking good.

            1. Course total is using the old (incorrect) average symbol, not the Greek epsilon.
            2. Bulk exclusions via the user screen results in NOTE: I actually couldn't reproduce this after it happened once.

              Fatal error: Class 'gradereport_singleview\local\ui\grade_grade' not found in /home/vagrant/moodles/integration_master/moodle/grade/report/singleview/classes/local/ui/exclude.php on line 99

            3. Exclude column not always included for some grade items but the header remains. i.e. if you add a new grade item (not a assignment, quiz etc) directly to the grade book and view it in single view.
            4. Bulk insert for grade resulted in

              Fatal error: Class 'gradereport_singleview\local\screen\bulk_insert' not found in /home/vagrant/moodles/integration_master/moodle/grade/report/singleview/classes/local/screen/grade.php on line 277

              Had all grades clicked to override, exclude, bulk insert to 12 and one feedback field filled in with random data.

            Show
            zac Zachary Durber added a comment - Thanks Damyon. Looking good. Course total is using the old (incorrect) average symbol, not the Greek epsilon. Bulk exclusions via the user screen results in NOTE: I actually couldn't reproduce this after it happened once. Fatal error: Class 'gradereport_singleview\local\ui\grade_grade' not found in /home/vagrant/moodles/integration_master/moodle/grade/report/singleview/classes/local/ui/exclude.php on line 99 Exclude column not always included for some grade items but the header remains. i.e. if you add a new grade item (not a assignment, quiz etc) directly to the grade book and view it in single view. Bulk insert for grade resulted in Fatal error: Class 'gradereport_singleview\local\screen\bulk_insert' not found in /home/vagrant/moodles/integration_master/moodle/grade/report/singleview/classes/local/screen/grade.php on line 277 Had all grades clicked to override, exclude, bulk insert to 12 and one feedback field filled in with random data.
            Hide
            zac Zachary Durber added a comment - - edited

            Rebased and added fixes for the above.

            1. There is already a issue for this in the gradebook MDL-47349
            2. Bulk exclusion fixed problem.
            3. Cannot reproduce.
            4. Fixed bulk insert problem.
            Show
            zac Zachary Durber added a comment - - edited Rebased and added fixes for the above. There is already a issue for this in the gradebook MDL-47349 Bulk exclusion fixed problem. Cannot reproduce. Fixed bulk insert problem.
            Hide
            zac Zachary Durber added a comment - - edited

            Pushing for integration, adding MDL-47562 as being blocked by this as it contains UI changes that we'd like to have for Singleview as well. The critical UX changes in that story have already been applied to this branch. (i.e. refreshing the page on update instead of dropping the user to the grade report).

            Show
            zac Zachary Durber added a comment - - edited Pushing for integration, adding MDL-47562 as being blocked by this as it contains UI changes that we'd like to have for Singleview as well. The critical UX changes in that story have already been applied to this branch. (i.e. refreshing the page on update instead of dropping the user to the grade report).
            Hide
            poltawski Dan Poltawski added a comment -

            This should be held, its landing too late and we should learn to get things in earlier.

            But I am not adding the integration_held flag because I am pretty sure Martin Dougiamas wants it.

            Show
            poltawski Dan Poltawski added a comment - This should be held, its landing too late and we should learn to get things in earlier. But I am not adding the integration_held flag because I am pretty sure Martin Dougiamas wants it.
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks guys, this is much better. Integrated to master.

            Note - I see there are still use of dropdowns for selecting users here, Mark McKay contacted me privately to say we should get rid of that because it doesn't scale. Hopefully one of those issues is addressing that.

            Show
            poltawski Dan Poltawski added a comment - Thanks guys, this is much better. Integrated to master. Note - I see there are still use of dropdowns for selecting users here, Mark McKay contacted me privately to say we should get rid of that because it doesn't scale. Hopefully one of those issues is addressing that.
            Hide
            dougiamas Martin Dougiamas added a comment -

            Not checked, but Petr raised this in dev chat:

            petr.skoda 9:07 am
            why is there no sesskey protection in grade/report/singleview/*?
            why is it creating object properties on the fly?

            Show
            dougiamas Martin Dougiamas added a comment - Not checked, but Petr raised this in dev chat: petr.skoda 9:07 am why is there no sesskey protection in grade/report/singleview/*? why is it creating object properties on the fly?
            Hide
            skodak Petr Skoda added a comment - - edited

            Fatal error: Class 'gradereport_singleview\local\ui\stdClass' not found in /Users/skodak/server/workspace/moodle/grade/report/singleview/classes/local/ui/finalgrade.php on line 168

            when entering ridiculously high number (or negative number) in user single view

            Show
            skodak Petr Skoda added a comment - - edited Fatal error: Class 'gradereport_singleview\local\ui\stdClass' not found in /Users/skodak/server/workspace/moodle/grade/report/singleview/classes/local/ui/finalgrade.php on line 168 when entering ridiculously high number (or negative number) in user single view
            Hide
            skodak Petr Skoda added a comment -

            there is a CSRF vulnerability which allows grade modification

            Show
            skodak Petr Skoda added a comment - there is a CSRF vulnerability which allows grade modification
            Hide
            skodak Petr Skoda added a comment -

            There does not seem to be a way to go from single activity view to single user view or the other way.

            Show
            skodak Petr Skoda added a comment - There does not seem to be a way to go from single activity view to single user view or the other way.
            Hide
            skodak Petr Skoda added a comment -

            Update button duplicated at the top and bottom seems to be agains the recent trends - quite recently it was removed from the user management page.

            Show
            skodak Petr Skoda added a comment - Update button duplicated at the top and bottom seems to be agains the recent trends - quite recently it was removed from the user management page.
            Hide
            damyon Damyon Wiese added a comment -

            "There does not seem to be a way to go from single activity view to single user view or the other way." - click on the edit icon next to a User / Activity.

            Show
            damyon Damyon Wiese added a comment - "There does not seem to be a way to go from single activity view to single user view or the other way." - click on the edit icon next to a User / Activity.
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Petr for checking the code btw!

            Show
            damyon Damyon Wiese added a comment - Thanks Petr for checking the code btw!
            Hide
            skodak Petr Skoda added a comment -

            Not a single hit of "moodle/site:accessallgroups" - how does the separate group mode work in there?

            Show
            skodak Petr Skoda added a comment - Not a single hit of "moodle/site:accessallgroups" - how does the separate group mode work in there?
            Hide
            skodak Petr Skoda added a comment -

            User names do not seem to be processed properly $user = $DB->get_record('user', array('id' => $userid), 'id, firstname, alternatename, lastname');

            Also the absence of get_all_user_name_fields() indicates it does not work with new user name fields, see:

            $this->items = get_role_users($roleids, $this->context, false, '','u.lastname, u.firstname', null, $this->groupid);

            Show
            skodak Petr Skoda added a comment - User names do not seem to be processed properly $user = $DB->get_record('user', array('id' => $userid), 'id, firstname, alternatename, lastname'); Also the absence of get_all_user_name_fields() indicates it does not work with new user name fields, see: $this->items = get_role_users($roleids, $this->context, false, '','u.lastname, u.firstname', null, $this->groupid);
            Hide
            damyon Damyon Wiese added a comment -

            It relies on the logic in the grade_report ( /grade/report/lib.php ) class to handle groups.

            Show
            damyon Damyon Wiese added a comment - It relies on the logic in the grade_report ( /grade/report/lib.php ) class to handle groups.
            Hide
            skodak Petr Skoda added a comment -

            $itemtype = optional_param('item', $defaulttype, PARAM_TEXT);

            please stop abusing PARAM_TEXT, this may not be XSS but it allows anybody to produce arbitrary error messages here

            Show
            skodak Petr Skoda added a comment - $itemtype = optional_param('item', $defaulttype, PARAM_TEXT); please stop abusing PARAM_TEXT, this may not be XSS but it allows anybody to produce arbitrary error messages here
            Hide
            skodak Petr Skoda added a comment -

            I dare to disagree, the userids are taken directly from _POST and they do not seem to be verified at all before updating the grades in DB...

            Show
            skodak Petr Skoda added a comment - I dare to disagree, the userids are taken directly from _POST and they do not seem to be verified at all before updating the grades in DB...
            Hide
            salvetore Michael de Raadt added a comment -

            I've separated out the non-essential sub-tasks of this issue into separate linked them back. This issue, once identified problems are resolved, should be able to be integrated. QA test writing on this can then proceed.

            Show
            salvetore Michael de Raadt added a comment - I've separated out the non-essential sub-tasks of this issue into separate linked them back. This issue, once identified problems are resolved, should be able to be integrated. QA test writing on this can then proceed.
            Hide
            marina Marina Glancy added a comment -

            Behat is failing:

            (::) failed steps (::)
             
            01. Option matching locator "'studentawesemo (Student) 4'" not found.
                In step `And I click on "studentawesemo (Student) 4" "option"'.        # behat_general::i_click_on()
                From scenario `I can update grades, add feedback and exclude grades.'. # /home/marina/repositories/naturalweights/moodle/grade/report/singleview/tests/behat/singleview.feature:40
                Of feature `We can use Single view'.                                   # /home/marina/repositories/naturalweights/moodle/grade/report/singleview/tests/behat/singleview.feature
             
            02. Option matching locator "'studentbro (Student) 1'" not found.
                In step `Then I click on "studentbro (Student) 1" "option"'.           # behat_general::i_click_on()
                From scenario `Navigation works in the Single view.'.                  # /home/marina/repositories/naturalweights/moodle/grade/report/singleview/tests/behat/singleview.feature:75
                Of feature `We can use Single view'.                                   # /home/marina/repositories/naturalweights/moodle/grade/report/singleview/tests/behat/singleview.feature
             
            3 scenarios (1 passed, 2 failed)
            65 steps (40 passed, 23 skipped, 2 failed)
            0m59.834s
            

            Show
            marina Marina Glancy added a comment - Behat is failing: (::) failed steps (::)   01. Option matching locator "'studentawesemo (Student) 4'" not found. In step `And I click on "studentawesemo (Student) 4" "option"'. # behat_general::i_click_on() From scenario `I can update grades, add feedback and exclude grades.'. # /home/marina/repositories/naturalweights/moodle/grade/report/singleview/tests/behat/singleview.feature:40 Of feature `We can use Single view'. # /home/marina/repositories/naturalweights/moodle/grade/report/singleview/tests/behat/singleview.feature   02. Option matching locator "'studentbro (Student) 1'" not found. In step `Then I click on "studentbro (Student) 1" "option"'. # behat_general::i_click_on() From scenario `Navigation works in the Single view.'. # /home/marina/repositories/naturalweights/moodle/grade/report/singleview/tests/behat/singleview.feature:75 Of feature `We can use Single view'. # /home/marina/repositories/naturalweights/moodle/grade/report/singleview/tests/behat/singleview.feature   3 scenarios (1 passed, 2 failed) 65 steps (40 passed, 23 skipped, 2 failed) 0m59.834s
            Hide
            zac Zachary Durber added a comment -

            Commits added to fix sesskey and large inputs.

            Looking into behat tests now.

            Show
            zac Zachary Durber added a comment - Commits added to fix sesskey and large inputs. Looking into behat tests now.
            Hide
            zac Zachary Durber added a comment -

            Behat tests repaired. Additional bug picked up and fixed too.

            Show
            zac Zachary Durber added a comment - Behat tests repaired. Additional bug picked up and fixed too.
            Hide
            marina Marina Glancy added a comment -

            Hi Zac, I can't even view the report now:

            Fatal error: Class 'gradereport_singleview\local\ui\html_writer' not found in /home/marina/repositories/naturalweights/moodle/grade/report/singleview/classes/local/ui/dropdown_attribute.php on line 95
            

            Show
            marina Marina Glancy added a comment - Hi Zac, I can't even view the report now: Fatal error: Class 'gradereport_singleview\local\ui\html_writer' not found in /home/marina/repositories/naturalweights/moodle/grade/report/singleview/classes/local/ui/dropdown_attribute.php on line 95
            Hide
            marina Marina Glancy added a comment -

            I pulled your fixes Zac and added a commit with fix for the error I noticed above. Sending back to testing

            Show
            marina Marina Glancy added a comment - I pulled your fixes Zac and added a commit with fix for the error I noticed above. Sending back to testing
            Hide
            lameze Simey Lameze added a comment -

            Thanks Zac, works as described in your testing instructions. Passing

            Show
            lameze Simey Lameze added a comment - Thanks Zac, works as described in your testing instructions. Passing
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks for your contributions, this change is now upstream!

            The trick is to fix the problem you have, rather than the problem you want.

            – Braham Cohen

            Show
            poltawski Dan Poltawski added a comment - Thanks for your contributions, this change is now upstream! The trick is to fix the problem you have, rather than the problem you want. – Braham Cohen
            Hide
            tsala Helen Foster added a comment -

            Removing qa_test_required label as this new featured is now covered by two QA tests - MDLQA-7623 and MDLQA-7625 (MDLQA-7624 and MDLQA-7626 respectively in the 2.8 cycle).

            Please comment if either of the tests needs amending or adding to.

            Show
            tsala Helen Foster added a comment - Removing qa_test_required label as this new featured is now covered by two QA tests - MDLQA-7623 and MDLQA-7625 ( MDLQA-7624 and MDLQA-7626 respectively in the 2.8 cycle). Please comment if either of the tests needs amending or adding to.
            Hide
            marycooch Mary Cooch added a comment -

            Removing docs_required label as this has been documented in https://docs.moodle.org/28/en/Single_view

            Show
            marycooch Mary Cooch added a comment - Removing docs_required label as this has been documented in https://docs.moodle.org/28/en/Single_view

              People

              • Votes:
                19 Vote for this issue
                Watchers:
                24 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Nov/14

                  Agile