Moodle
  1. Moodle
  2. MDL-22999

Allow maximum points for graded activities to exceed 100

    Details

    • Testing Instructions:
      Hide

      TC1 – Adding a graded activity and setting the maximum grade point

      1. Steps
        1. User adds a graded activity and selects the following:
        2. Site wide grade point maximum is set to 900, grade point default is set to 800.
        3. Grade Type - 'point'
        4. Scale - disabled because Grade Type is not 'scale'
        5. Maximum points - 600
      2. Expected Result
        1. When the add activity page loads, under the "grade" section. The "grade element" should have "points" selected for type, and 800 as the default value for maximum points.
        2. The user submits the form and the value stored in the grade column of the module's table is 600.

      TC2 – Adding a graded activity and selecting a scale grade

      1. Steps
        1. User adds a graded activity and selects the following:
        2. Site wide grade point maximum is set to 900, grade point default is set to 800
        3. Grade Type = 'scale'
        4. Scale - Scale value
        5. Maximum points - automatically disabled because Grade Type is not 'point'
      2. Expected Result
        1. When the add activity page loads, under the "grade" section. The "grade element" should have "points" selected for type, and 800 as the default value for maximum points.
        2. The user submits the form and the value stored in the grade column of the module's table is a negative integer.

      TC3 – Adding a graded activity and selecting 'no grade'

      1. Steps
        1. User adds a graded activity and selects the following:
        2. Site wide grade point maximum is set to 900, grade point default is set to 800.
        3. Grade Type = 'No grade'
        4. Scale - disabled because Grade Type is not 'scale'
        5. Maximum points - disabled because Grade Type is not 'point'
      2. Expected Result
        1. When the add activity page loads, under the "grade" section. The "grade element" should have "points" selected for type, and 800 as the default value for maximum points.
        2. The user submits the form and the value stored in the grade column of the module's table is 0.

      TC4 – Adding a graded activity and setting the maximum grade point higher than the site imposed maximum

      1. Steps
        1. User adds a graded activity and selects the following:
        2. Site wide grade point maximum is set to 900, grade point default is set to 800.
        3. Grade Type = 'Point'
        4. Scale - disabled because Grade Type is not 'scale'
        5. Maximum points - 20000
      2. Expected Result
        1. When the add activity page loads, under the "grade" section. The "grade element" should have "points" selected for type, and 800 as the default value for maximum points.
        2. The user submits the form and validation code returns an error highlighting that the grade point entered is higher than the site imposed maximum.

      TC5 – Updating a graded activity and setting the maximum grade point

      1. Steps
        1. Site wide grade point maximum is set to 900
        2. User updates a graded activity that is using a maximum points value of 600
      2. Expected Results
        1. The Grade Type dropdown value is set to 'Point'
        2. The Scale dropdown is disabled
        3. The Maximum points text field is populated with 600

      TC6 – Updating a graded activity, whose grade was previously set to 'no grade'

      1. Steps
        1. Site wide grade point maximum is set to 900
        2. User updates a graded activity that is using a grade point value of 0
      2. Expected Results
        1. The Grade Type dropdown value is set to 'No grade'
        2. The Scale dropdown is disabled
        3. The Maximum points text field is disabled

      TC7 – Updating a graded activity, whose grade was previously using a scale

      1. Steps
        1. Site wide grade point maximum is set to 900
        2. User updates a graded activity that is using a scale value of -1
      2. Expected Results
        1. The Grade Type dropdown value is set to 'Scale'
        2. The Scale dropdown value is set to the '-1' entry
        3. The Maximum points text field is disabled

      TC8 – Updating a graded activity and setting a grade point maximum; then changing the site wide grade point maximum to a value lower than the value used by the activity

      1. Steps
        1. Set site wide grade point maximum to 900.
        2. Update a grade activity, and set it to point grading, with a maximum of 600. Save.
        3. Set site wide grade point maximum to 100.
        4. Edit the activity again.
      2. Expected Results
        1. The Grade Type dropdown value is set to 'Point'
        2. The Scale dropdown is disabled
        3. The Maximum points text field is populated with 600
        4. The user submits the form and validation code returns an error highlighting that the grade point entered is higher than the site imposed maximum.

      TC9 – Adding a graded activity and setting the grade point maximum. Change the site wide grade point maximum to a value lower than the value used by the activity. Perform a backup and restore of the graded activity

      1. Steps
        1. Site wide grade point maximum is set to 1000
        2. User creates a graded activity
          1. Grade Type = 'Point'
          2. Scale - disabled because Grade Type is not 'scale'
          3. Maximum points - 1000
        3. Saves the form
        4. Creates a backup of the course, activities and user data
        5. Site wide grade point maximum is set to 100
        6. User restores the backuped course with activities and user data
        7. User edits the graded activity
      2. Expected Results
        1. The Grade Type dropdown value is set to 'Point'
        2. The Scale dropdown is disabled
        3. The Grade Point text field is populated with 1000
        4. The user submits the form and validation code returns an error highlighting that the grade point entered is higher than the site imposed maximum.

      TC10 – Adding a graded activity and setting the grade point maximum. Change the grade point maximum. Perform a backup and restore of the graded activity

      1. Steps
        1. Site wide grade point maximum is set to 1000
        2. User creates a graded activity
          1. Grade Type = 'Point'
          2. Scale - disabled because Grade Type is not 'scale'
          3. Maximum points - 30
        3. Saves the form
        4. Creates a backup of the course, activities and user data
        5. Site wide grade point maximum is set to 100
        6. User restores the backuped course with activities and user data
        7. User edits the graded activity
      2. Expected Results
        1. The Grade Type dropdown value is set to 'Point'
        2. The Scale dropdown is disabled
        3. The Grade Point text field is populated with 30
        4. The user submits the form and the value stored in the grade column of the module's table is 30.

      TC11 – Adding a grade activity snd setting a scale grade. Perform a backup of the activity. Removed the sale used by the activity by removing it from the course (or site). Restore the activity

      1. Steps
        1. Site wide grade point maximum is set to 1000
        2. User creates a graded activity
          1. Grade Type = 'scale'
          2. Scale - Scale value
          3. Maximum points - automatically disabled because Grade Type is not 'point'
        3. Saves the form
        4. Creates a backup of the course, activities and user data
        5. The scale used in the graded activity is removed
        6. User restores the backuped course with activities and user data
        7. User edits the graded activity
      2. Expected Results
        1. The Grade Type dropdown value is set to 'Scale'
        2. The Scale defaults to the "standard scale value" of -1
        3. The Grade Point text field is disabled
        4. The user submits the form and the value stored in the grade column of the module's table is -1.

      TC12 – Adding an activity that uses rating.

      1. Steps
        1. Site wide grade point maximum is set to 100
        2. User adds a forum activity to a course.
        3. User expands "Ratings" section and changes "Aggregate Type" to a value other than "No Ratings"
        4. User changes scale type to "scale"
        5. User changes scale type to "point"
        6. User enters a value less than the maximum grade (ex. 80) into the Maximum points input box, and saves the activity.
      2. Expected Results
        1. When first expanding the "Ratings" section, all "Scale" inputs should be disabled (type/scale/maximum grade)
        2. After changing the "Aggregate Type" to a value other than "No Ratings", The "type" select box should be enabled, and set to "Point". The Maximum Grade input box should be enabled and set to the sitewide maximum grade setting (100)
        3. When changing scale type to "scale", the maximum grade input should become disabled, and the scale select box should be enabled.
        4. When changing scale type back to "point", the maximum grade input should become enabled, and the scale select box should be disabled.
        5. After entering the maximum grade value, and saving the activity, the activity should save successfully.

      TC13 – Updating an activity that uses ratings, whose rating scale was previously set to "point"

      1. Steps
        1. Site wide grade point maximum is set to 100
        2. User updates a forum activity that is using ratings and a maximum grade value of 90
      2. Expected Results
        1. The Scale Type select box value is set to 'Point'
        2. The Scale select box is disabled.
        3. The Maximum points text field is enabled and set to 90.

      TC14 – Updating an activity that uses ratings, whose rating scale was previously set to "scale"

      1. Steps
        1. Site wide grade point maximum is set to 100
        2. User updates a forum activity that is using ratings and a scale value of -1
      2. Expected Results
        1. The Scale Type select box value is set to 'Scale'
        2. The Scale select box value is set to the '-1' entry
        3. The Maximum points text field is disabled

      TC15 – Adding a workshop with Accumulative Grading

      1. Steps
        1. Site wide grade point maximum is set to 100
        2. User adds a workshop activity to a course. A name is set, and grading strategy is set to "accumulative grading"
        3. User clicks "save and display", then clicks "edit assessment form"
        4. Under "best possible grade / scale to use", the type is changed to "scale"
        5. Under "best possible grade / scale to use", the type is changed to "point"
        6. An integer is input into the maximum grade box, and the user clicks "save and close"
      2. Expected results
        1. When first viewing the edit assessment form page, next to "best possible grade / scale to use" a type select box, a scale select box, and a maximum grade text box is present. The type select box is set to "point", the scale select box is disabled, and the maximum grade box is set to 10
        2. When "best possible grade / scale to use" type is changed to "scale", the scale select box is enabled, and the maximum grade box is disabled.

      TC16 – Setting the maximum and default grade point values.

      1. Steps
        1. User navigates to Site Administration > Grades > General Settings
        2. User sets the maximum grade point value to 100. and clicks "Save Changes"
        3. User leaves the maximum grade point value at 100, sets the grade point default value to 101, and clicks "Save Changes"
        4. User leaves the maximum grade point value at 100, sets the grade point default value to 90, and clicks "Save Changes"
        5. User sets the maximum grade point value to 80, leaves the grade point default value at 90, and clicks "Save Changes"
        6. User sets the maximum grade point value to 90, leaves the grade point default value at 90, and clicks "Save Changes"
        7. User sets the maximum grade point value to to 85, changes the grade point default value to 80, and clicks "Save Changes"
        8. User sets the maximum grade point value to to 80, changes the grade point default value to 85, and clicks "Save Changes"
        9. User sets the maximum grade point value back to 100, the default grade point value back to 100, and clicks "Save Changes"
      2. Expected Results.
        1. After step 3, a validation error should be shown, stating that the default value cannot exceed the maximum value. The values should not have saved.
        2. After step 4, the values should save successfully.
        3. After step 5, a validation error should be shown, stating that the default value cannot exceed the maximum value. The values should not have saved.
        4. After step 6, the values should save successfully.
        5. After step 7, the values should save successfully.
        6. After step 8, a validation error should be shown, stating that the default value cannot exceed the maximum value. The values should not have saved.
        7. After step 9, the values should save successfully.
      Show
      TC1 – Adding a graded activity and setting the maximum grade point Steps User adds a graded activity and selects the following: Site wide grade point maximum is set to 900, grade point default is set to 800. Grade Type - 'point' Scale - disabled because Grade Type is not 'scale' Maximum points - 600 Expected Result When the add activity page loads, under the "grade" section. The "grade element" should have "points" selected for type, and 800 as the default value for maximum points. The user submits the form and the value stored in the grade column of the module's table is 600. TC2 – Adding a graded activity and selecting a scale grade Steps User adds a graded activity and selects the following: Site wide grade point maximum is set to 900, grade point default is set to 800 Grade Type = 'scale' Scale - Scale value Maximum points - automatically disabled because Grade Type is not 'point' Expected Result When the add activity page loads, under the "grade" section. The "grade element" should have "points" selected for type, and 800 as the default value for maximum points. The user submits the form and the value stored in the grade column of the module's table is a negative integer. TC3 – Adding a graded activity and selecting 'no grade' Steps User adds a graded activity and selects the following: Site wide grade point maximum is set to 900, grade point default is set to 800. Grade Type = 'No grade' Scale - disabled because Grade Type is not 'scale' Maximum points - disabled because Grade Type is not 'point' Expected Result When the add activity page loads, under the "grade" section. The "grade element" should have "points" selected for type, and 800 as the default value for maximum points. The user submits the form and the value stored in the grade column of the module's table is 0. TC4 – Adding a graded activity and setting the maximum grade point higher than the site imposed maximum Steps User adds a graded activity and selects the following: Site wide grade point maximum is set to 900, grade point default is set to 800. Grade Type = 'Point' Scale - disabled because Grade Type is not 'scale' Maximum points - 20000 Expected Result When the add activity page loads, under the "grade" section. The "grade element" should have "points" selected for type, and 800 as the default value for maximum points. The user submits the form and validation code returns an error highlighting that the grade point entered is higher than the site imposed maximum. TC5 – Updating a graded activity and setting the maximum grade point Steps Site wide grade point maximum is set to 900 User updates a graded activity that is using a maximum points value of 600 Expected Results The Grade Type dropdown value is set to 'Point' The Scale dropdown is disabled The Maximum points text field is populated with 600 TC6 – Updating a graded activity, whose grade was previously set to 'no grade' Steps Site wide grade point maximum is set to 900 User updates a graded activity that is using a grade point value of 0 Expected Results The Grade Type dropdown value is set to 'No grade' The Scale dropdown is disabled The Maximum points text field is disabled TC7 – Updating a graded activity, whose grade was previously using a scale Steps Site wide grade point maximum is set to 900 User updates a graded activity that is using a scale value of -1 Expected Results The Grade Type dropdown value is set to 'Scale' The Scale dropdown value is set to the '-1' entry The Maximum points text field is disabled TC8 – Updating a graded activity and setting a grade point maximum; then changing the site wide grade point maximum to a value lower than the value used by the activity Steps Set site wide grade point maximum to 900. Update a grade activity, and set it to point grading, with a maximum of 600. Save. Set site wide grade point maximum to 100. Edit the activity again. Expected Results The Grade Type dropdown value is set to 'Point' The Scale dropdown is disabled The Maximum points text field is populated with 600 The user submits the form and validation code returns an error highlighting that the grade point entered is higher than the site imposed maximum. TC9 – Adding a graded activity and setting the grade point maximum. Change the site wide grade point maximum to a value lower than the value used by the activity. Perform a backup and restore of the graded activity Steps Site wide grade point maximum is set to 1000 User creates a graded activity Grade Type = 'Point' Scale - disabled because Grade Type is not 'scale' Maximum points - 1000 Saves the form Creates a backup of the course, activities and user data Site wide grade point maximum is set to 100 User restores the backuped course with activities and user data User edits the graded activity Expected Results The Grade Type dropdown value is set to 'Point' The Scale dropdown is disabled The Grade Point text field is populated with 1000 The user submits the form and validation code returns an error highlighting that the grade point entered is higher than the site imposed maximum. TC10 – Adding a graded activity and setting the grade point maximum. Change the grade point maximum. Perform a backup and restore of the graded activity Steps Site wide grade point maximum is set to 1000 User creates a graded activity Grade Type = 'Point' Scale - disabled because Grade Type is not 'scale' Maximum points - 30 Saves the form Creates a backup of the course, activities and user data Site wide grade point maximum is set to 100 User restores the backuped course with activities and user data User edits the graded activity Expected Results The Grade Type dropdown value is set to 'Point' The Scale dropdown is disabled The Grade Point text field is populated with 30 The user submits the form and the value stored in the grade column of the module's table is 30. TC11 – Adding a grade activity snd setting a scale grade. Perform a backup of the activity. Removed the sale used by the activity by removing it from the course (or site). Restore the activity Steps Site wide grade point maximum is set to 1000 User creates a graded activity Grade Type = 'scale' Scale - Scale value Maximum points - automatically disabled because Grade Type is not 'point' Saves the form Creates a backup of the course, activities and user data The scale used in the graded activity is removed User restores the backuped course with activities and user data User edits the graded activity Expected Results The Grade Type dropdown value is set to 'Scale' The Scale defaults to the "standard scale value" of -1 The Grade Point text field is disabled The user submits the form and the value stored in the grade column of the module's table is -1. TC12 – Adding an activity that uses rating. Steps Site wide grade point maximum is set to 100 User adds a forum activity to a course. User expands "Ratings" section and changes "Aggregate Type" to a value other than "No Ratings" User changes scale type to "scale" User changes scale type to "point" User enters a value less than the maximum grade (ex. 80) into the Maximum points input box, and saves the activity. Expected Results When first expanding the "Ratings" section, all "Scale" inputs should be disabled (type/scale/maximum grade) After changing the "Aggregate Type" to a value other than "No Ratings", The "type" select box should be enabled, and set to "Point". The Maximum Grade input box should be enabled and set to the sitewide maximum grade setting (100) When changing scale type to "scale", the maximum grade input should become disabled, and the scale select box should be enabled. When changing scale type back to "point", the maximum grade input should become enabled, and the scale select box should be disabled. After entering the maximum grade value, and saving the activity, the activity should save successfully. TC13 – Updating an activity that uses ratings, whose rating scale was previously set to "point" Steps Site wide grade point maximum is set to 100 User updates a forum activity that is using ratings and a maximum grade value of 90 Expected Results The Scale Type select box value is set to 'Point' The Scale select box is disabled. The Maximum points text field is enabled and set to 90. TC14 – Updating an activity that uses ratings, whose rating scale was previously set to "scale" Steps Site wide grade point maximum is set to 100 User updates a forum activity that is using ratings and a scale value of -1 Expected Results The Scale Type select box value is set to 'Scale' The Scale select box value is set to the '-1' entry The Maximum points text field is disabled TC15 – Adding a workshop with Accumulative Grading Steps Site wide grade point maximum is set to 100 User adds a workshop activity to a course. A name is set, and grading strategy is set to "accumulative grading" User clicks "save and display", then clicks "edit assessment form" Under "best possible grade / scale to use", the type is changed to "scale" Under "best possible grade / scale to use", the type is changed to "point" An integer is input into the maximum grade box, and the user clicks "save and close" Expected results When first viewing the edit assessment form page, next to "best possible grade / scale to use" a type select box, a scale select box, and a maximum grade text box is present. The type select box is set to "point", the scale select box is disabled, and the maximum grade box is set to 10 When "best possible grade / scale to use" type is changed to "scale", the scale select box is enabled, and the maximum grade box is disabled. TC16 – Setting the maximum and default grade point values. Steps User navigates to Site Administration > Grades > General Settings User sets the maximum grade point value to 100. and clicks "Save Changes" User leaves the maximum grade point value at 100, sets the grade point default value to 101, and clicks "Save Changes" User leaves the maximum grade point value at 100, sets the grade point default value to 90, and clicks "Save Changes" User sets the maximum grade point value to 80, leaves the grade point default value at 90, and clicks "Save Changes" User sets the maximum grade point value to 90, leaves the grade point default value at 90, and clicks "Save Changes" User sets the maximum grade point value to to 85, changes the grade point default value to 80, and clicks "Save Changes" User sets the maximum grade point value to to 80, changes the grade point default value to 85, and clicks "Save Changes" User sets the maximum grade point value back to 100, the default grade point value back to 100, and clicks "Save Changes" Expected Results. After step 3, a validation error should be shown, stating that the default value cannot exceed the maximum value. The values should not have saved. After step 4, the values should save successfully. After step 5, a validation error should be shown, stating that the default value cannot exceed the maximum value. The values should not have saved. After step 6, the values should save successfully. After step 7, the values should save successfully. After step 8, a validation error should be shown, stating that the default value cannot exceed the maximum value. The values should not have saved. After step 9, the values should save successfully.
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Epic Link:
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-22999-m27
    • Rank:
      1201

      Description

      Activities... most notably assignments have always only allowed whole numbers between 0 and 100. I don't know anyone who hasn't hacked this from the first time they've used Moodle to allow at least 250 points. Also, try to enter 95.5 as an assignment grade from the assignment grading interface. This field should not be a dropdown but a text input of any valid floating point number.

        Issue Links

          Activity

          Hide
          Eric Merrill added a comment -

          These are the same issue. I'm going to be looking into this.

          -eric

          Show
          Eric Merrill added a comment - These are the same issue. I'm going to be looking into this. -eric
          Hide
          Martin Dougiamas added a comment -

          See the linked comment for some subtleties here. But thanks for working on this Eric!

          Show
          Martin Dougiamas added a comment - See the linked comment for some subtleties here. But thanks for working on this Eric!
          Hide
          Robert Puffer added a comment -

          A patch for the 1.9 branch has been available for five months that is running in the CLAMP LAE version of moodle. Its (rightfully?) attached to the issue of which this issue is a clone MDL-9085. Wondering if we could have some review, rejection or whatever to indicate that actual progress is being accomplished. Maybe we could avoid having to shuffle the issue further down the release chain?

          Show
          Robert Puffer added a comment - A patch for the 1.9 branch has been available for five months that is running in the CLAMP LAE version of moodle. Its (rightfully?) attached to the issue of which this issue is a clone MDL-9085 . Wondering if we could have some review, rejection or whatever to indicate that actual progress is being accomplished. Maybe we could avoid having to shuffle the issue further down the release chain?
          Hide
          Chris Follin added a comment -

          As Robert said, a review and some kind of update would be helpful.

          Show
          Chris Follin added a comment - As Robert said, a review and some kind of update would be helpful.
          Hide
          Michael de Raadt added a comment -

          This issue has been around for a while.

          It is now possible to enter part marks in an assignment. The gradebook allows teachers to set marks higher than 100 when "unlimited grades" is turned on. However the assignment module does not allow you to enter a grade higher than 100 at this stage.

          Show
          Michael de Raadt added a comment - This issue has been around for a while. It is now possible to enter part marks in an assignment. The gradebook allows teachers to set marks higher than 100 when "unlimited grades" is turned on. However the assignment module does not allow you to enter a grade higher than 100 at this stage.
          Hide
          Damyon Wiese added a comment -

          Yes, Assignment does not allow you to enter a grade higher than 100. To allow this we would need to add a checkbox to the assignment setting page to allow unlimited grades (and push this value to the gradebook - and keep it in sync with the gradebook).

          Also - not sure without checking which of the other modules mentioned do not allow decimal grades.

          Show
          Damyon Wiese added a comment - Yes, Assignment does not allow you to enter a grade higher than 100. To allow this we would need to add a checkbox to the assignment setting page to allow unlimited grades (and push this value to the gradebook - and keep it in sync with the gradebook). Also - not sure without checking which of the other modules mentioned do not allow decimal grades.
          Hide
          Robert Puffer added a comment -

          Moodle core needs to get used to the idea that assignments must be capable of having a maximum value (weight) greater than 100 without needing to have grades exceed 100%. Why are we having such a hard time getting this through the pipeline?

          • Grade items have a weight of "maximum earnable" unless that weight is altered by having a category container that uses an aggregation of "Weighted mean of grades", whereby the "natural" weight of a grade item (or category) is overridden by an imposed "artificial" weight, normally ascribed to a category or a course total.
          Show
          Robert Puffer added a comment - Moodle core needs to get used to the idea that assignments must be capable of having a maximum value (weight) greater than 100 without needing to have grades exceed 100%. Why are we having such a hard time getting this through the pipeline? Grade items have a weight of "maximum earnable" unless that weight is altered by having a category container that uses an aggregation of "Weighted mean of grades", whereby the "natural" weight of a grade item (or category) is overridden by an imposed "artificial" weight, normally ascribed to a category or a course total.
          Hide
          Mike Churchward added a comment - - edited

          Martin Dougiamas, Michael de Raadt ... What is the possibility of getting this feature into Moodle core? We have a client willing to pay for this, but they need to know that it is possible to get it into core eventually. Essentially, all this needs to be is to change the current point limitation from 100 to some other maximum value (possibly configured at the site level). This is not increasing the point beyond 100%, just allowing for activities to be marked out of 300 (for example). This helps for courses who want to have a maximum number of points in a course and control that weighting with the activities.

          Show
          Mike Churchward added a comment - - edited Martin Dougiamas , Michael de Raadt ... What is the possibility of getting this feature into Moodle core? We have a client willing to pay for this, but they need to know that it is possible to get it into core eventually. Essentially, all this needs to be is to change the current point limitation from 100 to some other maximum value (possibly configured at the site level). This is not increasing the point beyond 100%, just allowing for activities to be marked out of 300 (for example). This helps for courses who want to have a maximum number of points in a course and control that weighting with the activities.
          Hide
          Martin Dougiamas added a comment -

          Mike, if you are able to make a patch I'm sure we can expedite this through with Damyon's help.

          Show
          Martin Dougiamas added a comment - Mike, if you are able to make a patch I'm sure we can expedite this through with Damyon's help.
          Hide
          Martin Dougiamas added a comment -

          To lubricate this it's probably OK to just limit this issue to assignment module only, and to make new linked issues for other modules as appropriate.

          Show
          Martin Dougiamas added a comment - To lubricate this it's probably OK to just limit this issue to assignment module only, and to make new linked issues for other modules as appropriate.
          Hide
          Mike Churchward added a comment -

          Hi Martin -

          We are thinking of changing the "/lib/form/modform.php:onQuickFormEvent" function where it limits the choices to "100". We'll do the testing with all of the core modules that use that function, but preliminary tests show that this would work everywhere. Any module that uses that function already has to deal with maximum point counts that are not 100 - anything between 1 and 100 are valid. So they already know that (for example) 80/80 is 100%. It seems trivial to make them understand that 150/150 is also 100%.

          We'll put forth our proposal which will include all the necessary testing.

          Show
          Mike Churchward added a comment - Hi Martin - We are thinking of changing the "/lib/form/modform.php:onQuickFormEvent" function where it limits the choices to "100". We'll do the testing with all of the core modules that use that function, but preliminary tests show that this would work everywhere. Any module that uses that function already has to deal with maximum point counts that are not 100 - anything between 1 and 100 are valid. So they already know that (for example) 80/80 is 100%. It seems trivial to make them understand that 150/150 is also 100%. We'll put forth our proposal which will include all the necessary testing.
          Hide
          Eric Merrill added a comment -

          To me, just adding more int values is at best a temporary solution. IMO, activites should have the same grade selection interface as grade items in the gradebook - you have a dropdown for selecting what type of grade (scale, value, etc), and then you either select a scale out of a dropdown, or enter the value into a box. It allows you to have arbitrarily sized grades, including decimal values. I'm attaching a screenshot of the grade item interface just for reference.

          Show
          Eric Merrill added a comment - To me, just adding more int values is at best a temporary solution. IMO, activites should have the same grade selection interface as grade items in the gradebook - you have a dropdown for selecting what type of grade (scale, value, etc), and then you either select a scale out of a dropdown, or enter the value into a box. It allows you to have arbitrarily sized grades, including decimal values. I'm attaching a screenshot of the grade item interface just for reference.
          Hide
          Mike Churchward added a comment -

          Eric Merrill I think we could change the forms to mimic the grade book and achieve the same goal. Since there is a function to return the dropdown, it just needs to be modified to include the same type of form elements.

          Show
          Mike Churchward added a comment - Eric Merrill I think we could change the forms to mimic the grade book and achieve the same goal. Since there is a function to return the dropdown, it just needs to be modified to include the same type of form elements.
          Hide
          Mike Churchward added a comment -

          I'll get a proposal of the changes required to do this and post them here soon.

          Show
          Mike Churchward added a comment - I'll get a proposal of the changes required to do this and post them here soon.
          Hide
          Akin Delamarre added a comment - - edited

          Proposed solution:

          Required Code Changes

          Create a new admin setting
          • Create a new admin settings class that extends the 'admin_setting_configtext' class. The new setting class will override the validate() method to ensure the user enters an integer that is greater than 0 but less than or equal to 10000
            • A hard coded maximum of 10000 will be enforced by the admin setting
            • The default value will be 100
          • Modify admin/settings/grades.php by adding the new admin settings field, to allow the user to enter a site wide imposed grade point maximum.

            Example 1

            • User goes into Site Administration -> Grades -> General settings
            • User scolls to the 'Grade point maximum' setting
            • User enters a value of 600
            • User clicks 'Save changes' button
            • Site wide grade point maximum is now 600

            Example 2

            • User goes into Site Administration -> Grades -> General settings
            • User scolls to the 'Grade point maximum' setting
            • User enters a value of 20000
            • User clicks 'Save changes' button
            • The admin setting code notifies the user of the 10000 point limit

            Example 3

            • User goes into Site Administration -> Grades -> General settings
            • User scolls to the 'Grade point maximum' setting
            • User enters a value of 0
            • User clicks 'Save changes' button
            • The admin setting code notifies the user that the value must be greater 0 but less than 10000

            Example 4

            • User goes into Site Administration -> Grades -> General settings
            • User scolls to the 'Grade point maximum' setting
            • User leaves the field blank
            • User clicks 'Save changes' button
            • Site wide grade point maximum is now 100 (the default value)

            Example 5

            • User goes into Site Administration -> Grades -> General settings
            • User scolls to the 'Grade point maximum' setting
            • User enters a non-numeric value or negative number
            • User clicks 'Save changes' button
            • The admin setting code notifies the user that the value must be greater 0 but less than 10000
          Create a new formslib element
          • Create a new form definition for displaying the grading elements (defined as an instance of MoodleQuickForm_group).
          • In expotValue() the selected options must be analyized such that one value is returned instead of 3 (one for each grading element). The reason for this is that all graded activities currently use one column, in the module table, to store the grade value. A grade value of '- x' denotes a scale where '-' tells the module a grade scale is to be used and 'x' is the id of the scale selected. Where as a grade value of 'x' denotes a grade point is to be used and 'x' is any positive number.

            Example 1
            User adds a graded activity and selects the following:

            • Site wide grade point maximum is set to 900
            • Grade Type - 'point'
            • Scale - disabled because Grade Type is not 'scale'
            • Grade Point - 600

            The user submits the form and the value stored in the grade column of the module's table is 600.

            Example 2
            User adds a graded activity and selects the following:

            • Site wide grade point maximum is set to 900
            • Grade Type = 'scale'
            • Scale - Scale value
            • Grade Point - automatically disabled because Grade Type is not 'point'

            The user submits the form and the value stored in the grade column of the module's table is a negative integer.

            Example 3
            User adds a graded activity and selects the following:

            • Site wide grade point maximum is set to 900
            • Grade Type = 'No grade'
            • Scale - disabled because Grade Type is not 'scale'
            • Grade Point - disabled because Grade Type is not 'point'

            The user submits the form and the value stored in the grade column of the module's table is 0.

            Example 4
            User adds a graded activity and selects the following:

            • Site wide grade point maximum is set to 900
            • Grade Type = 'Point'
            • Scale - disabled because Grade Type is not 'scale'
            • Grade Point - 20000

            The user submits the form and validation code returns an error highlighting that the grade point entered is higher than the site imposed maximum.

          • In onQuickFormEvent() in the 'updaveValue' event the values passed to the formslib definition can be used to initialize the state of the 3 grade elements.

            Example 1

            • Site wide grade point maximum is set to 900
            • User updates a graded activity that is using a grade point value of 600
            • The value is analyzed
            • The Grade Type dropdown value is set to 'Point'
            • The Scale dropdown is disabled
            • The Grade Point text field is populated with 600

            Example 2

            • Site wide grade point maximum is set to 900
            • User updates a graded activity that is using a grade point value of 0
            • The value is analyzed
            • The Grade Type dropdown value is set to 'No grade'
            • The Scale dropdown is disabled
            • The Grade Point text field is disabled

            Example 3

            • Site wide grade point maximum is set to 900
            • User updates a graded activity that is using a scale value of -1
            • The value is analyzed
            • The Grade Type dropdown value is set to 'Scale'
            • The Scale dropdown value is set to the '-1' entry
            • The Grade Point text field is disabled

            Example 4

            • Site wide grade point maximum is set to 100
            • User updates a graded activity that is using a grade point value of 600
            • The value is analyzed
            • The Grade Type dropdown value is set to 'Point'
            • The Scale dropdown is disabled
            • The Grade Point text field is populated with 600

            The user submits the form and validation code returns an error highlighting that the grade point entered is higher than the site imposed maximum.

          Backup and restore usecases

          Example 1

          • Site wide grade point maximum is set to 1000
          • User creates a graded activity
          • Grade Type = 'Point'
          • Scale - disabled because Grade Type is not 'scale'
          • Grade Point - 1000
          • Saves the form
          • Creates a backup of the course, activities and user data
          • Site wide grade point maximum is set to 100
          • User restores the backuped course with activities and user data
          • User edits the graded activity
          • The grade value is analyzed
          • The Grade Type dropdown value is set to 'Point'
          • The Scale dropdown is disabled
          • The Grade Point text field is populated with 1000

          The user submits the form and validation code returns an error highlighting that the grade point entered is higher than the site imposed maximum.

          Example 2

          • Site wide grade point maximum is set to 1000
          • User creates a graded activity
          • Grade Type = 'Point'
          • Scale - disabled because Grade Type is not 'scale'
          • Grade Point - 30
          • Saves the form
          • Creates a backup of the course, activities and user data
          • Site wide grade point maximum is set to 100
          • User restores the backuped course with activities and user data
          • User edits the graded activity
          • The grade value is analyzed
          • The Grade Type dropdown value is set to 'Point'
          • The Scale dropdown is disabled
          • The Grade Point text field is populated with 30

          The user submits the form and the value stored in the grade column of the module's table is 30.

          Example 3

          • Site wide grade point maximum is set to 1000
          • User creates a graded activity
          • Grade Type = 'scale'
          • Scale - Scale value
          • Grade Point - automatically disabled because Grade Type is not 'point'
          • Saves the form
          • Creates a backup of the course, activities and user data
          • The scale used in the graded activity is removed
          • User restores the backuped course with activities and user data
          • User edits the graded activity
          • The grade value is analyzed
          • The Grade Type dropdown value is set to 'Scale'
          • The Scale defaults to the "standard scale value" of -1
          • The Grade Point text field is disabled

          The user submits the form and the value stored in the grade column of the module's table is -1.

          Modify moodleform_mod.php to use new grading formslib element

          Two areas in this file require modification. The first is in the standard_grade_coursemodule_element(), the second is in the standard_coursemodule_elements.

          The standard_grade_coursemodule_element() first checks to see if the module does not support a rating grade. Then adds the standard grading element to the page.

          The standard_coursemodule_elements() first checks to see if the modules does support a rating grade. Adds the standard grading element to the page, but sets the grade element to disabled if the user selects 'No Ratings' for the Aggregate type

          Both of these areas will have to be modified to call the newly created grade element.

          Inconsistencies in graded module tables

          For lesson module's table, the grade column is defined as a smallint(3) datatype. Other modules, except the quiz, store the grade column as a int(10). The quiz module stores the grade as a decimal(10, 5).

          The problem here being that the lesson module can't accept grade point values greater than 32767. Where as other modules could accept grade point values as high as 2147483647. In the case of the quiz module it could go beyond 2147483647.

          The gradebook table has a grade value defined as a decimal(10, 5).

          Show
          Akin Delamarre added a comment - - edited Proposed solution: Required Code Changes Create a new admin setting Create a new admin settings class that extends the 'admin_setting_configtext' class. The new setting class will override the validate() method to ensure the user enters an integer that is greater than 0 but less than or equal to 10000 A hard coded maximum of 10000 will be enforced by the admin setting The default value will be 100 Modify admin/settings/grades.php by adding the new admin settings field, to allow the user to enter a site wide imposed grade point maximum. Example 1 User goes into Site Administration -> Grades -> General settings User scolls to the 'Grade point maximum' setting User enters a value of 600 User clicks 'Save changes' button Site wide grade point maximum is now 600 Example 2 User goes into Site Administration -> Grades -> General settings User scolls to the 'Grade point maximum' setting User enters a value of 20000 User clicks 'Save changes' button The admin setting code notifies the user of the 10000 point limit Example 3 User goes into Site Administration -> Grades -> General settings User scolls to the 'Grade point maximum' setting User enters a value of 0 User clicks 'Save changes' button The admin setting code notifies the user that the value must be greater 0 but less than 10000 Example 4 User goes into Site Administration -> Grades -> General settings User scolls to the 'Grade point maximum' setting User leaves the field blank User clicks 'Save changes' button Site wide grade point maximum is now 100 (the default value) Example 5 User goes into Site Administration -> Grades -> General settings User scolls to the 'Grade point maximum' setting User enters a non-numeric value or negative number User clicks 'Save changes' button The admin setting code notifies the user that the value must be greater 0 but less than 10000 Create a new formslib element Create a new form definition for displaying the grading elements (defined as an instance of MoodleQuickForm_group). In expotValue() the selected options must be analyized such that one value is returned instead of 3 (one for each grading element). The reason for this is that all graded activities currently use one column, in the module table, to store the grade value. A grade value of '- x' denotes a scale where '-' tells the module a grade scale is to be used and 'x' is the id of the scale selected. Where as a grade value of 'x' denotes a grade point is to be used and 'x' is any positive number. Example 1 User adds a graded activity and selects the following: Site wide grade point maximum is set to 900 Grade Type - 'point' Scale - disabled because Grade Type is not 'scale' Grade Point - 600 The user submits the form and the value stored in the grade column of the module's table is 600. Example 2 User adds a graded activity and selects the following: Site wide grade point maximum is set to 900 Grade Type = 'scale' Scale - Scale value Grade Point - automatically disabled because Grade Type is not 'point' The user submits the form and the value stored in the grade column of the module's table is a negative integer. Example 3 User adds a graded activity and selects the following: Site wide grade point maximum is set to 900 Grade Type = 'No grade' Scale - disabled because Grade Type is not 'scale' Grade Point - disabled because Grade Type is not 'point' The user submits the form and the value stored in the grade column of the module's table is 0. Example 4 User adds a graded activity and selects the following: Site wide grade point maximum is set to 900 Grade Type = 'Point' Scale - disabled because Grade Type is not 'scale' Grade Point - 20000 The user submits the form and validation code returns an error highlighting that the grade point entered is higher than the site imposed maximum. In onQuickFormEvent() in the 'updaveValue' event the values passed to the formslib definition can be used to initialize the state of the 3 grade elements. Example 1 Site wide grade point maximum is set to 900 User updates a graded activity that is using a grade point value of 600 The value is analyzed The Grade Type dropdown value is set to 'Point' The Scale dropdown is disabled The Grade Point text field is populated with 600 Example 2 Site wide grade point maximum is set to 900 User updates a graded activity that is using a grade point value of 0 The value is analyzed The Grade Type dropdown value is set to 'No grade' The Scale dropdown is disabled The Grade Point text field is disabled Example 3 Site wide grade point maximum is set to 900 User updates a graded activity that is using a scale value of -1 The value is analyzed The Grade Type dropdown value is set to 'Scale' The Scale dropdown value is set to the '-1' entry The Grade Point text field is disabled Example 4 Site wide grade point maximum is set to 100 User updates a graded activity that is using a grade point value of 600 The value is analyzed The Grade Type dropdown value is set to 'Point' The Scale dropdown is disabled The Grade Point text field is populated with 600 The user submits the form and validation code returns an error highlighting that the grade point entered is higher than the site imposed maximum. Backup and restore usecases Example 1 Site wide grade point maximum is set to 1000 User creates a graded activity Grade Type = 'Point' Scale - disabled because Grade Type is not 'scale' Grade Point - 1000 Saves the form Creates a backup of the course, activities and user data Site wide grade point maximum is set to 100 User restores the backuped course with activities and user data User edits the graded activity The grade value is analyzed The Grade Type dropdown value is set to 'Point' The Scale dropdown is disabled The Grade Point text field is populated with 1000 The user submits the form and validation code returns an error highlighting that the grade point entered is higher than the site imposed maximum. Example 2 Site wide grade point maximum is set to 1000 User creates a graded activity Grade Type = 'Point' Scale - disabled because Grade Type is not 'scale' Grade Point - 30 Saves the form Creates a backup of the course, activities and user data Site wide grade point maximum is set to 100 User restores the backuped course with activities and user data User edits the graded activity The grade value is analyzed The Grade Type dropdown value is set to 'Point' The Scale dropdown is disabled The Grade Point text field is populated with 30 The user submits the form and the value stored in the grade column of the module's table is 30. Example 3 Site wide grade point maximum is set to 1000 User creates a graded activity Grade Type = 'scale' Scale - Scale value Grade Point - automatically disabled because Grade Type is not 'point' Saves the form Creates a backup of the course, activities and user data The scale used in the graded activity is removed User restores the backuped course with activities and user data User edits the graded activity The grade value is analyzed The Grade Type dropdown value is set to 'Scale' The Scale defaults to the "standard scale value" of -1 The Grade Point text field is disabled The user submits the form and the value stored in the grade column of the module's table is -1. Modify moodleform_mod.php to use new grading formslib element Two areas in this file require modification. The first is in the standard_grade_coursemodule_element(), the second is in the standard_coursemodule_elements. The standard_grade_coursemodule_element() first checks to see if the module does not support a rating grade. Then adds the standard grading element to the page. The standard_coursemodule_elements() first checks to see if the modules does support a rating grade. Adds the standard grading element to the page, but sets the grade element to disabled if the user selects 'No Ratings' for the Aggregate type Both of these areas will have to be modified to call the newly created grade element. Inconsistencies in graded module tables For lesson module's table, the grade column is defined as a smallint(3) datatype. Other modules, except the quiz, store the grade column as a int(10). The quiz module stores the grade as a decimal(10, 5). The problem here being that the lesson module can't accept grade point values greater than 32767. Where as other modules could accept grade point values as high as 2147483647. In the case of the quiz module it could go beyond 2147483647. The gradebook table has a grade value defined as a decimal(10, 5).
          Hide
          Akin Delamarre added a comment -

          Attaching UI mockup of new grader element group of elements. (grader_formslib_group.pdf)

          Show
          Akin Delamarre added a comment - Attaching UI mockup of new grader element group of elements. (grader_formslib_group.pdf)
          Hide
          Robert Puffer added a comment -

          I'm making the assumption that grade column formats will change in order to enable decimal grade values. If you skip that point you've missed a big part of the mess so not sure why lesson would not be able to change also.

          Show
          Robert Puffer added a comment - I'm making the assumption that grade column formats will change in order to enable decimal grade values. If you skip that point you've missed a big part of the mess so not sure why lesson would not be able to change also.
          Hide
          Justin Filip added a comment -

          Hi Robert Puffer, I think when you're asking for grades to be floating point numbers you are specifically referring to the value stored on a per-activity basis when grading student submissions? You're not asking for the maximum grade to be a non-integer are you?

          Show
          Justin Filip added a comment - Hi Robert Puffer , I think when you're asking for grades to be floating point numbers you are specifically referring to the value stored on a per-activity basis when grading student submissions? You're not asking for the maximum grade to be a non-integer are you?
          Hide
          Robert Puffer added a comment -

          No, the max grade can be integer. I just don't want it to get lost that a very big part of this problem is associated with instructors being unable to award marks for an assignment in fractions of a point.

          Show
          Robert Puffer added a comment - No, the max grade can be integer. I just don't want it to get lost that a very big part of this problem is associated with instructors being unable to award marks for an assignment in fractions of a point.
          Hide
          Justin Filip added a comment -

          Robert, I think that would need to be a separate issue as they would be implemented separately and have different test cases associated with each of them. We should create a new issue for that that work and link it to this one.

          Show
          Justin Filip added a comment - Robert, I think that would need to be a separate issue as they would be implemented separately and have different test cases associated with each of them. We should create a new issue for that that work and link it to this one.
          Hide
          Robert Puffer added a comment -

          That's fine but this issue was created over three years ago and we're just coming to that conclusion??
          Please don't lose sight of the fact that the job will be less than half done if all you do is increase maximum point value. That's easily done and has been done for the past eight years by countless schools.

          Show
          Robert Puffer added a comment - That's fine but this issue was created over three years ago and we're just coming to that conclusion?? Please don't lose sight of the fact that the job will be less than half done if all you do is increase maximum point value. That's easily done and has been done for the past eight years by countless schools.
          Hide
          Mike Churchward added a comment -

          Yes. There are hacks to increase the maximum points. But this will make the code and functionality consistent, and get into core. At this point, the need we have is for the maximum points issue. That will be the one we focus on. Maybe we'll just clone a new issue from this so that it can deal with the points specifically and be able to be closed separately.

          Show
          Mike Churchward added a comment - Yes. There are hacks to increase the maximum points. But this will make the code and functionality consistent, and get into core. At this point, the need we have is for the maximum points issue. That will be the one we focus on. Maybe we'll just clone a new issue from this so that it can deal with the points specifically and be able to be closed separately.
          Hide
          Robert Puffer added a comment -

          Forgive my skepticism but its hard to believe the really important issue here will possibly wait another three years or more. Surely nobody actually TEACHING with Moodle is making the decision or this would be a layup.

          Show
          Robert Puffer added a comment - Forgive my skepticism but its hard to believe the really important issue here will possibly wait another three years or more. Surely nobody actually TEACHING with Moodle is making the decision or this would be a layup.
          Hide
          Mike Churchward added a comment -

          Hi Robert. Not a problem. I accept your skepticism. But I can only focus our efforts on the areas that have been identified as high priority for us. I can't speak for our client who wants this, but I believe that they consider what they do "teaching with Moodle".

          Cheers.

          Show
          Mike Churchward added a comment - Hi Robert. Not a problem. I accept your skepticism. But I can only focus our efforts on the areas that have been identified as high priority for us. I can't speak for our client who wants this, but I believe that they consider what they do "teaching with Moodle". Cheers.
          Hide
          Robert Puffer added a comment -

          I have to apologize having just been caught off-guard by the fact that decimal input for grades has already been implemented in Moodle assign as of 2.3. When you look at the grade field for mdl_assign you see bigint(10). Digging deeper I find a table, mdl_assign_grade that also contains a grade field that is decimal(10,5). That's great news (for anybody who doesn't have to try to figure out Moodle's assignment 2.2 DB schema and rationalize it).

          Show
          Robert Puffer added a comment - I have to apologize having just been caught off-guard by the fact that decimal input for grades has already been implemented in Moodle assign as of 2.3. When you look at the grade field for mdl_assign you see bigint(10). Digging deeper I find a table, mdl_assign_grade that also contains a grade field that is decimal(10,5). That's great news (for anybody who doesn't have to try to figure out Moodle's assignment 2.2 DB schema and rationalize it).
          Hide
          Akin Delamarre added a comment -

          Peer review

          [Y] Syntax
          [-] Output
          [Y] Whitespace
          [Y] Language
          [-] Databases
          [Y] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check
          

          No issues found.

          Show
          Akin Delamarre added a comment - Peer review [Y] Syntax [-] Output [Y] Whitespace [Y] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check No issues found.
          Hide
          Akin Delamarre added a comment -

          @Damyon Wiese

          Hello Damyon

          I've completed a peer review of the pull requests.

          I don't have permissions to send the task up for integration testing. Can you please move this process into the next phase of the workflow?

          Thanks,

          Show
          Akin Delamarre added a comment - @Damyon Wiese Hello Damyon I've completed a peer review of the pull requests. I don't have permissions to send the task up for integration testing. Can you please move this process into the next phase of the workflow? Thanks,
          Hide
          Andrew Davis added a comment -

          Hi. The error strings like 'gradeconfigerrbadscale' shouldn't have an abbreviation in them. Just change them to 'gradeconfigerrorbadscale' for example.

          In lib/adminlib.php please revisit these parameter descriptions.

          + /**
          + * Config gradepointmax constructor
          + *
          + * @param string $name unique ascii name, either 'mysetting' for settings that in config, or 'myplugin/mysetting' for ones in config_plugins.
          + * @param string $visiblename localised
          + * @param string $description long localised info
          + * @param string $defaultsetting
          + * @param mixed $paramtype int means PARAM_XXX type, string is a allowed format in regex
          + * @param int $size default field size
          + */

          in lib/adminlib.php

          if ($data === '')

          would

          if (empty($data))

          be better? That would catch nulls as well as zero length strings.

          Still in lib/adminlib.php in validate()

          ((string)(int)$data === (string)$data

          Is the purpose of that just to check that the number is an integer? Is there a reason you haven't used is_int() (http://php.net/manual/en/function.is-int.php)

          Similarly this, $isintlike = ((string)(int)$val === $val) ? true : false;

          I'm not saying if its right or wrong (because I'm not sure) but what was the reasoning for adding a new
          form element 'gradeconfig' instead of modifying 'modgrade'?

          Show
          Andrew Davis added a comment - Hi. The error strings like 'gradeconfigerrbadscale' shouldn't have an abbreviation in them. Just change them to 'gradeconfigerrorbadscale' for example. In lib/adminlib.php please revisit these parameter descriptions. + /** + * Config gradepointmax constructor + * + * @param string $name unique ascii name, either 'mysetting' for settings that in config, or 'myplugin/mysetting' for ones in config_plugins. + * @param string $visiblename localised + * @param string $description long localised info + * @param string $defaultsetting + * @param mixed $paramtype int means PARAM_XXX type, string is a allowed format in regex + * @param int $size default field size + */ in lib/adminlib.php if ($data === '') would if (empty($data)) be better? That would catch nulls as well as zero length strings. Still in lib/adminlib.php in validate() ((string)(int)$data === (string)$data Is the purpose of that just to check that the number is an integer? Is there a reason you haven't used is_int() ( http://php.net/manual/en/function.is-int.php ) Similarly this, $isintlike = ((string)(int)$val === $val) ? true : false; I'm not saying if its right or wrong (because I'm not sure) but what was the reasoning for adding a new form element 'gradeconfig' instead of modifying 'modgrade'?
          Hide
          James McQuillan added a comment -

          Hi Andrew,

          • I've updated the language strings and the parameter descriptions.
          • The idea behind using "data === ''" rather than empty() is to show a validation error when the user enters "0" - indicating that the entry must be between 1 and 10000. Changing this to used empty() causes the default value to be used when "0" is entered.
          • For "((string)(int)$data === (string)$data", the purpose here is to validate a string that contains an integer. $data is going to be a string, so is_int() won't work here, and double-casting and strict-comparing ensures that the string $data contains only a valid integer. (For example, "123abc" would fail this test). This is the same idea for "$isintlike = ((string)(int)$val === $val) ? true : false;".
          • Main reason for not updating modgrade is that it is used in at least one other place not covered by this issue (/mod/workshop/form/accumulative/edit_form.php), and didn't want to break anything else (plugins, etc) that might be using it.
          Show
          James McQuillan added a comment - Hi Andrew, I've updated the language strings and the parameter descriptions. The idea behind using "data === ''" rather than empty() is to show a validation error when the user enters "0" - indicating that the entry must be between 1 and 10000. Changing this to used empty() causes the default value to be used when "0" is entered. For "((string)(int)$data === (string)$data", the purpose here is to validate a string that contains an integer. $data is going to be a string, so is_int() won't work here, and double-casting and strict-comparing ensures that the string $data contains only a valid integer. (For example, "123abc" would fail this test). This is the same idea for "$isintlike = ((string)(int)$val === $val) ? true : false;". Main reason for not updating modgrade is that it is used in at least one other place not covered by this issue (/mod/workshop/form/accumulative/edit_form.php), and didn't want to break anything else (plugins, etc) that might be using it.
          Hide
          James McQuillan added a comment -

          Hi Andrew,

          Just wondering what the status is on this, did you get a chance to look at the changes? Please let me know if there's anything you need from me.

          Thanks!

          Show
          James McQuillan added a comment - Hi Andrew, Just wondering what the status is on this, did you get a chance to look at the changes? Please let me know if there's anything you need from me. Thanks!
          Hide
          Justin Filip added a comment -

          Andrew Davis you did the review on this one originally and we have updated based on your feedback, are you available to look at the latest work here? Thanks.

          Show
          Justin Filip added a comment - Andrew Davis you did the review on this one originally and we have updated based on your feedback, are you available to look at the latest work here? Thanks.
          Hide
          CiBoT added a comment -

          Results for MDL-22999

          Show
          CiBoT added a comment - Results for MDL-22999 Remote repository: https://github.com/jamesmcq/moodle Remote branch MDL-22999 -m24 to be integrated into upstream MOODLE_24_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/335 Warning: The MDL-22999 -m24 branch at https://github.com/jamesmcq/moodle has not been rebased recently. Details: http://integration.moodle.org/job/Precheck%20remote%20branch/335/artifact/work/smurf.html Remote branch MDL-22999 -m25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/336 Error: The MDL-22999 -m25 branch at https://github.com/jamesmcq/moodle is very old. Please rebase against current MOODLE_25_STABLE. Remote branch MDL-22999 -m26 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/337 Error: The MDL-22999 -m26 branch at https://github.com/jamesmcq/moodle is very old. Please rebase against current master.
          Hide
          CiBoT added a comment -

          Results for MDL-22999

          Show
          CiBoT added a comment - Results for MDL-22999 Remote repository: https://github.com/jamesmcq/moodle Remote branch MDL-22999 -m24 to be integrated into upstream MOODLE_24_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/384 Warning: The MDL-22999 -m24 branch at https://github.com/jamesmcq/moodle has not been rebased recently. Details: http://integration.moodle.org/job/Precheck%20remote%20branch/384/artifact/work/smurf.html Remote branch MDL-22999 -m25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/385 Warning: The MDL-22999 -m25 branch at https://github.com/jamesmcq/moodle has not been rebased recently. Details: http://integration.moodle.org/job/Precheck%20remote%20branch/385/artifact/work/smurf.html Remote branch MDL-22999 -m26 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/386 Warning: The MDL-22999 -m26 branch at https://github.com/jamesmcq/moodle has not been rebased recently. Error: The MDL-22999 -m26 branch at https://github.com/jamesmcq/moodle does not apply clean to master
          Hide
          CiBoT added a comment -

          Results for MDL-22999

          Show
          CiBoT added a comment - Results for MDL-22999 Remote repository: https://github.com/jamesmcq/moodle Remote branch MDL-22999 -m24 to be integrated into upstream MOODLE_24_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/495 Warning: The MDL-22999 -m24 branch at https://github.com/jamesmcq/moodle has not been rebased recently. Details: http://integration.moodle.org/job/Precheck%20remote%20branch/495/artifact/work/smurf.html Remote branch MDL-22999 -m25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/496 Warning: The MDL-22999 -m25 branch at https://github.com/jamesmcq/moodle has not been rebased recently. Details: http://integration.moodle.org/job/Precheck%20remote%20branch/496/artifact/work/smurf.html Remote branch MDL-22999 -m26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/497 Warning: The MDL-22999 -m26 branch at https://github.com/jamesmcq/moodle has not been rebased recently. Details: http://integration.moodle.org/job/Precheck%20remote%20branch/497/artifact/work/smurf.html Remote branch MDL-22999 -m27 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/498 Warning: The MDL-22999 -m27 branch at https://github.com/jamesmcq/moodle has not been rebased recently. Details: http://integration.moodle.org/job/Precheck%20remote%20branch/498/artifact/work/smurf.html
          Hide
          Martín Langhoff added a comment -

          We think CiBoT is complaining 'has not rebased recently' is just reading the commit date. There aren't any interesting patches to rebase on top off...

          Show
          Martín Langhoff added a comment - We think CiBoT is complaining 'has not rebased recently' is just reading the commit date. There aren't any interesting patches to rebase on top off...
          Show
          CiBoT added a comment - Results for MDL-22999 Remote repository: https://github.com/jamesmcq/moodle Remote branch MDL-22999 -m24 to be integrated into upstream MOODLE_24_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/790 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/790/artifact/work/smurf.html Remote branch MDL-22999 -m25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/791 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/791/artifact/work/smurf.html Remote branch MDL-22999 -m26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/792 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/792/artifact/work/smurf.html Remote branch MDL-22999 -m27 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/793 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/793/artifact/work/smurf.html
          Hide
          Tim Hunt added a comment -

          Some review comments:

          0. See CiBoT comments above.

          1. I see no need for admin_setting_special_gradepointmax. Just use a admin_setting_configtext with PARAM_INT.

          2. I see no need for MoodleQuickForm_gradeconfig. Why not just make a minimal change to MoodleQuickForm_modgrade?

          3. If, however, the gradeconfig thing bringes some genuine advantage, you need to explain that in the comments in the code, and in the commit comment. However, that sort of UI change can only be in master.

          That way, you will have a patch that is 10% of the size, which will be much easier to get reviewed and integrated.

          Show
          Tim Hunt added a comment - Some review comments: 0. See CiBoT comments above. 1. I see no need for admin_setting_special_gradepointmax. Just use a admin_setting_configtext with PARAM_INT. 2. I see no need for MoodleQuickForm_gradeconfig. Why not just make a minimal change to MoodleQuickForm_modgrade? 3. If, however, the gradeconfig thing bringes some genuine advantage, you need to explain that in the comments in the code, and in the commit comment. However, that sort of UI change can only be in master. That way, you will have a patch that is 10% of the size, which will be much easier to get reviewed and integrated.
          Hide
          Tim Hunt added a comment -

          4. In the tests instructions, add a one-line summary of what each TCx is testing.

          Show
          Tim Hunt added a comment - 4. In the tests instructions, add a one-line summary of what each TCx is testing.
          Hide
          Tim Hunt added a comment -

          Note, that was not a proper peer review. Martin Langhoff was asking for a peer reivewer, and so I just took a quick look and pointed out some stuff that seemed immeditately questionable.

          Show
          Tim Hunt added a comment - Note, that was not a proper peer review. Martin Langhoff was asking for a peer reivewer, and so I just took a quick look and pointed out some stuff that seemed immeditately questionable.
          Hide
          Akin Delamarre added a comment -

          I'm unable to edit the test instructions section, so I'm posting the titles for each of the test instructions here:

          TC1 - Adding a graded activity and setting the maximum grade point

          TC2 - Adding a graded activity and selecting a scale grade

          TC3 - Adding a graded activity and selecting 'no grade'

          TC4 - Adding a graded activity and setting the maximum grade point higher than the site imposed maximum

          TC5 - Updating a graded activity and setting the maximum grade point

          TC6 - Updating a graded activity, whose grade was previously set to 'no grade'

          TC7 - Updating a graded activity, whose grade was previously using a scale

          TC8 - Updating a graded activity and setting a grade point maximum; then changing the site wide grade point maximum to a value lower than the value used by the activity

          TC9 - Adding a graded activity and setting the grade point maximum. Change the site wide grade point maximum to a value lower than the value used by the activity. Perform a backup and restore of the graded activity

          TC10 - Adding a graded activity and setting the grade point maximum. Change the grade point maximum. Perform a backup and restore of the graded activity

          TC11 - Adding a grade activity snd setting a scale grade. Perform a backup of the activity. Removed the sale used by the activity by removing it from the course (or site). Restore the activity

          Show
          Akin Delamarre added a comment - I'm unable to edit the test instructions section, so I'm posting the titles for each of the test instructions here: TC1 - Adding a graded activity and setting the maximum grade point TC2 - Adding a graded activity and selecting a scale grade TC3 - Adding a graded activity and selecting 'no grade' TC4 - Adding a graded activity and setting the maximum grade point higher than the site imposed maximum TC5 - Updating a graded activity and setting the maximum grade point TC6 - Updating a graded activity, whose grade was previously set to 'no grade' TC7 - Updating a graded activity, whose grade was previously using a scale TC8 - Updating a graded activity and setting a grade point maximum; then changing the site wide grade point maximum to a value lower than the value used by the activity TC9 - Adding a graded activity and setting the grade point maximum. Change the site wide grade point maximum to a value lower than the value used by the activity. Perform a backup and restore of the graded activity TC10 - Adding a graded activity and setting the grade point maximum. Change the grade point maximum. Perform a backup and restore of the graded activity TC11 - Adding a grade activity snd setting a scale grade. Perform a backup of the activity. Removed the sale used by the activity by removing it from the course (or site). Restore the activity
          Hide
          Justin Filip added a comment -

          Updated the testing instructions with titles for each test case.

          Show
          Justin Filip added a comment - Updated the testing instructions with titles for each test case.
          Hide
          Marina Glancy added a comment -

          Hello guys,
          thanks for the work. First it is an improvement and it can't go to the stable branches but only to master.
          Second thing that I've noticed is that it does not have any automated test coverage - unittest or behat. And this thought led to another one - have you run existing behat tests to make sure it does not break anything? I have only ran a couple of tests to immediately discover the debugging message. Developer debugging must always be enabled on development machines, why did not you notice it?

          01. debugging() message/s found:
              Help title string does not exist: [gradeconfig, grades]
              line 461 of /lib/outputcomponents.php: call to debugging()
              line 2130 of /lib/outputrenderers.php: call to help_icon->diag_strings()
              line 1893 of /lib/formslib.php: call to core_renderer->help_icon()
              line 790 of /course/moodleform_mod.php: call to MoodleQuickForm->addHelpButton()
              line 172 of /mod/assign/mod_form.php: call to moodleform_mod->standard_grading_coursemodule_elements()
              line 191 of /lib/formslib.php: call to mod_assign_mod_form->definition()
              line 86 of /course/moodleform_mod.php: call to moodleform->moodleform()
              line 256 of /course/modedit.php: call to moodleform_mod->moodleform_mod()
              
              Invalid get_string() identifier: 'gradeconfig' or component 'grades'. Perhaps you are missing $string['gradeconfig'] = ''; in lang/en/grades.php?
              line 293 of /lib/classes/string_manager_standard.php: call to debugging()
              line 6897 of /lib/moodlelib.php: call to core_string_manager_standard->get_string()
              line 2151 of /lib/outputrenderers.php: call to get_string()
              line 101 of /lib/outputrenderers.php: call to core_renderer->render_help_icon()
              line 2136 of /lib/outputrenderers.php: call to renderer_base->render()
              line 1893 of /lib/formslib.php: call to core_renderer->help_icon()
              line 790 of /course/moodleform_mod.php: call to MoodleQuickForm->addHelpButton()
              line 172 of /mod/assign/mod_form.php: call to moodleform_mod->standard_grading_coursemodule_elements()
              line 191 of /lib/formslib.php: call to mod_assign_mod_form->definition()
              line 86 of /course/moodleform_mod.php: call to moodleform->moodleform()
              line 256 of /course/modedit.php: call to moodleform_mod->moodleform_mod()
          

          Obviously even after fixing it, behat tests still fail. Please make sure that all existing tests pass and ideally create ones to check the new functionality.

          Thanks.

          Show
          Marina Glancy added a comment - Hello guys, thanks for the work. First it is an improvement and it can't go to the stable branches but only to master. Second thing that I've noticed is that it does not have any automated test coverage - unittest or behat. And this thought led to another one - have you run existing behat tests to make sure it does not break anything? I have only ran a couple of tests to immediately discover the debugging message. Developer debugging must always be enabled on development machines, why did not you notice it? 01. debugging() message/s found: Help title string does not exist: [gradeconfig, grades] line 461 of /lib/outputcomponents.php: call to debugging() line 2130 of /lib/outputrenderers.php: call to help_icon->diag_strings() line 1893 of /lib/formslib.php: call to core_renderer->help_icon() line 790 of /course/moodleform_mod.php: call to MoodleQuickForm->addHelpButton() line 172 of /mod/assign/mod_form.php: call to moodleform_mod->standard_grading_coursemodule_elements() line 191 of /lib/formslib.php: call to mod_assign_mod_form->definition() line 86 of /course/moodleform_mod.php: call to moodleform->moodleform() line 256 of /course/modedit.php: call to moodleform_mod->moodleform_mod() Invalid get_string() identifier: 'gradeconfig' or component 'grades'. Perhaps you are missing $string['gradeconfig'] = ''; in lang/en/grades.php? line 293 of /lib/classes/string_manager_standard.php: call to debugging() line 6897 of /lib/moodlelib.php: call to core_string_manager_standard->get_string() line 2151 of /lib/outputrenderers.php: call to get_string() line 101 of /lib/outputrenderers.php: call to core_renderer->render_help_icon() line 2136 of /lib/outputrenderers.php: call to renderer_base->render() line 1893 of /lib/formslib.php: call to core_renderer->help_icon() line 790 of /course/moodleform_mod.php: call to MoodleQuickForm->addHelpButton() line 172 of /mod/assign/mod_form.php: call to moodleform_mod->standard_grading_coursemodule_elements() line 191 of /lib/formslib.php: call to mod_assign_mod_form->definition() line 86 of /course/moodleform_mod.php: call to moodleform->moodleform() line 256 of /course/modedit.php: call to moodleform_mod->moodleform_mod() Obviously even after fixing it, behat tests still fail. Please make sure that all existing tests pass and ideally create ones to check the new functionality. Thanks.
          Hide
          Marina Glancy added a comment -

          There need to be default value set, preferably "Points", "100" as it was before. Thanks.

          Show
          Marina Glancy added a comment - There need to be default value set, preferably "Points", "100" as it was before. Thanks.
          Hide
          Justin Filip added a comment -

          Just a heads up: we have the failed Behat tests passing now and are working on getting the default value set in the form element. Hoping that we can get that looked at and into the current integration pass so we have a chance to correct any failures before the 2.7 freeze.

          Show
          Justin Filip added a comment - Just a heads up: we have the failed Behat tests passing now and are working on getting the default value set in the form element. Hoping that we can get that looked at and into the current integration pass so we have a chance to correct any failures before the 2.7 freeze.
          Hide
          CiBoT added a comment -
          Show
          CiBoT added a comment - Results for MDL-22999 Remote repository: https://github.com/jamesmcq/moodle Remote branch MDL-22999 -m27 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2643 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2643/artifact/work/smurf.html
          Hide
          CiBoT added a comment -
          Show
          CiBoT added a comment - Results for MDL-22999 Remote repository: https://github.com/jamesmcq/moodle Remote branch MDL-22999 -m27 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2645 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2645/artifact/work/smurf.html
          Hide
          Justin Filip added a comment -

          This issue now has Behat tests for most of our test cases. The restore actions don't seem to be working in our tests so those aren't included here.

          Show
          Justin Filip added a comment - This issue now has Behat tests for most of our test cases. The restore actions don't seem to be working in our tests so those aren't included here.
          Hide
          CiBoT added a comment -
          Show
          CiBoT added a comment - Results for MDL-22999 Remote repository: https://github.com/jfilip/moodle Remote branch wip- MDL-22999 -m27 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2648 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2648/artifact/work/smurf.html
          Hide
          Justin Filip added a comment -

          Hey, Marina Glancy if you don't have time to continue with the peer review on this one can you maybe set this one back to waiting for peer review (or, better yet, find someone who can)? Right now it looks like you are actively doing the review which is not true. Maybe if one of the non-integrator HQ devs can take it on?

          Show
          Justin Filip added a comment - Hey, Marina Glancy if you don't have time to continue with the peer review on this one can you maybe set this one back to waiting for peer review (or, better yet, find someone who can)? Right now it looks like you are actively doing the review which is not true. Maybe if one of the non-integrator HQ devs can take it on?
          Hide
          Marina Glancy added a comment -

          Hi Justin,
          when I'm on integration roster, Mondays and Tuesdays I'm completely flat out and Wednesdays often as well.
          This is great that you've added behat tests, it really helps.

          At the moment the default value for the gradetype is "None" and this is what breaks existing behat tests. I have made a patch to correctly set the default value:
          https://github.com/marinaglancy/moodle/commit/2c6578c666fe3396c3336d46d1eec3f25b491c95
          With this patch you can revert your changes to all existing behat tests.

          Second concern is that you introduce the new form element 'gradeconfig' and replace the usage of existing 'modgrade' with it. Can I ask why did you decide to do it instead of replacing the element itself? You are not adding new functionality to this form element, you just make it look different. But all 3rd party code that used element 'modgrade' now has to be upgraded to use 'gradeconfig'. Also there is one unreplaced instance in workshop.

          While testing I noticed that ratings do not work very well - there is double disableIf on elements of your new control which does not apply well. Try creating a forum and enabling ratings, when you change the grading type the scale/points inputs do not get disabled any more. Btw, please add testing of ratings to your testing instructions.

          And the last thing - you do not need to check if isset($CFG->gradepointmax) - if you add this setting correctly, it will be set during upgrade/install. Just don't forget to bump the version to trigger upgrade process.

          As a resume:

          • replace the existing modgrade form element instead of introducing new one (plus add short notification about changes in the 'modgrade' element in mod/upgrade.txt)
          • correctly set the default value (see my patch above)
          • revert changes to existing behat tests
          • add testing instruction for ratings (create forum and enable ratings)
          • and testing instruction for workshop (create workshop in "Accumulative grading" and then click "Edit assessment form")
          • check your code for disabling/enabling the elements, I also have a related failure in your behat test
          • bump the version number (since you introduce new setting)

          This looks like a long list but really not much left to do. Great job guys.

          Show
          Marina Glancy added a comment - Hi Justin, when I'm on integration roster, Mondays and Tuesdays I'm completely flat out and Wednesdays often as well. This is great that you've added behat tests, it really helps. At the moment the default value for the gradetype is "None" and this is what breaks existing behat tests. I have made a patch to correctly set the default value: https://github.com/marinaglancy/moodle/commit/2c6578c666fe3396c3336d46d1eec3f25b491c95 With this patch you can revert your changes to all existing behat tests. Second concern is that you introduce the new form element 'gradeconfig' and replace the usage of existing 'modgrade' with it. Can I ask why did you decide to do it instead of replacing the element itself? You are not adding new functionality to this form element, you just make it look different. But all 3rd party code that used element 'modgrade' now has to be upgraded to use 'gradeconfig'. Also there is one unreplaced instance in workshop. While testing I noticed that ratings do not work very well - there is double disableIf on elements of your new control which does not apply well. Try creating a forum and enabling ratings, when you change the grading type the scale/points inputs do not get disabled any more. Btw, please add testing of ratings to your testing instructions. And the last thing - you do not need to check if isset($CFG->gradepointmax) - if you add this setting correctly, it will be set during upgrade/install. Just don't forget to bump the version to trigger upgrade process. As a resume: replace the existing modgrade form element instead of introducing new one (plus add short notification about changes in the 'modgrade' element in mod/upgrade.txt) correctly set the default value (see my patch above) revert changes to existing behat tests add testing instruction for ratings (create forum and enable ratings) and testing instruction for workshop (create workshop in "Accumulative grading" and then click "Edit assessment form") check your code for disabling/enabling the elements, I also have a related failure in your behat test bump the version number (since you introduce new setting) This looks like a long list but really not much left to do. Great job guys.
          Hide
          CiBoT added a comment -
          Show
          CiBoT added a comment - Results for MDL-22999 Remote repository: https://github.com/jamesmcq/moodle Remote branch wip- MDL-22999 -m27 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2697 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2697/artifact/work/smurf.html
          Hide
          James McQuillan added a comment -

          Hi Marina,

          Thanks for looking this over!

          I have made the requested changes and updated the branch/diff information in the issue.

          • Replaced the existing modgrade with the old gradeconfig element + added note in mod/upgrade.txt.
          • Added your patch for setting the default value into the original commit.
          • Reverted changed to existing behat tests + modified code so our new behat tests are all passing.
          • Added additional test cases for the ratings (via forum), and the workshop.
          • The disable/enable code has been modified to work in the forum case.
          • Version has been bumped up.

          While testing the workshop, I came across what I believe to be a bug in 2.6.2 and master, and created MDL-44942 for it. The same behavior happens with the new modgrade element and this branch, so it may not be possible to get the workshop test cases passing until MDL-44942 is dealt with.

          Show
          James McQuillan added a comment - Hi Marina, Thanks for looking this over! I have made the requested changes and updated the branch/diff information in the issue. Replaced the existing modgrade with the old gradeconfig element + added note in mod/upgrade.txt. Added your patch for setting the default value into the original commit. Reverted changed to existing behat tests + modified code so our new behat tests are all passing. Added additional test cases for the ratings (via forum), and the workshop. The disable/enable code has been modified to work in the forum case. Version has been bumped up. While testing the workshop, I came across what I believe to be a bug in 2.6.2 and master, and created MDL-44942 for it. The same behavior happens with the new modgrade element and this branch, so it may not be possible to get the workshop test cases passing until MDL-44942 is dealt with.
          Hide
          CiBoT added a comment -
          Show
          CiBoT added a comment - Results for MDL-22999 Remote repository: https://github.com/jamesmcq/moodle Remote branch wip- MDL-22999 -m27 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/2698 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/2698/artifact/work/smurf.html
          Hide
          Marina Glancy added a comment -

          Thanks James, this looks great. I will run some tests now and then push it for integration.
          Maybe now when you have a good and throughout behat test you can remove most of your manual testing instructions? We can just ask the tester to go through all forms in Moodle that have this element and make sure they work as expected. This will be just three test cases - adding/editing graded module (i.e. assignment), adding/editing module with ratings (i.e. forum), adding/editing the workshop aspects.

          Show
          Marina Glancy added a comment - Thanks James, this looks great. I will run some tests now and then push it for integration. Maybe now when you have a good and throughout behat test you can remove most of your manual testing instructions? We can just ask the tester to go through all forms in Moodle that have this element and make sure they work as expected. This will be just three test cases - adding/editing graded module (i.e. assignment), adding/editing module with ratings (i.e. forum), adding/editing the workshop aspects.
          Hide
          Marina Glancy added a comment -

          Hm, there is still one thing that I'm not sure about. I thought that $CFG->gradepointmax is the default value for the number of points but apparently it is the maximum. And we use it as default as well. This means that whatever settings are we can never set the number of points more than default.

          What do you think if we set default for grades and ratings as min($CFG->gradepointmax, 100)
          and for the workshop aspects min($CFG->gradepointmax, 10)

          In this case mod_assign and other graded modules can overwrite the default in their mod_form.php if they want. assign plugin has the whole page of default settings, they can just add one more (as a separate issue).

          Show
          Marina Glancy added a comment - Hm, there is still one thing that I'm not sure about. I thought that $CFG->gradepointmax is the default value for the number of points but apparently it is the maximum. And we use it as default as well. This means that whatever settings are we can never set the number of points more than default. What do you think if we set default for grades and ratings as min($CFG->gradepointmax, 100) and for the workshop aspects min($CFG->gradepointmax, 10) In this case mod_assign and other graded modules can overwrite the default in their mod_form.php if they want. assign plugin has the whole page of default settings, they can just add one more (as a separate issue).
          Hide
          Marina Glancy added a comment -

          Just re-read the issue description - it was initially asked to be able to have float as the maximum grade but you made the "points" field integer-only. Looking at the DB structure I can see that the field 'grade' is located in module-specific table. It would be nice to add potential possibility for a module to specify that it will be ok with float value. Worth to consider but maybe too late to implement now.

          Show
          Marina Glancy added a comment - Just re-read the issue description - it was initially asked to be able to have float as the maximum grade but you made the "points" field integer-only. Looking at the DB structure I can see that the field 'grade' is located in module-specific table. It would be nice to add potential possibility for a module to specify that it will be ok with float value. Worth to consider but maybe too late to implement now.
          Hide
          Marina Glancy added a comment -

          Tests are passing, the new form element looks good.

          I'm submitting for integration (simply because if I don't do it now I will go away on the weekend and you won't be able to do it).

          There are two minor things that I'd like you to address:
          1. Consider separating maximum and default value for points. In fact, as a second thought, why do we even need a side-wide maximum limit? I don't see teachers abusing this and setting it to billions.
          2. Please rename string "Maximum grade" into "Maximum points", or otherwise it looks awkward in "Ratings" section

          Thanks for your work again!

          Show
          Marina Glancy added a comment - Tests are passing, the new form element looks good. I'm submitting for integration (simply because if I don't do it now I will go away on the weekend and you won't be able to do it). There are two minor things that I'd like you to address: 1. Consider separating maximum and default value for points. In fact, as a second thought, why do we even need a side-wide maximum limit? I don't see teachers abusing this and setting it to billions. 2. Please rename string "Maximum grade" into "Maximum points", or otherwise it looks awkward in "Ratings" section Thanks for your work again!
          Hide
          Martín Langhoff added a comment -

          Marina Glancy, big thanks to YOU! The RL team has been working on this for a while, and we were stressed for a moment that we would miss the v2.7 boat. Thanks for your patience.

          We are standing by to handle any issues arising from the integration.

          Show
          Martín Langhoff added a comment - Marina Glancy , big thanks to YOU! The RL team has been working on this for a while, and we were stressed for a moment that we would miss the v2.7 boat. Thanks for your patience. We are standing by to handle any issues arising from the integration.
          Hide
          CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          Damyon Wiese added a comment -

          Thanks for the patch - and in particular all the behat tests. There are some things that need further fixing before this is integrated:

          1. Change of copyright in lib/form/modgrade.php - this is forbidden always.
          2. This is failing behat tests for mod_workshop (fails are listed below). This issue will not be integrated if there are any behat tests failing anywhere - so please run the whole suite (I know it takes a long time)
          3. I agree with Marina that there should be a separate setting for the default value as well as the maximum value
          4. I agree with Marina that "Maximum grade" should be "Maximum points"

          That's all - I'll leave this issue in review until wednesday morning - I hope you can address these points before then.

          Thanks!

          ...........................F------------------------------------------ 70
          -------------------------...........................F----------------- 140
          --------------------------------------------------
          
          (::) failed steps (::)
          
          01. debugging() message/s found:
              Argument $record must be an instance of stdClass.
              line 692 of /lib/classes/event/base.php: call to debugging()
              line 235 of /mod/workshop/submission.php: call to core\event\base->add_record_snapshot()
              In step `Given I press "Save changes"'.          # behat_forms::press_button()
              From scenario background.                        # /home/damyonw/Documents/Moodle/integration/master/moodle/mod/workshop/tests/behat/workshop_assessment.feature:7
              Of feature `Workshop submission and assessment'. # /home/damyonw/Documents/Moodle/integration/master/moodle/mod/workshop/tests/behat/workshop_assessment.feature
          
          02. debugging() message/s found:
              Argument $record must be an instance of stdClass.
              line 692 of /lib/classes/event/base.php: call to debugging()
              line 235 of /mod/workshop/submission.php: call to core\event\base->add_record_snapshot()
              In step `Given I press "Save changes"'.          # behat_forms::press_button()
              From scenario background.                        # /home/damyonw/Documents/Moodle/integration/master/moodle/mod/workshop/tests/behat/workshop_assessment.feature:7
              Of feature `Workshop submission and assessment'. # /home/damyonw/Documents/Moodle/integration/master/moodle/mod/workshop/tests/behat/workshop_assessment.feature
          
          2 scenarios (2 failed)
          190 steps (54 passed, 134 skipped, 2 failed)
          
          Show
          Damyon Wiese added a comment - Thanks for the patch - and in particular all the behat tests. There are some things that need further fixing before this is integrated: 1. Change of copyright in lib/form/modgrade.php - this is forbidden always. 2. This is failing behat tests for mod_workshop (fails are listed below). This issue will not be integrated if there are any behat tests failing anywhere - so please run the whole suite (I know it takes a long time) 3. I agree with Marina that there should be a separate setting for the default value as well as the maximum value 4. I agree with Marina that "Maximum grade" should be "Maximum points" That's all - I'll leave this issue in review until wednesday morning - I hope you can address these points before then. Thanks! ...........................F------------------------------------------ 70 -------------------------...........................F----------------- 140 -------------------------------------------------- (::) failed steps (::) 01. debugging() message/s found: Argument $record must be an instance of stdClass. line 692 of /lib/classes/event/base.php: call to debugging() line 235 of /mod/workshop/submission.php: call to core\event\base->add_record_snapshot() In step `Given I press "Save changes" '. # behat_forms::press_button() From scenario background. # /home/damyonw/Documents/Moodle/integration/master/moodle/mod/workshop/tests/behat/workshop_assessment.feature:7 Of feature `Workshop submission and assessment'. # /home/damyonw/Documents/Moodle/integration/master/moodle/mod/workshop/tests/behat/workshop_assessment.feature 02. debugging() message/s found: Argument $record must be an instance of stdClass. line 692 of /lib/classes/event/base.php: call to debugging() line 235 of /mod/workshop/submission.php: call to core\event\base->add_record_snapshot() In step `Given I press "Save changes" '. # behat_forms::press_button() From scenario background. # /home/damyonw/Documents/Moodle/integration/master/moodle/mod/workshop/tests/behat/workshop_assessment.feature:7 Of feature `Workshop submission and assessment'. # /home/damyonw/Documents/Moodle/integration/master/moodle/mod/workshop/tests/behat/workshop_assessment.feature 2 scenarios (2 failed) 190 steps (54 passed, 134 skipped, 2 failed)
          Hide
          Marina Glancy added a comment - - edited

          Damyon, the behat test should be fixed by MDL-44945

          Show
          Marina Glancy added a comment - - edited Damyon, the behat test should be fixed by MDL-44945
          Hide
          James McQuillan added a comment -

          Thanks Damyon and Marina - changes have been made.

          Show
          James McQuillan added a comment - Thanks Damyon and Marina - changes have been made.
          Hide
          Damyon Wiese added a comment -

          Thanks for the update James,

          This has been integrated to master now - the updates all look good and behat is passing (tested @core_grade and @mod_workshop).

          Off to testing...

          Show
          Damyon Wiese added a comment - Thanks for the update James, This has been integrated to master now - the updates all look good and behat is passing (tested @core_grade and @mod_workshop). Off to testing...
          Hide
          Jason Fowler added a comment -

          Thanks James, tests all pass nicely

          Show
          Jason Fowler added a comment - Thanks James, tests all pass nicely
          Hide
          Justin Filip added a comment -

          Thank you Andrew Davis, Tim Hunt, Marina Glancy, Damyon Wiese, and Jason Fowler for the feedback and work on this one!

          Show
          Justin Filip added a comment - Thank you Andrew Davis , Tim Hunt , Marina Glancy , Damyon Wiese , and Jason Fowler for the feedback and work on this one!
          Hide
          Dan Poltawski added a comment -

          Thanks for your efforts, this change is now part of Moodle!

          Show
          Dan Poltawski added a comment - Thanks for your efforts, this change is now part of Moodle!

            People

            • Votes:
              23 Vote for this issue
              Watchers:
              18 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: