Moodle
  1. Moodle
  2. MDL-40230

Integrate Outcomes stage 2 into core

    Details

    • Testing Instructions:
      Hide

      Make sure you have "New Outcomes" enabled in the site settings.
      You'll need a second site to test importing. The second site is only required for a single test (#4).

      AS ADMIN
      Go to Settings > Site Administration > Grades > Outcomes
      1) Check that you can add a new outcome set with at least one outcome within it.
      2) Check that you can edit an outcome.
      3) Export your outcome set. You should get an xml file.
      4) Import the xml file. The format is "moodle general format". Unfortunately you'll need to import it into an entirely different site (MDL-42088). Check that the imported outcomes are intact.

      5) Go to course settings and select the outcome set for use in the course.
      6) Go to the outcomes page in site settings. The number of "mapped courses" should be at least 1. Clicking on the number should tell you what courses are mapped.

      AS TEACHER
      7) Go to course administration > outcomes and click on unmapped content items and quiz questions. All activities should be unmapped.
      8) Create lots of different kinds of activities. Within each activities' settings, select one or more outcomes to be associated with that activity.
      NOTE: Include one assignment that uses a new rubric. For the rubric item do not map the outcome in the activity settings. Instead do it when creating the rubric itself (a link will appear when you move focus to the rubric item name field) thus associating the outcome with a specific rubric item.
      9) Similarly, add a few resources and associate outcomes with them.
      10) Check that the unmapped content items page doesn't show the activites and resources you just added outcomes to.

      11) As a student complete the activities.
      12) As a teacher grade their assignments or do whatever you have to do within the activities to get them a grade (assuming the activity is gradeable).

      13) Go to course admin > outcomes and open the Activity and performance page.
      14) Check you see the average grade for activities associated with a given outcome and that you can access the grade scales used by those activities as well as the names of the relevant activities.
      15) Sorting the table of outcomes should work as you would expect.

      16) Go to course admin > outcomes and open the Coverage page.
      17) You should be able to access the names of the resources, activities and questions associated with a given outcome.
      18) Sorting the table of outcomes should work as you would expect.
      19) Go to the course question bank, create a new question (but dont put it in a quiz) and associate an outcome with it.
      20) On the outcome Coverage page there should be a star next to the number of questions associated with that outcome.
      21) Put the question in a quiz. Now the Coverage page should not have a star next to the number.

      22) Go to course admin > outcomes and open the Completion Marking page.
      23) Select the student you graded and click Filter.
      24) Check that the student's grade information appears. Clicking on the grades and "view items" links should tell you where the grade has come from.
      25) Mark an outcome complete and click save changes.
      26) Move to another user using Filter and check that the previous user's grade, completion status etc is not displayed.
      27) Sorting the table of outcomes should work as you would expect.

      Backup and Restore
      28) Backup your outcome test course.
      29) Restore it into a new course.
      30) Verify that the outcome sets in the course settings are correct.
      31) Verify that the outcome associations in the individual activities are correct.

      Show
      Make sure you have "New Outcomes" enabled in the site settings. You'll need a second site to test importing. The second site is only required for a single test (#4). AS ADMIN Go to Settings > Site Administration > Grades > Outcomes 1) Check that you can add a new outcome set with at least one outcome within it. 2) Check that you can edit an outcome. 3) Export your outcome set. You should get an xml file. 4) Import the xml file. The format is "moodle general format". Unfortunately you'll need to import it into an entirely different site ( MDL-42088 ). Check that the imported outcomes are intact. 5) Go to course settings and select the outcome set for use in the course. 6) Go to the outcomes page in site settings. The number of "mapped courses" should be at least 1. Clicking on the number should tell you what courses are mapped. AS TEACHER 7) Go to course administration > outcomes and click on unmapped content items and quiz questions. All activities should be unmapped. 8) Create lots of different kinds of activities. Within each activities' settings, select one or more outcomes to be associated with that activity. NOTE: Include one assignment that uses a new rubric. For the rubric item do not map the outcome in the activity settings. Instead do it when creating the rubric itself (a link will appear when you move focus to the rubric item name field) thus associating the outcome with a specific rubric item. 9) Similarly, add a few resources and associate outcomes with them. 10) Check that the unmapped content items page doesn't show the activites and resources you just added outcomes to. 11) As a student complete the activities. 12) As a teacher grade their assignments or do whatever you have to do within the activities to get them a grade (assuming the activity is gradeable). 13) Go to course admin > outcomes and open the Activity and performance page. 14) Check you see the average grade for activities associated with a given outcome and that you can access the grade scales used by those activities as well as the names of the relevant activities. 15) Sorting the table of outcomes should work as you would expect. 16) Go to course admin > outcomes and open the Coverage page. 17) You should be able to access the names of the resources, activities and questions associated with a given outcome. 18) Sorting the table of outcomes should work as you would expect. 19) Go to the course question bank, create a new question (but dont put it in a quiz) and associate an outcome with it. 20) On the outcome Coverage page there should be a star next to the number of questions associated with that outcome. 21) Put the question in a quiz. Now the Coverage page should not have a star next to the number. 22) Go to course admin > outcomes and open the Completion Marking page. 23) Select the student you graded and click Filter. 24) Check that the student's grade information appears. Clicking on the grades and "view items" links should tell you where the grade has come from. 25) Mark an outcome complete and click save changes. 26) Move to another user using Filter and check that the previous user's grade, completion status etc is not displayed. 27) Sorting the table of outcomes should work as you would expect. Backup and Restore 28) Backup your outcome test course. 29) Restore it into a new course. 30) Verify that the outcome sets in the course settings are correct. 31) Verify that the outcome associations in the individual activities are correct.
    • Affected Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-40230_outcomes
    • Story Points (Obsolete):
      40

      Description

      Moodlerooms have developed the code for Outcomes stage 2. It has already gone through quite a lot of review from users and other stakeholders.

      Spec: http://docs.moodle.org/dev/Outcomes_Specification
      Code: https://github.com/moodlerooms/moodle/tree/INT-4475_outcomes.

      The rough peer review process will be as follows:

      1) FRONTEND team will review the UI for basic usability and process.
      2) Then, BACKEND team will review the code for structure and general coding.
      3) Then, someone from integration will do a more detailed review for final integration.

        Gliffy Diagrams

        1. competencies-badges.pptx
          2.18 MB
          Elizabeth Dalton
        2. Summer2013_OutcomesTestCases.xlsx
          20 kB
          Cecelia Green
        3. Use case badges for Moodle and PLA.docx
          153 kB
          Elizabeth Dalton
        1. addoutcome.png
          28 kB
        2. addOutcomeDialog.png
          177 kB
        3. AddOutcomeLTR.png
          30 kB
        4. AddOutcomeRTL.png
          56 kB
        5. arn-management-actions.png
          15 kB
        6. danp-outcomes1.png
          18 kB
        7. danp-outcomes2.png
          67 kB
        8. dialogue-screensize.png
          281 kB
        9. management-general-nointroduction.png
          24 kB
        10. management-mapped-courses.png
          22 kB
        11. management-outcome-add_notrequired.png
          29 kB
        12. management-outcome-description_in_hierarchy-1.png
          44 kB
        13. management-outcome-description_in_hierarchy-2.png
          44 kB
        14. management-outcome-description_in_hierarchy-3.png
          89 kB
        15. management-outcome-description_in_hierarchy-4.png
          47 kB
        16. management-outcome-uniqueid_not_unique.png
          47 kB
        17. management-table-headings.png
          10 kB
        18. management-touchscreen-actions.png
          29 kB
        19. moveOutcome.png
          172 kB
        20. report-associatedcontent-1.png
          338 kB
        21. report-associatedcontent-2.png
          10 kB
        22. report-associatedcontent-dialogue-content.png
          30 kB
        23. report-coverage-italics.png
          38 kB
        24. report-users-lexical.png
          52 kB
        25. select-action-suggestions.png
          19 kB
        26. selection-colour-accessibility.png
          62 kB
        27. selection-level-lexical.png
          18 kB
        28. selection-longnames.png
          44 kB
        29. selection-unticked.png
          30 kB
        30. theme-clean-icons.png
          10 kB
        31. theme-clean-narrow.png
          217 kB

          Issue Links

            Activity

            Hide
            Andrew Davis added a comment - - edited

            I'm still working on this but I've noticed some stuff.

            The add outcome dialog appears to extend off the right hand side. https://tracker.moodle.org/secure/attachment/33369/addOutcomeDialog.png This is on Ubuntu, Firefox 22, 13" laptop.

            If outcomes are nested the tree loads collapsed. It would probably be better to have it load fully expanded. I nested some outcomes, clicked save changes, the page reloaded and... where did my outcomes go? By all means let people collapse part of the tree but don't force them to click around to discover where their outcomes have gone.

            Should outcomes have a name? Right now the outcome page is displaying the outcome's document ID plus it's description. It's quite possible that the current behaviour is entirely correct

            It may be worth displaying the folder icon even when you have no nested outcomes. Perhaps there should be a top level node, the outcome set name. Even if we don't display the outcome set name it would be good if the UI suggested that this is indeed a tree structure, even if the user initially has it all at one level.

            If I go to the outcome set edit page and modify the outcome's fields at the top of the page then try to move to another page I get a warning popup telling me that I have unsaved changes. However if I move an outcome but don't save then leave the page I don't get any warning that I'm about to lose my work.

            Next to the individual outcomes are the words "add edit move delete". Would it look better if we used the icons that we use when editing the course page? That would certainly be more consistent.

            Instead of this dialog https://tracker.moodle.org/secure/attachment/33370/moveOutcome.png perhaps we should have a page similar to the gradebook categories and items screen. The one that is displayed when you click the move icon for a grade item. To put it another way, we have multiple places in Moodle where the user must manage a tree of things. Ideally these should all work the same.

            Show
            Andrew Davis added a comment - - edited I'm still working on this but I've noticed some stuff. The add outcome dialog appears to extend off the right hand side. https://tracker.moodle.org/secure/attachment/33369/addOutcomeDialog.png This is on Ubuntu, Firefox 22, 13" laptop. If outcomes are nested the tree loads collapsed. It would probably be better to have it load fully expanded. I nested some outcomes, clicked save changes, the page reloaded and... where did my outcomes go? By all means let people collapse part of the tree but don't force them to click around to discover where their outcomes have gone. Should outcomes have a name? Right now the outcome page is displaying the outcome's document ID plus it's description. It's quite possible that the current behaviour is entirely correct It may be worth displaying the folder icon even when you have no nested outcomes. Perhaps there should be a top level node, the outcome set name. Even if we don't display the outcome set name it would be good if the UI suggested that this is indeed a tree structure, even if the user initially has it all at one level. If I go to the outcome set edit page and modify the outcome's fields at the top of the page then try to move to another page I get a warning popup telling me that I have unsaved changes. However if I move an outcome but don't save then leave the page I don't get any warning that I'm about to lose my work. Next to the individual outcomes are the words "add edit move delete". Would it look better if we used the icons that we use when editing the course page? That would certainly be more consistent. Instead of this dialog https://tracker.moodle.org/secure/attachment/33370/moveOutcome.png perhaps we should have a page similar to the gradebook categories and items screen. The one that is displayed when you click the move icon for a grade item. To put it another way, we have multiple places in Moodle where the user must manage a tree of things. Ideally these should all work the same.
            Hide
            Andrew Davis added a comment -

            "The outcomes in this set have been modified, but not saved. Use the Save changes button to make the changes permanent or use the Cancel button to revert all changes."
            is really wordy. What about something like this
            "The modified outcomes have not been saved. Press Save changes or use the Cancel button to revert changes."
            or anything really that isn't quite as long.

            Show
            Andrew Davis added a comment - "The outcomes in this set have been modified, but not saved. Use the Save changes button to make the changes permanent or use the Cancel button to revert all changes." is really wordy. What about something like this "The modified outcomes have not been saved. Press Save changes or use the Cancel button to revert changes." or anything really that isn't quite as long.
            Hide
            Andrew Davis added a comment -

            I'm getting an error on the outcome set coverage report at /outcome/course.php?action=report_course_coverage&contextid=15&forceoutcomesetid=1 "Error reading from database"

            Actually all three reports are giving me that same error ie both Completion Marking and Activity and Performance give me the same error.

            When you are selecting an outcome to use as part of the rubric the outcome set is initially displayed collapsed. It would probably be better if it was expanded. It does seem to remember when you expanded it so this isn't a huge deal.

            When you're editing the course settings and adding outcomes the position of the add button seems odd. As I understand it its adding another set of controls so that you can add an additional outcome. Because its next to the first outcome it looks like you need to click it to add the first outcome.

            Show
            Andrew Davis added a comment - I'm getting an error on the outcome set coverage report at /outcome/course.php?action=report_course_coverage&contextid=15&forceoutcomesetid=1 "Error reading from database" Actually all three reports are giving me that same error ie both Completion Marking and Activity and Performance give me the same error. When you are selecting an outcome to use as part of the rubric the outcome set is initially displayed collapsed. It would probably be better if it was expanded. It does seem to remember when you expanded it so this isn't a huge deal. When you're editing the course settings and adding outcomes the position of the add button seems odd. As I understand it its adding another set of controls so that you can add an additional outcome. Because its next to the first outcome it looks like you need to click it to add the first outcome.
            Hide
            Andrew Davis added a comment -

            Clicking the finish peer review button. Note that I've just been looking at the UI. Apparently someone from the backend team is going to go through the code.

            Show
            Andrew Davis added a comment - Clicking the finish peer review button. Note that I've just been looking at the UI. Apparently someone from the backend team is going to go through the code.
            Hide
            Mark Nielsen added a comment -

            FYI, just updated the git branch with latest round*s* of bug fixes. Should hopefully resolve some of the issues.

            Show
            Mark Nielsen added a comment - FYI, just updated the git branch with latest round*s* of bug fixes. Should hopefully resolve some of the issues.
            Hide
            Andrew Davis added a comment -

            Hi Mark. Having another look. I'm going to repeat the more important problems that are still present for the sake of having an up to date list of the important stuff.

            The add outcome dialog appears to extend off the right hand side. https://tracker.moodle.org/secure/attachment/33369/addOutcomeDialog.png This is on Ubuntu, Firefox 22, 13" laptop.

            I'm getting an error on the outcome set coverage report at /outcome/course.php?action=report_course_coverage&contextid=15&forceoutcomesetid=1 "Error reading from database"
            Actually all three reports are giving me that same error ie both Completion Marking and Activity and Performance give me the same error.

            If I go to the outcome set edit page and modify the outcome's fields at the top of the page then try to move to another page I get a warning popup telling me that I have unsaved changes. However if I move an outcome but don't save then leave the page I don't get any warning that I'm about to lose my work.

            Show
            Andrew Davis added a comment - Hi Mark. Having another look. I'm going to repeat the more important problems that are still present for the sake of having an up to date list of the important stuff. The add outcome dialog appears to extend off the right hand side. https://tracker.moodle.org/secure/attachment/33369/addOutcomeDialog.png This is on Ubuntu, Firefox 22, 13" laptop. I'm getting an error on the outcome set coverage report at /outcome/course.php?action=report_course_coverage&contextid=15&forceoutcomesetid=1 "Error reading from database" Actually all three reports are giving me that same error ie both Completion Marking and Activity and Performance give me the same error. If I go to the outcome set edit page and modify the outcome's fields at the top of the page then try to move to another page I get a warning popup telling me that I have unsaved changes. However if I move an outcome but don't save then leave the page I don't get any warning that I'm about to lose my work.
            Hide
            Mark Nielsen added a comment -

            Andrew, are you using MySQL?

            Show
            Mark Nielsen added a comment - Andrew, are you using MySQL?
            Hide
            Andrew Davis added a comment -

            Im using PostgreSQL 9.1.9. Postgres tends to be less forgiving/more picky than MySQL.

            Show
            Andrew Davis added a comment - Im using PostgreSQL 9.1.9. Postgres tends to be less forgiving/more picky than MySQL.
            Hide
            Mark Nielsen added a comment -

            That's probably it, we do testing on MySQL - though we were trying to keep it compatible with other platforms, but the queries for the reports are pretty complicated. Will probably need some assistance to figure out where we went wrong there.

            Show
            Mark Nielsen added a comment - That's probably it, we do testing on MySQL - though we were trying to keep it compatible with other platforms, but the queries for the reports are pretty complicated. Will probably need some assistance to figure out where we went wrong there.
            Hide
            Andrew Davis added a comment - - edited

            My apologies. I should have provided you with more information. As I have some time I'll have a go at fixing this now but no promises

            This is the error on the completion marking page.

            Debug info: ERROR: non-integer constant in ORDER BY
            LINE 18: ORDER BY NULL
            ^
            SELECT
            o.id, o.docnum, o.description, ((SUM(a.rawgrade) - SUM(a.mingrade)) / (SUM(a.maxgrade) - SUM(a.mingrade)) * 100) avegrade, MIN(gi.gradetype) scales, m.id markid, m.result markresult
            FROM
            outcomes2_outcome o
            LEFT JOIN outcomes2_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = $1) LEFT JOIN outcomes2_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = $2)
            LEFT OUTER JOIN outcomes2_outcome_marks m ON o.id = m.outcomeid AND m.courseid = $3 AND m.userid = $4
            LEFT OUTER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid
            LEFT OUTER JOIN outcomes2_outcome_areas areas ON areas.id = ao.outcomeareaid
            LEFT OUTER JOIN (outcomes2_outcome_used_areas used INNER JOIN
            outcomes2_course_modules acm ON used.cmid = acm.id AND acm.course = $5 INNER JOIN
            outcomes2_modules amods ON amods.id = acm.module AND amods.visible = $6
            ) ON areas.id = used.outcomeareaid
            LEFT OUTER JOIN (
            outcomes2_outcome_attempts a INNER JOIN (
            SELECT outcomeusedareaid, userid, MAX(timemodified) timemodified
            FROM outcomes2_outcome_attempts
            GROUP BY outcomeusedareaid, userid
            ORDER BY NULL
            ) latest ON a.outcomeusedareaid = latest.outcomeusedareaid
            AND a.userid = latest.userid
            AND a.timemodified = latest.timemodified
            ) ON used.id = a.outcomeusedareaid AND a.userid = $7
            LEFT OUTER JOIN outcomes2_course_modules cmscale ON cmscale.id = used.cmid
            LEFT OUTER JOIN outcomes2_modules mods ON mods.id = cmscale.module
            LEFT OUTER JOIN outcomes2_grade_items gi ON gi.itemtype = $8 AND gi.iteminstance = cmscale.instance
            AND gi.itemmodule = mods.name AND gi.itemnumber = $9
            AND gi.gradetype = $10

            WHERE o.outcomesetid = $11 AND (edulevels.value = $12 AND subjects.value = $13) AND o.deleted = $14 AND o.assessable = $15 GROUP BY o.id
            ORDER BY description ASC LIMIT 50 OFFSET 0
            [array (
            0 => 'edulevels',
            1 => 'subjects',
            2 => '2',
            3 => '3',
            4 => '2',
            5 => 1,
            6 => '3',
            7 => 'mod',
            8 => 0,
            9 => 2,
            10 => '2',
            11 => 'all',
            12 => 'English',
            13 => 0,
            14 => 1,
            )]
            Error code: dmlreadexception
            Stack trace:

            line 426 of /lib/dml/moodle_database.php: dml_read_exception thrown
            line 248 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
            line 753 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
            line 1395 of /lib/tablelib.php: call to pgsql_native_moodle_database->get_records_sql()
            line 1415 of /lib/tablelib.php: call to table_sql->query_db()
            line 122 of /outcome/renderer.php: call to table_sql->out()
            line 160 of /outcome/classes/controller/report.php: call to core_outcome_renderer->outcome_marking()
            line ? of unknownfile: call to outcome_controller_report->report_marking_action()
            line 144 of /outcome/classes/controller/kernel.php: call to call_user_func()
            line 105 of /outcome/classes/controller/kernel.php: call to outcome_controller_kernel->execute_callback()
            line 52 of /outcome/course.php: call to outcome_controller_kernel->handle()

            Output buffer: <h2 class="main">Completion Marking</h2> <form autocomplete="off" action="http://localhost/moodle/dev/master/outcome/course.php" method="post" accept-charset="utf-8" id="mform1" class="mform"> <div style="display: none;"><input type="hidden" name="action" value="report_marking" /> <input type="hidden" name="contextid" value="15" /> <input name="sesskey" type="hidden" value="AOu2mXu3yG" /> <input name="qf_outcome_form_marking_filter" type="hidden" value="1" /> </div> <fieldset class="clearfix" id="filters"> <legend class="ftoggler">Filters</legend> <div class="advancedbutton"></div><div class="fcontainer clearfix"> <div id="fitem_id_outcomesetid" class="fitem fitem_fselect"><div class="fitemtitle"><label for="id_outcomesetid">Outcome set </label></div><div class="felement fselect"><select name="outcomesetid" id="id_outcomesetid"> <option value="2" selected="selected">Literacy</option> </select></div></div> <div id="fitem_id_userid" class="fitem fitem_fselect"><div class="fitemtitle"><label for="id_userid">User </label></div><div class="felement fselect"><select name="userid" id="id_userid"> <option value="3" selected="selected">student sam</option> </select></div></div> <div id="fitem_id_submitbutton" class="fitem fitem_actionbuttons fitem_fsubmit"><div class="felement fsubmit"><input name="submitbutton" value="Filter" type="submit" id="id_submitbutton" /></div></div> </div></fieldset> </form>

            This is on the activity and performance page.

            Debug info: ERROR: syntax error at or near "WHERE"
            LINE 45: WHERE o.outcomesetid = $16 AND (edulevels.va...
            ^
            SELECT
            o.id, o.docnum, o.description, ((SUM(a.rawgrade) - SUM(a.mingrade)) / (SUM(a.maxgrade) - SUM(a.mingrade)) * 100) avegrade, MIN(scale.gradetype) scales, COUNT(DISTINCT used.cmid) activities
            FROM
            outcomes2_outcome o
            LEFT JOIN outcomes2_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = $1) LEFT JOIN outcomes2_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = $2)
            INNER JOIN outcomes2_user u
            INNER JOIN (
            SELECT DISTINCT u.id
            FROM outcomes2_user u
            JOIN outcomes2_user_enrolments ue ON ue.userid = u.id
            JOIN outcomes2_enrol e ON (e.id = ue.enrolid AND e.courseid = $3)
            JOIN outcomes2_role_assignments ra ON ra.userid = u.id

            WHERE u.deleted = $4
            AND u.id <> $5
            AND ra.roleid = $6
            AND ra.contextid IN ($7,$8,$9)
            ) e ON e.id = u.id
            LEFT OUTER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid
            LEFT OUTER JOIN outcomes2_outcome_areas areas ON areas.id = ao.outcomeareaid
            LEFT OUTER JOIN (outcomes2_outcome_used_areas used INNER JOIN
            outcomes2_course_modules acm ON used.cmid = acm.id AND acm.course = $10 INNER JOIN
            outcomes2_modules amods ON amods.id = acm.module AND amods.visible = $11
            ) ON areas.id = used.outcomeareaid
            LEFT OUTER JOIN (
            outcomes2_outcome_attempts a INNER JOIN (
            SELECT outcomeusedareaid, userid, MAX(timemodified) timemodified
            FROM outcomes2_outcome_attempts
            GROUP BY outcomeusedareaid, userid
            ORDER BY NULL
            ) latest ON a.outcomeusedareaid = latest.outcomeusedareaid
            AND a.userid = latest.userid
            AND a.timemodified = latest.timemodified
            ) ON used.id = a.outcomeusedareaid AND a.userid = u.id
            LEFT OUTER JOIN (
            SELECT cmscale.id cmid, gi.gradetype
            FROM outcomes2_course_modules cmscale
            JOIN outcomes2_modules mods ON mods.id = cmscale.module
            JOIN outcomes2_grade_items gi ON gi.itemtype = $12 AND gi.iteminstance = cmscale.instance
            AND gi.itemmodule = mods.name AND gi.itemnumber = $13
            AND gi.gradetype = $14 AND gi.courseid = $15
            ) scale ON scale.cmid = used.cmid

            WHERE o.outcomesetid = $16 AND (edulevels.value = $17 AND subjects.value = $18) AND o.deleted = $19 AND o.assessable = $20 GROUP BY o.id
            ORDER BY description ASC LIMIT 50 OFFSET 0
            [array (
            0 => 'edulevels',
            1 => 'subjects',
            2 => '2',
            3 => 0,
            4 => '1',
            5 => '5',
            6 => '15',
            7 => '3',
            8 => '1',
            9 => '2',
            10 => 1,
            11 => 'mod',
            12 => 0,
            13 => 2,
            14 => '2',
            15 => '2',
            16 => 'all',
            17 => 'English',
            18 => 0,
            19 => 1,
            )]
            Error code: dmlreadexception
            Stack trace:

            line 426 of /lib/dml/moodle_database.php: dml_read_exception thrown
            line 248 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
            line 753 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
            line 1395 of /lib/tablelib.php: call to pgsql_native_moodle_database->get_records_sql()
            line 1415 of /lib/tablelib.php: call to table_sql->query_db()
            line 133 of /outcome/renderer.php: call to table_sql->out()
            line 192 of /outcome/classes/controller/report.php: call to core_outcome_renderer->outcome_course_performance()
            line ? of unknownfile: call to outcome_controller_report->report_course_performance_action()
            line 144 of /outcome/classes/controller/kernel.php: call to call_user_func()
            line 105 of /outcome/classes/controller/kernel.php: call to outcome_controller_kernel->execute_callback()
            line 52 of /outcome/course.php: call to outcome_controller_kernel->handle()

            Output buffer: <h2 class="main">Activity and Performance</h2> <form autocomplete="off" action="http://localhost/moodle/dev/master/outcome/course.php" method="post" accept-charset="utf-8" id="mform1" class="mform"> <div style="display: none;"><input type="hidden" name="action" value="report_course_performance" /> <input type="hidden" name="contextid" value="15" /> <input name="sesskey" type="hidden" value="AOu2mXu3yG" /> <input name="qf_outcome_form_course_performance_filter" type="hidden" value="1" /> </div> <fieldset class="clearfix" id="filters"> <legend class="ftoggler">Filters</legend> <div class="advancedbutton"></div><div class="fcontainer clearfix"> <div id="fitem_id_outcomesetid" class="fitem fitem_fselect"><div class="fitemtitle"><label for="id_outcomesetid">Outcome set </label></div><div class="felement fselect"><select name="outcomesetid" id="id_outcomesetid"> <option value="2" selected="selected">Literacy</option> </select></div></div> <div id="fitem_id_groupid" class="fitem fitem_fselect"><div class="fitemtitle"><label for="id_groupid">Group </label></div><div class="felement fselect"><select name="groupid" id="id_groupid"> <option value="0" selected="selected">All groups</option> </select></div></div> <div id="fitem_id_submitbutton" class="fitem fitem_actionbuttons fitem_fsubmit"><div class="felement fsubmit"><input name="submitbutton" value="Filter" type="submit" id="id_submitbutton" /></div></div> </div></fieldset> </form>

            This is on the coverage page.

            Debug info: ERROR: column "t1.resources" must appear in the GROUP BY clause or be used in an aggregate function
            SELECT
            o.id, o.docnum, o.description, resources, activities, questions, questionsused
            FROM
            outcomes2_outcome o
            LEFT JOIN outcomes2_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = $1) LEFT JOIN outcomes2_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = $2)
            LEFT JOIN (
            SELECT o.id, COUNT(used.id) resources
            FROM outcomes2_outcome o
            INNER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid
            INNER JOIN outcomes2_outcome_areas areas ON areas.id = ao.outcomeareaid
            INNER JOIN outcomes2_outcome_used_areas used ON areas.id = used.outcomeareaid
            INNER JOIN outcomes2_course_modules cm ON cm.id = used.cmid AND cm.course = $3
            INNER JOIN outcomes2_modules mods ON mods.id = cm.module AND mods.name IN ($4,$5,$6,$7,$8,$9,$10)
            GROUP BY o.id
            ) t1
            ON t1.id = o.id

            LEFT JOIN (
            SELECT o.id, COUNT(DISTINCT cm2.id) activities
            FROM outcomes2_outcome o
            INNER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid
            INNER JOIN outcomes2_outcome_areas areas2 ON areas2.id = ao.outcomeareaid
            INNER JOIN outcomes2_outcome_used_areas used2 ON areas2.id = used2.outcomeareaid
            INNER JOIN outcomes2_course_modules cm2 ON cm2.id = used2.cmid AND cm2.course = $11
            INNER JOIN outcomes2_modules mods2 ON mods2.id = cm2.module AND mods2.name IN ($12,$13,$14,$15,$16,$17,$18,$19,$20,$21,$22,$23,$24)
            GROUP BY o.id
            ) t2
            ON t2.id = o.id

            LEFT JOIN (
            SELECT o.id, COUNT(DISTINCT ao.id) questions, COUNT(DISTINCT cm3.id) questionsused
            FROM outcomes2_outcome o
            INNER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid
            INNER JOIN outcomes2_outcome_areas areas3 ON areas3.id = ao.outcomeareaid AND areas3.area = $25
            AND areas3.component LIKE $26 ESCAPE E'
            '
            INNER JOIN outcomes2_question q ON q.id = areas3.itemid
            INNER JOIN outcomes2_question_categories qc ON qc.id = q.category
            INNER JOIN outcomes2_context ctx ON (ctx.id = qc.contextid
            AND (ctx.id = $27 OR ctx.path LIKE $28 ESCAPE E'
            ' OR ctx.id = $29))
            LEFT JOIN outcomes2_outcome_used_areas used3 ON areas3.id = used3.outcomeareaid
            LEFT JOIN outcomes2_course_modules cm3 ON cm3.id = used3.cmid AND cm3.course = $30
            WHERE (ctx.id != $31 OR (ctx.id = $32 AND cm3.id IS NOT NULL))
            GROUP BY o.id
            ) t3
            ON t3.id = o.id

            WHERE o.outcomesetid = $33 AND (edulevels.value = $34 AND subjects.value = $35) AND o.deleted = $36 AND o.assessable = $37 GROUP BY o.id
            ORDER BY description ASC LIMIT 50 OFFSET 0
            [array (
            0 => 'edulevels',
            1 => 'subjects',
            2 => '2',
            3 => 'book',
            4 => 'resource',
            5 => 'folder',
            6 => 'imscp',
            7 => 'label',
            8 => 'page',
            9 => 'url',
            10 => '2',
            11 => 'assign',
            12 => 'chat',
            13 => 'choice',
            14 => 'data',
            15 => 'lti',
            16 => 'forum',
            17 => 'glossary',
            18 => 'lesson',
            19 => 'quiz',
            20 => 'scorm',
            21 => 'survey',
            22 => 'wiki',
            23 => 'workshop',
            24 => 'qtype',
            25 => 'qtype_%',
            26 => '15',
            27 => '/1/3/15/%',
            28 => '1',
            29 => '2',
            30 => '1',
            31 => '1',
            32 => '2',
            33 => 'all',
            34 => 'English',
            35 => 0,
            36 => 1,
            )]
            Error code: dmlreadexception
            Stack trace:

            line 426 of /lib/dml/moodle_database.php: dml_read_exception thrown
            line 248 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
            line 753 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
            line 1395 of /lib/tablelib.php: call to pgsql_native_moodle_database->get_records_sql()
            line 1415 of /lib/tablelib.php: call to table_sql->query_db()
            line 144 of /outcome/renderer.php: call to table_sql->out()
            line 224 of /outcome/classes/controller/report.php: call to core_outcome_renderer->outcome_course_coverage()
            line ? of unknownfile: call to outcome_controller_report->report_course_coverage_action()
            line 144 of /outcome/classes/controller/kernel.php: call to call_user_func()
            line 105 of /outcome/classes/controller/kernel.php: call to outcome_controller_kernel->execute_callback()
            line 52 of /outcome/course.php: call to outcome_controller_kernel->handle()

            Output buffer: <h2 class="main">Coverage</h2> <form autocomplete="off" action="http://localhost/moodle/dev/master/outcome/course.php" method="post" accept-charset="utf-8" id="mform1" class="mform"> <div style="display: none;"><input type="hidden" name="action" value="report_course_coverage" /> <input type="hidden" name="contextid" value="15" /> <input name="sesskey" type="hidden" value="AOu2mXu3yG" /> <input name="qf_outcome_form_course_coverage_filter" type="hidden" value="1" /> </div> <fieldset class="clearfix" id="filters"> <legend class="ftoggler">Filters</legend> <div class="advancedbutton"></div><div class="fcontainer clearfix"> <div id="fitem_id_outcomesetid" class="fitem fitem_fselect"><div class="fitemtitle"><label for="id_outcomesetid">Outcome set </label></div><div class="felement fselect"><select name="outcomesetid" id="id_outcomesetid"> <option value="2" selected="selected">Literacy</option> </select></div></div> <div id="fitem_id_submitbutton" class="fitem fitem_actionbuttons fitem_fsubmit"><div class="felement fsubmit"><input name="submitbutton" value="Filter" type="submit" id="id_submitbutton" /></div></div> </div></fieldset> </form>

            Show
            Andrew Davis added a comment - - edited My apologies. I should have provided you with more information. As I have some time I'll have a go at fixing this now but no promises This is the error on the completion marking page. Debug info: ERROR: non-integer constant in ORDER BY LINE 18: ORDER BY NULL ^ SELECT o.id, o.docnum, o.description, ((SUM(a.rawgrade) - SUM(a.mingrade)) / (SUM(a.maxgrade) - SUM(a.mingrade)) * 100) avegrade, MIN(gi.gradetype) scales, m.id markid, m.result markresult FROM outcomes2_outcome o LEFT JOIN outcomes2_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = $1) LEFT JOIN outcomes2_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = $2) LEFT OUTER JOIN outcomes2_outcome_marks m ON o.id = m.outcomeid AND m.courseid = $3 AND m.userid = $4 LEFT OUTER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid LEFT OUTER JOIN outcomes2_outcome_areas areas ON areas.id = ao.outcomeareaid LEFT OUTER JOIN (outcomes2_outcome_used_areas used INNER JOIN outcomes2_course_modules acm ON used.cmid = acm.id AND acm.course = $5 INNER JOIN outcomes2_modules amods ON amods.id = acm.module AND amods.visible = $6 ) ON areas.id = used.outcomeareaid LEFT OUTER JOIN ( outcomes2_outcome_attempts a INNER JOIN ( SELECT outcomeusedareaid, userid, MAX(timemodified) timemodified FROM outcomes2_outcome_attempts GROUP BY outcomeusedareaid, userid ORDER BY NULL ) latest ON a.outcomeusedareaid = latest.outcomeusedareaid AND a.userid = latest.userid AND a.timemodified = latest.timemodified ) ON used.id = a.outcomeusedareaid AND a.userid = $7 LEFT OUTER JOIN outcomes2_course_modules cmscale ON cmscale.id = used.cmid LEFT OUTER JOIN outcomes2_modules mods ON mods.id = cmscale.module LEFT OUTER JOIN outcomes2_grade_items gi ON gi.itemtype = $8 AND gi.iteminstance = cmscale.instance AND gi.itemmodule = mods.name AND gi.itemnumber = $9 AND gi.gradetype = $10 WHERE o.outcomesetid = $11 AND (edulevels.value = $12 AND subjects.value = $13) AND o.deleted = $14 AND o.assessable = $15 GROUP BY o.id ORDER BY description ASC LIMIT 50 OFFSET 0 [array ( 0 => 'edulevels', 1 => 'subjects', 2 => '2', 3 => '3', 4 => '2', 5 => 1, 6 => '3', 7 => 'mod', 8 => 0, 9 => 2, 10 => '2', 11 => 'all', 12 => 'English', 13 => 0, 14 => 1, )] Error code: dmlreadexception Stack trace: line 426 of /lib/dml/moodle_database.php: dml_read_exception thrown line 248 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end() line 753 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end() line 1395 of /lib/tablelib.php: call to pgsql_native_moodle_database->get_records_sql() line 1415 of /lib/tablelib.php: call to table_sql->query_db() line 122 of /outcome/renderer.php: call to table_sql->out() line 160 of /outcome/classes/controller/report.php: call to core_outcome_renderer->outcome_marking() line ? of unknownfile: call to outcome_controller_report->report_marking_action() line 144 of /outcome/classes/controller/kernel.php: call to call_user_func() line 105 of /outcome/classes/controller/kernel.php: call to outcome_controller_kernel->execute_callback() line 52 of /outcome/course.php: call to outcome_controller_kernel->handle() Output buffer: <h2 class="main">Completion Marking</h2> <form autocomplete="off" action="http://localhost/moodle/dev/master/outcome/course.php" method="post" accept-charset="utf-8" id="mform1" class="mform"> <div style="display: none;"><input type="hidden" name="action" value="report_marking" /> <input type="hidden" name="contextid" value="15" /> <input name="sesskey" type="hidden" value="AOu2mXu3yG" /> <input name=" qf _outcome_form_marking_filter" type="hidden" value="1" /> </div> <fieldset class="clearfix" id="filters"> <legend class="ftoggler">Filters</legend> <div class="advancedbutton"></div><div class="fcontainer clearfix"> <div id="fitem_id_outcomesetid" class="fitem fitem_fselect"><div class="fitemtitle"><label for="id_outcomesetid">Outcome set </label></div><div class="felement fselect"><select name="outcomesetid" id="id_outcomesetid"> <option value="2" selected="selected">Literacy</option> </select></div></div> <div id="fitem_id_userid" class="fitem fitem_fselect"><div class="fitemtitle"><label for="id_userid">User </label></div><div class="felement fselect"><select name="userid" id="id_userid"> <option value="3" selected="selected">student sam</option> </select></div></div> <div id="fitem_id_submitbutton" class="fitem fitem_actionbuttons fitem_fsubmit"><div class="felement fsubmit"><input name="submitbutton" value="Filter" type="submit" id="id_submitbutton" /></div></div> </div></fieldset> </form> This is on the activity and performance page. Debug info: ERROR: syntax error at or near "WHERE" LINE 45: WHERE o.outcomesetid = $16 AND (edulevels.va... ^ SELECT o.id, o.docnum, o.description, ((SUM(a.rawgrade) - SUM(a.mingrade)) / (SUM(a.maxgrade) - SUM(a.mingrade)) * 100) avegrade, MIN(scale.gradetype) scales, COUNT(DISTINCT used.cmid) activities FROM outcomes2_outcome o LEFT JOIN outcomes2_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = $1) LEFT JOIN outcomes2_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = $2) INNER JOIN outcomes2_user u INNER JOIN ( SELECT DISTINCT u.id FROM outcomes2_user u JOIN outcomes2_user_enrolments ue ON ue.userid = u.id JOIN outcomes2_enrol e ON (e.id = ue.enrolid AND e.courseid = $3) JOIN outcomes2_role_assignments ra ON ra.userid = u.id WHERE u.deleted = $4 AND u.id <> $5 AND ra.roleid = $6 AND ra.contextid IN ($7,$8,$9) ) e ON e.id = u.id LEFT OUTER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid LEFT OUTER JOIN outcomes2_outcome_areas areas ON areas.id = ao.outcomeareaid LEFT OUTER JOIN (outcomes2_outcome_used_areas used INNER JOIN outcomes2_course_modules acm ON used.cmid = acm.id AND acm.course = $10 INNER JOIN outcomes2_modules amods ON amods.id = acm.module AND amods.visible = $11 ) ON areas.id = used.outcomeareaid LEFT OUTER JOIN ( outcomes2_outcome_attempts a INNER JOIN ( SELECT outcomeusedareaid, userid, MAX(timemodified) timemodified FROM outcomes2_outcome_attempts GROUP BY outcomeusedareaid, userid ORDER BY NULL ) latest ON a.outcomeusedareaid = latest.outcomeusedareaid AND a.userid = latest.userid AND a.timemodified = latest.timemodified ) ON used.id = a.outcomeusedareaid AND a.userid = u.id LEFT OUTER JOIN ( SELECT cmscale.id cmid, gi.gradetype FROM outcomes2_course_modules cmscale JOIN outcomes2_modules mods ON mods.id = cmscale.module JOIN outcomes2_grade_items gi ON gi.itemtype = $12 AND gi.iteminstance = cmscale.instance AND gi.itemmodule = mods.name AND gi.itemnumber = $13 AND gi.gradetype = $14 AND gi.courseid = $15 ) scale ON scale.cmid = used.cmid WHERE o.outcomesetid = $16 AND (edulevels.value = $17 AND subjects.value = $18) AND o.deleted = $19 AND o.assessable = $20 GROUP BY o.id ORDER BY description ASC LIMIT 50 OFFSET 0 [array ( 0 => 'edulevels', 1 => 'subjects', 2 => '2', 3 => 0, 4 => '1', 5 => '5', 6 => '15', 7 => '3', 8 => '1', 9 => '2', 10 => 1, 11 => 'mod', 12 => 0, 13 => 2, 14 => '2', 15 => '2', 16 => 'all', 17 => 'English', 18 => 0, 19 => 1, )] Error code: dmlreadexception Stack trace: line 426 of /lib/dml/moodle_database.php: dml_read_exception thrown line 248 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end() line 753 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end() line 1395 of /lib/tablelib.php: call to pgsql_native_moodle_database->get_records_sql() line 1415 of /lib/tablelib.php: call to table_sql->query_db() line 133 of /outcome/renderer.php: call to table_sql->out() line 192 of /outcome/classes/controller/report.php: call to core_outcome_renderer->outcome_course_performance() line ? of unknownfile: call to outcome_controller_report->report_course_performance_action() line 144 of /outcome/classes/controller/kernel.php: call to call_user_func() line 105 of /outcome/classes/controller/kernel.php: call to outcome_controller_kernel->execute_callback() line 52 of /outcome/course.php: call to outcome_controller_kernel->handle() Output buffer: <h2 class="main">Activity and Performance</h2> <form autocomplete="off" action="http://localhost/moodle/dev/master/outcome/course.php" method="post" accept-charset="utf-8" id="mform1" class="mform"> <div style="display: none;"><input type="hidden" name="action" value="report_course_performance" /> <input type="hidden" name="contextid" value="15" /> <input name="sesskey" type="hidden" value="AOu2mXu3yG" /> <input name=" qf _outcome_form_course_performance_filter" type="hidden" value="1" /> </div> <fieldset class="clearfix" id="filters"> <legend class="ftoggler">Filters</legend> <div class="advancedbutton"></div><div class="fcontainer clearfix"> <div id="fitem_id_outcomesetid" class="fitem fitem_fselect"><div class="fitemtitle"><label for="id_outcomesetid">Outcome set </label></div><div class="felement fselect"><select name="outcomesetid" id="id_outcomesetid"> <option value="2" selected="selected">Literacy</option> </select></div></div> <div id="fitem_id_groupid" class="fitem fitem_fselect"><div class="fitemtitle"><label for="id_groupid">Group </label></div><div class="felement fselect"><select name="groupid" id="id_groupid"> <option value="0" selected="selected">All groups</option> </select></div></div> <div id="fitem_id_submitbutton" class="fitem fitem_actionbuttons fitem_fsubmit"><div class="felement fsubmit"><input name="submitbutton" value="Filter" type="submit" id="id_submitbutton" /></div></div> </div></fieldset> </form> This is on the coverage page. Debug info: ERROR: column "t1.resources" must appear in the GROUP BY clause or be used in an aggregate function SELECT o.id, o.docnum, o.description, resources, activities, questions, questionsused FROM outcomes2_outcome o LEFT JOIN outcomes2_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = $1) LEFT JOIN outcomes2_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = $2) LEFT JOIN ( SELECT o.id, COUNT(used.id) resources FROM outcomes2_outcome o INNER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid INNER JOIN outcomes2_outcome_areas areas ON areas.id = ao.outcomeareaid INNER JOIN outcomes2_outcome_used_areas used ON areas.id = used.outcomeareaid INNER JOIN outcomes2_course_modules cm ON cm.id = used.cmid AND cm.course = $3 INNER JOIN outcomes2_modules mods ON mods.id = cm.module AND mods.name IN ($4,$5,$6,$7,$8,$9,$10) GROUP BY o.id ) t1 ON t1.id = o.id LEFT JOIN ( SELECT o.id, COUNT(DISTINCT cm2.id) activities FROM outcomes2_outcome o INNER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid INNER JOIN outcomes2_outcome_areas areas2 ON areas2.id = ao.outcomeareaid INNER JOIN outcomes2_outcome_used_areas used2 ON areas2.id = used2.outcomeareaid INNER JOIN outcomes2_course_modules cm2 ON cm2.id = used2.cmid AND cm2.course = $11 INNER JOIN outcomes2_modules mods2 ON mods2.id = cm2.module AND mods2.name IN ($12,$13,$14,$15,$16,$17,$18,$19,$20,$21,$22,$23,$24) GROUP BY o.id ) t2 ON t2.id = o.id LEFT JOIN ( SELECT o.id, COUNT(DISTINCT ao.id) questions, COUNT(DISTINCT cm3.id) questionsused FROM outcomes2_outcome o INNER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid INNER JOIN outcomes2_outcome_areas areas3 ON areas3.id = ao.outcomeareaid AND areas3.area = $25 AND areas3.component LIKE $26 ESCAPE E' ' INNER JOIN outcomes2_question q ON q.id = areas3.itemid INNER JOIN outcomes2_question_categories qc ON qc.id = q.category INNER JOIN outcomes2_context ctx ON (ctx.id = qc.contextid AND (ctx.id = $27 OR ctx.path LIKE $28 ESCAPE E' ' OR ctx.id = $29)) LEFT JOIN outcomes2_outcome_used_areas used3 ON areas3.id = used3.outcomeareaid LEFT JOIN outcomes2_course_modules cm3 ON cm3.id = used3.cmid AND cm3.course = $30 WHERE (ctx.id != $31 OR (ctx.id = $32 AND cm3.id IS NOT NULL)) GROUP BY o.id ) t3 ON t3.id = o.id WHERE o.outcomesetid = $33 AND (edulevels.value = $34 AND subjects.value = $35) AND o.deleted = $36 AND o.assessable = $37 GROUP BY o.id ORDER BY description ASC LIMIT 50 OFFSET 0 [array ( 0 => 'edulevels', 1 => 'subjects', 2 => '2', 3 => 'book', 4 => 'resource', 5 => 'folder', 6 => 'imscp', 7 => 'label', 8 => 'page', 9 => 'url', 10 => '2', 11 => 'assign', 12 => 'chat', 13 => 'choice', 14 => 'data', 15 => 'lti', 16 => 'forum', 17 => 'glossary', 18 => 'lesson', 19 => 'quiz', 20 => 'scorm', 21 => 'survey', 22 => 'wiki', 23 => 'workshop', 24 => 'qtype', 25 => 'qtype_%', 26 => '15', 27 => '/1/3/15/%', 28 => '1', 29 => '2', 30 => '1', 31 => '1', 32 => '2', 33 => 'all', 34 => 'English', 35 => 0, 36 => 1, )] Error code: dmlreadexception Stack trace: line 426 of /lib/dml/moodle_database.php: dml_read_exception thrown line 248 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end() line 753 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end() line 1395 of /lib/tablelib.php: call to pgsql_native_moodle_database->get_records_sql() line 1415 of /lib/tablelib.php: call to table_sql->query_db() line 144 of /outcome/renderer.php: call to table_sql->out() line 224 of /outcome/classes/controller/report.php: call to core_outcome_renderer->outcome_course_coverage() line ? of unknownfile: call to outcome_controller_report->report_course_coverage_action() line 144 of /outcome/classes/controller/kernel.php: call to call_user_func() line 105 of /outcome/classes/controller/kernel.php: call to outcome_controller_kernel->execute_callback() line 52 of /outcome/course.php: call to outcome_controller_kernel->handle() Output buffer: <h2 class="main">Coverage</h2> <form autocomplete="off" action="http://localhost/moodle/dev/master/outcome/course.php" method="post" accept-charset="utf-8" id="mform1" class="mform"> <div style="display: none;"><input type="hidden" name="action" value="report_course_coverage" /> <input type="hidden" name="contextid" value="15" /> <input name="sesskey" type="hidden" value="AOu2mXu3yG" /> <input name=" qf _outcome_form_course_coverage_filter" type="hidden" value="1" /> </div> <fieldset class="clearfix" id="filters"> <legend class="ftoggler">Filters</legend> <div class="advancedbutton"></div><div class="fcontainer clearfix"> <div id="fitem_id_outcomesetid" class="fitem fitem_fselect"><div class="fitemtitle"><label for="id_outcomesetid">Outcome set </label></div><div class="felement fselect"><select name="outcomesetid" id="id_outcomesetid"> <option value="2" selected="selected">Literacy</option> </select></div></div> <div id="fitem_id_submitbutton" class="fitem fitem_actionbuttons fitem_fsubmit"><div class="felement fsubmit"><input name="submitbutton" value="Filter" type="submit" id="id_submitbutton" /></div></div> </div></fieldset> </form>
            Hide
            Andrew Davis added a comment -

            Postgres is a real pain to get started on but you may want to consider installing it and installing Moodle into it just so you can see these errors yourself. I get these errors even with a basically empty course so you dont need to worry about importing data or anything. MySQL's forgiving nature makes it a bit dangerous to develop on.

            Show
            Andrew Davis added a comment - Postgres is a real pain to get started on but you may want to consider installing it and installing Moodle into it just so you can see these errors yourself. I get these errors even with a basically empty course so you dont need to worry about importing data or anything. MySQL's forgiving nature makes it a bit dangerous to develop on.
            Hide
            Martin Dougiamas added a comment -

            We really need to have some others devs help on these queries.

            To see if they need refactoring or some sort of switch statement for diff dbs

            Show
            Martin Dougiamas added a comment - We really need to have some others devs help on these queries. To see if they need refactoring or some sort of switch statement for diff dbs
            Hide
            Martin Dougiamas added a comment -

            Generally we really try to avoid any db-specific queries, but if it really helps for performance then we do have some examples in core where we do it:

            question/engine/datalib.php delete_usage_records_for_mysql()

            http://git.moodle.org/gw?p=moodle.git;a=blob;f=question/engine/datalib.php;h=47dc925395f4f0e7ffc0a2b5b049ecf3c867702d;hb=HEAD#l736

            Show
            Martin Dougiamas added a comment - Generally we really try to avoid any db-specific queries, but if it really helps for performance then we do have some examples in core where we do it: question/engine/datalib.php delete_usage_records_for_mysql() http://git.moodle.org/gw?p=moodle.git;a=blob;f=question/engine/datalib.php;h=47dc925395f4f0e7ffc0a2b5b049ecf3c867702d;hb=HEAD#l736
            Hide
            Andrew Davis added a comment -

            Starting with the completion marking query. Using the output error message I copied the query over into postgres' SQL query app. I manually swapped out the $1 style tokens for the values from the array. That gave me this.

            SELECT
            o.id, o.docnum, o.description, ((SUM(a.rawgrade) - SUM(a.mingrade)) / (SUM(a.maxgrade) - SUM(a.mingrade)) * 100) avegrade, MIN(gi.gradetype) scales, m.id markid, m.result markresult
            FROM
            outcomes2_outcome o
            LEFT JOIN outcomes2_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = 'edulevels') LEFT JOIN outcomes2_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = 'subjects')
            LEFT OUTER JOIN outcomes2_outcome_marks m ON o.id = m.outcomeid AND m.courseid = 2 AND m.userid = 3
            LEFT OUTER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid
            LEFT OUTER JOIN outcomes2_outcome_areas areas ON areas.id = ao.outcomeareaid
            LEFT OUTER JOIN (outcomes2_outcome_used_areas used INNER JOIN
            outcomes2_course_modules acm ON used.cmid = acm.id AND acm.course = 2 INNER JOIN
            outcomes2_modules amods ON amods.id = acm.module AND amods.visible = 1
            ) ON areas.id = used.outcomeareaid
            LEFT OUTER JOIN (
            outcomes2_outcome_attempts a INNER JOIN (
            SELECT outcomeusedareaid, userid, MAX(timemodified) timemodified
            FROM outcomes2_outcome_attempts
            GROUP BY outcomeusedareaid, userid
            ORDER BY NULL
            ) latest ON a.outcomeusedareaid = latest.outcomeusedareaid
            AND a.userid = latest.userid
            AND a.timemodified = latest.timemodified
            ) ON used.id = a.outcomeusedareaid AND a.userid = 3
            LEFT OUTER JOIN outcomes2_course_modules cmscale ON cmscale.id = used.cmid
            LEFT OUTER JOIN outcomes2_modules mods ON mods.id = cmscale.module
            LEFT OUTER JOIN outcomes2_grade_items gi ON gi.itemtype = 'mod' AND gi.iteminstance = cmscale.instance
            AND gi.itemmodule = mods.name AND gi.itemnumber = 0
            AND gi.gradetype = 2
            WHERE o.outcomesetid = 2 AND (edulevels.value = 'all' AND subjects.value = 'English') AND o.deleted = 0 AND o.assessable = 1 GROUP BY o.id
            ORDER BY description ASC LIMIT 50 OFFSET 0
            

            First error

            Debug info: ERROR: non-integer constant in ORDER BY
            LINE 18: ORDER BY NULL

            Removed "ORDER BY NULL"

            New error

            ERROR: column "m.id" must appear in the GROUP BY clause or be used in an aggregate function
            LINE 2: ...rade)) * 100) avegrade, MIN(gi.gradetype) scales, m.id marki...

            Changed the second last line from
            GROUP BY o.id
            to
            GROUP BY o.id, m.id

            And it now works. Its hard for me to tell whether this is producing correct data. Mark, are you able to figure out whether I've avoided the error but am now producing bad data? I don't think these two changes should have an effect but its hard for me to be entirely confident.

            Show
            Andrew Davis added a comment - Starting with the completion marking query. Using the output error message I copied the query over into postgres' SQL query app. I manually swapped out the $1 style tokens for the values from the array. That gave me this. SELECT o.id, o.docnum, o.description, ((SUM(a.rawgrade) - SUM(a.mingrade)) / (SUM(a.maxgrade) - SUM(a.mingrade)) * 100) avegrade, MIN(gi.gradetype) scales, m.id markid, m.result markresult FROM outcomes2_outcome o LEFT JOIN outcomes2_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = 'edulevels') LEFT JOIN outcomes2_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = 'subjects') LEFT OUTER JOIN outcomes2_outcome_marks m ON o.id = m.outcomeid AND m.courseid = 2 AND m.userid = 3 LEFT OUTER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid LEFT OUTER JOIN outcomes2_outcome_areas areas ON areas.id = ao.outcomeareaid LEFT OUTER JOIN (outcomes2_outcome_used_areas used INNER JOIN outcomes2_course_modules acm ON used.cmid = acm.id AND acm.course = 2 INNER JOIN outcomes2_modules amods ON amods.id = acm.module AND amods.visible = 1 ) ON areas.id = used.outcomeareaid LEFT OUTER JOIN ( outcomes2_outcome_attempts a INNER JOIN ( SELECT outcomeusedareaid, userid, MAX(timemodified) timemodified FROM outcomes2_outcome_attempts GROUP BY outcomeusedareaid, userid ORDER BY NULL ) latest ON a.outcomeusedareaid = latest.outcomeusedareaid AND a.userid = latest.userid AND a.timemodified = latest.timemodified ) ON used.id = a.outcomeusedareaid AND a.userid = 3 LEFT OUTER JOIN outcomes2_course_modules cmscale ON cmscale.id = used.cmid LEFT OUTER JOIN outcomes2_modules mods ON mods.id = cmscale.module LEFT OUTER JOIN outcomes2_grade_items gi ON gi.itemtype = 'mod' AND gi.iteminstance = cmscale.instance AND gi.itemmodule = mods.name AND gi.itemnumber = 0 AND gi.gradetype = 2 WHERE o.outcomesetid = 2 AND (edulevels.value = 'all' AND subjects.value = 'English') AND o.deleted = 0 AND o.assessable = 1 GROUP BY o.id ORDER BY description ASC LIMIT 50 OFFSET 0 First error Debug info: ERROR: non-integer constant in ORDER BY LINE 18: ORDER BY NULL Removed "ORDER BY NULL" New error ERROR: column "m.id" must appear in the GROUP BY clause or be used in an aggregate function LINE 2: ...rade)) * 100) avegrade, MIN(gi.gradetype) scales, m.id marki... Changed the second last line from GROUP BY o.id to GROUP BY o.id, m.id And it now works. Its hard for me to tell whether this is producing correct data. Mark, are you able to figure out whether I've avoided the error but am now producing bad data? I don't think these two changes should have an effect but its hard for me to be entirely confident.
            Hide
            Andrew Davis added a comment - - edited

            Moving on to the Activity and Performance page. The query once I've done the token substitution is this.

            SELECT
            o.id, o.docnum, o.description, ((SUM(a.rawgrade) - SUM(a.mingrade)) / (SUM(a.maxgrade) - SUM(a.mingrade)) * 100) avegrade, MIN(scale.gradetype) scales, COUNT(DISTINCT used.cmid) activities
            FROM
            outcomes2_outcome o
            LEFT JOIN outcomes2_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = 'edulevels') LEFT JOIN outcomes2_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = 'subjects')
            INNER JOIN outcomes2_user u
            INNER JOIN (
            SELECT DISTINCT u.id
            FROM outcomes2_user u
            JOIN outcomes2_user_enrolments ue ON ue.userid = u.id
            JOIN outcomes2_enrol e ON (e.id = ue.enrolid AND e.courseid = 2)
            JOIN outcomes2_role_assignments ra ON ra.userid = u.id
             
            WHERE u.deleted = 0
            AND u.id <> 1
            AND ra.roleid = 5
            AND ra.contextid IN (15,3,1)
            ) e ON e.id = u.id
            LEFT OUTER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid
            LEFT OUTER JOIN outcomes2_outcome_areas areas ON areas.id = ao.outcomeareaid
            LEFT OUTER JOIN (outcomes2_outcome_used_areas used INNER JOIN
            outcomes2_course_modules acm ON used.cmid = acm.id AND acm.course = 2 INNER JOIN
            outcomes2_modules amods ON amods.id = acm.module AND amods.visible = 1
            ) ON areas.id = used.outcomeareaid
            LEFT OUTER JOIN (
            outcomes2_outcome_attempts a INNER JOIN (
            SELECT outcomeusedareaid, userid, MAX(timemodified) timemodified
            FROM outcomes2_outcome_attempts
            GROUP BY outcomeusedareaid, userid
            ORDER BY NULL
            ) latest ON a.outcomeusedareaid = latest.outcomeusedareaid
            AND a.userid = latest.userid
            AND a.timemodified = latest.timemodified
            ) ON used.id = a.outcomeusedareaid AND a.userid = u.id
            LEFT OUTER JOIN (
            SELECT cmscale.id cmid, gi.gradetype
            FROM outcomes2_course_modules cmscale
            JOIN outcomes2_modules mods ON mods.id = cmscale.module
            JOIN outcomes2_grade_items gi ON gi.itemtype = 'mod' AND gi.iteminstance = cmscale.instance
            AND gi.itemmodule = mods.name AND gi.itemnumber = 2
            AND gi.gradetype = 2 AND gi.courseid = 2
            ) scale ON scale.cmid = used.cmid
             
            WHERE o.outcomesetid = 2 AND (edulevels.value = 'all' AND subjects.value = 'English') AND o.deleted = 0 AND o.assessable = 1 GROUP BY o.id
            ORDER BY description ASC LIMIT 50 OFFSET 0
            

            I'm seeing the same error as I get within Moodle.

            ERROR: syntax error at or near "WHERE"
            LINE 44: WHERE o.outcomesetid = 2 AND (edulevels.value = 'all' AND su...

            The problem is an inner join with no "on" clause much earlier in the query. Changing
            INNER JOIN outcomes2_user u
            to
            INNER JOIN outcomes2_user u on u.id = u.id
            makes the error go away but having to append a bogus on clause is worrying. This section of the query looks odd.

            INNER JOIN outcomes2_user u
            INNER JOIN (
            SELECT DISTINCT u.id
            FROM outcomes2_user u
            JOIN outcomes2_user_enrolments ue ON ue.userid = u.id
            JOIN outcomes2_enrol e ON (e.id = ue.enrolid AND e.courseid = 2)
            JOIN outcomes2_role_assignments ra ON ra.userid = u.id
             
            WHERE u.deleted = 0
            AND u.id <> 1
            AND ra.roleid = 5
            AND ra.contextid IN (15,3,1)
            ) e ON e.id = u.id
            

            Next error. Our old friend.
            ERROR: non-integer constant in ORDER BY
            LINE 30: ORDER BY NULL

            Removing "ORDER BY NULL" made that error go away. I'm now able to execute the query.

            Show
            Andrew Davis added a comment - - edited Moving on to the Activity and Performance page. The query once I've done the token substitution is this. SELECT o.id, o.docnum, o.description, ((SUM(a.rawgrade) - SUM(a.mingrade)) / (SUM(a.maxgrade) - SUM(a.mingrade)) * 100) avegrade, MIN(scale.gradetype) scales, COUNT(DISTINCT used.cmid) activities FROM outcomes2_outcome o LEFT JOIN outcomes2_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = 'edulevels') LEFT JOIN outcomes2_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = 'subjects') INNER JOIN outcomes2_user u INNER JOIN ( SELECT DISTINCT u.id FROM outcomes2_user u JOIN outcomes2_user_enrolments ue ON ue.userid = u.id JOIN outcomes2_enrol e ON (e.id = ue.enrolid AND e.courseid = 2) JOIN outcomes2_role_assignments ra ON ra.userid = u.id   WHERE u.deleted = 0 AND u.id <> 1 AND ra.roleid = 5 AND ra.contextid IN (15,3,1) ) e ON e.id = u.id LEFT OUTER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid LEFT OUTER JOIN outcomes2_outcome_areas areas ON areas.id = ao.outcomeareaid LEFT OUTER JOIN (outcomes2_outcome_used_areas used INNER JOIN outcomes2_course_modules acm ON used.cmid = acm.id AND acm.course = 2 INNER JOIN outcomes2_modules amods ON amods.id = acm.module AND amods.visible = 1 ) ON areas.id = used.outcomeareaid LEFT OUTER JOIN ( outcomes2_outcome_attempts a INNER JOIN ( SELECT outcomeusedareaid, userid, MAX(timemodified) timemodified FROM outcomes2_outcome_attempts GROUP BY outcomeusedareaid, userid ORDER BY NULL ) latest ON a.outcomeusedareaid = latest.outcomeusedareaid AND a.userid = latest.userid AND a.timemodified = latest.timemodified ) ON used.id = a.outcomeusedareaid AND a.userid = u.id LEFT OUTER JOIN ( SELECT cmscale.id cmid, gi.gradetype FROM outcomes2_course_modules cmscale JOIN outcomes2_modules mods ON mods.id = cmscale.module JOIN outcomes2_grade_items gi ON gi.itemtype = 'mod' AND gi.iteminstance = cmscale.instance AND gi.itemmodule = mods.name AND gi.itemnumber = 2 AND gi.gradetype = 2 AND gi.courseid = 2 ) scale ON scale.cmid = used.cmid   WHERE o.outcomesetid = 2 AND (edulevels.value = 'all' AND subjects.value = 'English') AND o.deleted = 0 AND o.assessable = 1 GROUP BY o.id ORDER BY description ASC LIMIT 50 OFFSET 0 I'm seeing the same error as I get within Moodle. ERROR: syntax error at or near "WHERE" LINE 44: WHERE o.outcomesetid = 2 AND (edulevels.value = 'all' AND su... The problem is an inner join with no "on" clause much earlier in the query. Changing INNER JOIN outcomes2_user u to INNER JOIN outcomes2_user u on u.id = u.id makes the error go away but having to append a bogus on clause is worrying. This section of the query looks odd. INNER JOIN outcomes2_user u INNER JOIN ( SELECT DISTINCT u.id FROM outcomes2_user u JOIN outcomes2_user_enrolments ue ON ue.userid = u.id JOIN outcomes2_enrol e ON (e.id = ue.enrolid AND e.courseid = 2) JOIN outcomes2_role_assignments ra ON ra.userid = u.id   WHERE u.deleted = 0 AND u.id <> 1 AND ra.roleid = 5 AND ra.contextid IN (15,3,1) ) e ON e.id = u.id Next error. Our old friend. ERROR: non-integer constant in ORDER BY LINE 30: ORDER BY NULL Removing "ORDER BY NULL" made that error go away. I'm now able to execute the query.
            Hide
            Andrew Davis added a comment -

            And now Coverage

            SELECT
            o.id, o.docnum, o.description, resources, activities, questions, questionsused
            FROM
            outcomes2_outcome o
            LEFT JOIN outcomes2_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = 'edulevels') LEFT JOIN outcomes2_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = 'subjects')
            LEFT JOIN (
            SELECT o.id, COUNT(used.id) resources
            FROM outcomes2_outcome o
            INNER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid
            INNER JOIN outcomes2_outcome_areas areas ON areas.id = ao.outcomeareaid
            INNER JOIN outcomes2_outcome_used_areas used ON areas.id = used.outcomeareaid
            INNER JOIN outcomes2_course_modules cm ON cm.id = used.cmid AND cm.course = 2
            INNER JOIN outcomes2_modules mods ON mods.id = cm.module AND mods.name IN ('book','resource','folder','imscp','label','page','url')
            GROUP BY o.id
            ) t1
            ON t1.id = o.id
             
            LEFT JOIN (
            SELECT o.id, COUNT(DISTINCT cm2.id) activities
            FROM outcomes2_outcome o
            INNER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid
            INNER JOIN outcomes2_outcome_areas areas2 ON areas2.id = ao.outcomeareaid
            INNER JOIN outcomes2_outcome_used_areas used2 ON areas2.id = used2.outcomeareaid
            INNER JOIN outcomes2_course_modules cm2 ON cm2.id = used2.cmid AND cm2.course = 2
            INNER JOIN outcomes2_modules mods2 ON mods2.id = cm2.module AND mods2.name IN ('assign','chat','choice','data','lti','forum','glossary','lesson','quiz','scorm','survey','wiki','workshop')
            GROUP BY o.id
            ) t2
            ON t2.id = o.id
             
            LEFT JOIN (
            SELECT o.id, COUNT(DISTINCT ao.id) questions, COUNT(DISTINCT cm3.id) questionsused
            FROM outcomes2_outcome o
            INNER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid
            INNER JOIN outcomes2_outcome_areas areas3 ON areas3.id = ao.outcomeareaid AND areas3.area = 'qtype'
            AND areas3.component LIKE 'qtype_%' ESCAPE E'\\'
            INNER JOIN outcomes2_question q ON q.id = areas3.itemid
            INNER JOIN outcomes2_question_categories qc ON qc.id = q.category
            INNER JOIN outcomes2_context ctx ON (ctx.id = qc.contextid
            AND (ctx.id = 15 OR ctx.path LIKE '/1/3/15/%' ESCAPE E'\\' OR ctx.id = 1))
            LEFT JOIN outcomes2_outcome_used_areas used3 ON areas3.id = used3.outcomeareaid
            LEFT JOIN outcomes2_course_modules cm3 ON cm3.id = used3.cmid AND cm3.course = 2
            WHERE (ctx.id != 1 OR (ctx.id = 1 AND cm3.id IS NOT NULL))
            GROUP BY o.id
            ) t3
            ON t3.id = o.id
             
            WHERE o.outcomesetid = 2 AND (edulevels.value = 'all' AND subjects.value = 'English') AND o.deleted = 0 AND o.assessable = 1 GROUP BY o.id
            ORDER BY description ASC LIMIT 50 OFFSET 0
            

            I'm getting the same error as Moodle.

            ERROR: column "t1.resources" must appear in the GROUP BY clause or be used in an aggregate function

            This was the first in a series of very similar errors to do with selecting columns that haven't been grouped. This is the final working version.

            GROUP BY o.id, t1.resources, t2.activities, t3.questions, t3.questionsused

            To make it clearer what is coming from where I changed the select line from
            o.id, o.docnum, o.description, resources, activities, questions, questionsused
            to
            o.id, o.docnum, o.description, t1.resources, t2.activities, t3.questions, t3.questionsused

            Consider updating the select in this way although it isnt technically necessary to make the query work.

            Show
            Andrew Davis added a comment - And now Coverage SELECT o.id, o.docnum, o.description, resources, activities, questions, questionsused FROM outcomes2_outcome o LEFT JOIN outcomes2_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = 'edulevels') LEFT JOIN outcomes2_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = 'subjects') LEFT JOIN ( SELECT o.id, COUNT(used.id) resources FROM outcomes2_outcome o INNER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid INNER JOIN outcomes2_outcome_areas areas ON areas.id = ao.outcomeareaid INNER JOIN outcomes2_outcome_used_areas used ON areas.id = used.outcomeareaid INNER JOIN outcomes2_course_modules cm ON cm.id = used.cmid AND cm.course = 2 INNER JOIN outcomes2_modules mods ON mods.id = cm.module AND mods.name IN ('book','resource','folder','imscp','label','page','url') GROUP BY o.id ) t1 ON t1.id = o.id   LEFT JOIN ( SELECT o.id, COUNT(DISTINCT cm2.id) activities FROM outcomes2_outcome o INNER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid INNER JOIN outcomes2_outcome_areas areas2 ON areas2.id = ao.outcomeareaid INNER JOIN outcomes2_outcome_used_areas used2 ON areas2.id = used2.outcomeareaid INNER JOIN outcomes2_course_modules cm2 ON cm2.id = used2.cmid AND cm2.course = 2 INNER JOIN outcomes2_modules mods2 ON mods2.id = cm2.module AND mods2.name IN ('assign','chat','choice','data','lti','forum','glossary','lesson','quiz','scorm','survey','wiki','workshop') GROUP BY o.id ) t2 ON t2.id = o.id   LEFT JOIN ( SELECT o.id, COUNT(DISTINCT ao.id) questions, COUNT(DISTINCT cm3.id) questionsused FROM outcomes2_outcome o INNER JOIN outcomes2_outcome_area_outcomes ao ON o.id = ao.outcomeid INNER JOIN outcomes2_outcome_areas areas3 ON areas3.id = ao.outcomeareaid AND areas3.area = 'qtype' AND areas3.component LIKE 'qtype_%' ESCAPE E'\\' INNER JOIN outcomes2_question q ON q.id = areas3.itemid INNER JOIN outcomes2_question_categories qc ON qc.id = q.category INNER JOIN outcomes2_context ctx ON (ctx.id = qc.contextid AND (ctx.id = 15 OR ctx.path LIKE '/1/3/15/%' ESCAPE E'\\' OR ctx.id = 1)) LEFT JOIN outcomes2_outcome_used_areas used3 ON areas3.id = used3.outcomeareaid LEFT JOIN outcomes2_course_modules cm3 ON cm3.id = used3.cmid AND cm3.course = 2 WHERE (ctx.id != 1 OR (ctx.id = 1 AND cm3.id IS NOT NULL)) GROUP BY o.id ) t3 ON t3.id = o.id   WHERE o.outcomesetid = 2 AND (edulevels.value = 'all' AND subjects.value = 'English') AND o.deleted = 0 AND o.assessable = 1 GROUP BY o.id ORDER BY description ASC LIMIT 50 OFFSET 0 I'm getting the same error as Moodle. ERROR: column "t1.resources" must appear in the GROUP BY clause or be used in an aggregate function This was the first in a series of very similar errors to do with selecting columns that haven't been grouped. This is the final working version. GROUP BY o.id, t1.resources, t2.activities, t3.questions, t3.questionsused To make it clearer what is coming from where I changed the select line from o.id, o.docnum, o.description, resources, activities, questions, questionsused to o.id, o.docnum, o.description, t1.resources, t2.activities, t3.questions, t3.questionsused Consider updating the select in this way although it isnt technically necessary to make the query work.
            Hide
            Andrew Davis added a comment -

            That explains the errors on the three reports. Hopefully my explanations are clear. Are you able to replicate the changes and verify that these changes do not alter the query output in any undesirable ways?

            Show
            Andrew Davis added a comment - That explains the errors on the three reports. Hopefully my explanations are clear. Are you able to replicate the changes and verify that these changes do not alter the query output in any undesirable ways?
            Hide
            Kris Stokking added a comment -

            1. Re: ORDER BY NULL's - this is generally put into MySQL queries to disregard sorting and improve performance. Mark's going to test it without and verify that the EXPLAIN and response times are equivalent.
            2. Re: GROUP BY's - MySQL handles GROUP BY's differently than other RDBMS's, in that you don't need to specify all columns needed by the GROUP BY. This introduces the potential to produce incorrect or random results, so we'll definitely need to fix this and verify the results are equivalent before and after the change.
            3. Re: INNER JOIN's - Very good catch. I'd agree that the query needs to be looked at a bit further to see if we can join on columns, or at least make it a little more explicit that we're doing a cross join.

            So, it looks like we should be able to handle each problem, though there may be implications we need to work around (namely performance). We'll post a fix once Mark has the new code in place and we've had a chance to verify the result sets and EXPLAIN's. Thanks very much for the help Andrew!

            Show
            Kris Stokking added a comment - 1. Re: ORDER BY NULL's - this is generally put into MySQL queries to disregard sorting and improve performance. Mark's going to test it without and verify that the EXPLAIN and response times are equivalent. 2. Re: GROUP BY's - MySQL handles GROUP BY's differently than other RDBMS's, in that you don't need to specify all columns needed by the GROUP BY. This introduces the potential to produce incorrect or random results, so we'll definitely need to fix this and verify the results are equivalent before and after the change. 3. Re: INNER JOIN's - Very good catch. I'd agree that the query needs to be looked at a bit further to see if we can join on columns, or at least make it a little more explicit that we're doing a cross join. So, it looks like we should be able to handle each problem, though there may be implications we need to work around (namely performance). We'll post a fix once Mark has the new code in place and we've had a chance to verify the result sets and EXPLAIN's. Thanks very much for the help Andrew!
            Hide
            Martin Dougiamas added a comment -

            Andrew please continue reviewing under MySQL as well to see if you spot any issues in the UI as well.

            Show
            Martin Dougiamas added a comment - Andrew please continue reviewing under MySQL as well to see if you spot any issues in the UI as well.
            Hide
            Andrew Davis added a comment - - edited

            I think I've covered most of the UI reasonably thoroughly above (before the SQL stuff). However I now have this running in mysql so I can see the reports so I have a few comments about them.

            Clicking the Outcomes node in the course navigation block takes you to a page listing Outcome sets. That page has links to the per outcome reports and the unmapped items page. Perhaps the node should be "Outcome sets" with "Unmapped items" as a child node. Just an idea.

            When I opened the completing marking screen it was initially unclear what I was looking at. Typically when you are presented with data and a filter option you initially see everything but then you can apply a filter to reduce the amount of data presented. I'm not sure if this page wants to default to showing multiple students until a filter is applied, showing no student data until a filter is applied or if we should just be using a word other than filter.

            Also, this issue is missing testing instructions. Beyond actual testing, testing instructions would be instructive for everyone looking at this issue wanting a sense of how it will work.

            Show
            Andrew Davis added a comment - - edited I think I've covered most of the UI reasonably thoroughly above (before the SQL stuff). However I now have this running in mysql so I can see the reports so I have a few comments about them. Clicking the Outcomes node in the course navigation block takes you to a page listing Outcome sets. That page has links to the per outcome reports and the unmapped items page. Perhaps the node should be "Outcome sets" with "Unmapped items" as a child node. Just an idea. When I opened the completing marking screen it was initially unclear what I was looking at. Typically when you are presented with data and a filter option you initially see everything but then you can apply a filter to reduce the amount of data presented. I'm not sure if this page wants to default to showing multiple students until a filter is applied, showing no student data until a filter is applied or if we should just be using a word other than filter. Also, this issue is missing testing instructions. Beyond actual testing, testing instructions would be instructive for everyone looking at this issue wanting a sense of how it will work.
            Hide
            Andrew Davis added a comment - - edited

            I've added three commits to my github repository that may be of interest. They're not to do with the queries. Just some small fixes for some of the other items listed above. Feel free to either pull these commits in, squish them into your own commits or disregard them in favour of your own fixes.

            To fix this https://tracker.moodle.org/secure/attachment/33369/addOutcomeDialog.png
            https://github.com/andyjdavis/moodle/commit/eb9e1145a1f45ae1e5bcb1f4c582938c25f504b0
            Not sure if this will make it look terrible on other screens. If so we may have to do something clever with @media tags.

            A slightly less wordy string. https://github.com/andyjdavis/moodle/commit/030532d348f496a44f7ac74c1f75e6ba392d3467

            Some white space corrections. https://github.com/andyjdavis/moodle/commit/4e9d74aec378b893111f024913d7b8936296bb8f

            Preserving my identity against the commits is not necessary so don't worry about that when making the "pull/pull and squish/disregard" decision.

            Show
            Andrew Davis added a comment - - edited I've added three commits to my github repository that may be of interest. They're not to do with the queries. Just some small fixes for some of the other items listed above. Feel free to either pull these commits in, squish them into your own commits or disregard them in favour of your own fixes. To fix this https://tracker.moodle.org/secure/attachment/33369/addOutcomeDialog.png https://github.com/andyjdavis/moodle/commit/eb9e1145a1f45ae1e5bcb1f4c582938c25f504b0 Not sure if this will make it look terrible on other screens. If so we may have to do something clever with @media tags. A slightly less wordy string. https://github.com/andyjdavis/moodle/commit/030532d348f496a44f7ac74c1f75e6ba392d3467 Some white space corrections. https://github.com/andyjdavis/moodle/commit/4e9d74aec378b893111f024913d7b8936296bb8f Preserving my identity against the commits is not necessary so don't worry about that when making the "pull/pull and squish/disregard" decision.
            Hide
            Mark Nielsen added a comment -

            Hi Andrew! Thank you so much for all of your help with those queries, really helped me out! I finally got postgres installed and was able to test out the new queries. I hope that I now have everything working for you in the latest update. You can see what is new from my last push here. The last commit is probably most relevant to you as it has all the changes from your review.

            Note: regarding the fix for the add outcome dialog, I didn't go with your fix and I actually couldn't reproduce it. What I ended up doing was instead of hardcoding the width at 450px, I'm instead enforcing a minimum width of 450px which should allow the modal to expand if needed. Hopefully this addresses your problem.

            Again, thanks for all your help!

            Show
            Mark Nielsen added a comment - Hi Andrew! Thank you so much for all of your help with those queries, really helped me out! I finally got postgres installed and was able to test out the new queries. I hope that I now have everything working for you in the latest update. You can see what is new from my last push here . The last commit is probably most relevant to you as it has all the changes from your review. Note: regarding the fix for the add outcome dialog, I didn't go with your fix and I actually couldn't reproduce it. What I ended up doing was instead of hardcoding the width at 450px, I'm instead enforcing a minimum width of 450px which should allow the modal to expand if needed. Hopefully this addresses your problem. Again, thanks for all your help!
            Hide
            Andrew Davis added a comment -

            Hi Mark.

            Two of the three reports are now working nicely however Activity and Performance report is still giving me

            Debug info: ERROR: non-integer constant in ORDER BY
            LINE 30: ORDER BY NULL
            ^
            SELECT
            o.id, o.docnum, o.description, ((SUM(a.rawgrade) - SUM(a.mingrade)) / (SUM(a.maxgrade) - SUM(a.mingrade)) * 100) avegrade, MIN(scale.gradetype) scales, COUNT(DISTINCT used.cmid) activities
            FROM
            outcomes2_outcome o
            LEFT JOIN outcomes2_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = $1) LEFT JOIN outcomes2_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = $2)
            INNER JOIN outcomes2_user u ON 1 = 1
            INNER JOIN (
            SELECT DISTINCT u.id
            FROM outcomes2_user u
            JOIN outcomes2_user_enrolments ue ON ue.userid = u.id
            JOIN outcomes2_enrol e ON (e.id = ue.enrolid AND e.courseid = $3)
            JOIN outcomes2_role_assignments ra ON ra.userid = u.id
            .
            .
            .
            

            Your change to the add outcome dialog does indeed fix my problem

            One other thing I noticed. We could possibly split this out to a separate MDL.

            When adding a new outcome and entering the education level its just a text box so typos and differences in case results in multiple levels being created.

            Show
            Andrew Davis added a comment - Hi Mark. Two of the three reports are now working nicely however Activity and Performance report is still giving me Debug info: ERROR: non-integer constant in ORDER BY LINE 30: ORDER BY NULL ^ SELECT o.id, o.docnum, o.description, ((SUM(a.rawgrade) - SUM(a.mingrade)) / (SUM(a.maxgrade) - SUM(a.mingrade)) * 100) avegrade, MIN(scale.gradetype) scales, COUNT(DISTINCT used.cmid) activities FROM outcomes2_outcome o LEFT JOIN outcomes2_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = $1) LEFT JOIN outcomes2_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = $2) INNER JOIN outcomes2_user u ON 1 = 1 INNER JOIN ( SELECT DISTINCT u.id FROM outcomes2_user u JOIN outcomes2_user_enrolments ue ON ue.userid = u.id JOIN outcomes2_enrol e ON (e.id = ue.enrolid AND e.courseid = $3) JOIN outcomes2_role_assignments ra ON ra.userid = u.id . . . Your change to the add outcome dialog does indeed fix my problem One other thing I noticed. We could possibly split this out to a separate MDL. When adding a new outcome and entering the education level its just a text box so typos and differences in case results in multiple levels being created.
            Hide
            Mark Nielsen added a comment -

            Sorry about that Andrew, just pushed up a fix. I think I was trying it with and without and then forgot to remove it again.

            Yes, the subjects/education levels could be improved like how tags are entered with a nice AJAXy interface. This could indeed be done at a later date as a nice improvement, but due note that we think a more common use case would be the importing of standards instead of manually creating them. The import system is pluggable, so folks should be able to add new formats easily.

            Show
            Mark Nielsen added a comment - Sorry about that Andrew, just pushed up a fix. I think I was trying it with and without and then forgot to remove it again. Yes, the subjects/education levels could be improved like how tags are entered with a nice AJAXy interface. This could indeed be done at a later date as a nice improvement, but due note that we think a more common use case would be the importing of standards instead of manually creating them. The import system is pluggable, so folks should be able to add new formats easily.
            Hide
            Andrew Davis added a comment -

            I've raised four MDLs. They're all linked to this one. However, I think this can go up for integration. See if there are any commits that can be logically squished.

            We still need some testing instructions for this issue.

            Once those are done put this up for integration whenever you are ready.

            Show
            Andrew Davis added a comment - I've raised four MDLs. They're all linked to this one. However, I think this can go up for integration. See if there are any commits that can be logically squished. We still need some testing instructions for this issue. Once those are done put this up for integration whenever you are ready.
            Hide
            Andrew Davis added a comment -

            Has there been any progress on the testing instructions? Just needs to be enough to give a tester confidence that this has been integrated correctly and that it all appears to be working as it should.

            Show
            Andrew Davis added a comment - Has there been any progress on the testing instructions? Just needs to be enough to give a tester confidence that this has been integrated correctly and that it all appears to be working as it should.
            Hide
            Cecelia Green added a comment -

            Attached test cases from Moodlerooms.

            Show
            Cecelia Green added a comment - Attached test cases from Moodlerooms.
            Hide
            Andrew Davis added a comment -

            I'm taking the liberty of putting this up for integration.

            Show
            Andrew Davis added a comment - I'm taking the liberty of putting this up for integration.
            Hide
            Damyon Wiese added a comment - - edited

            There are 274 files conflicting with integration in this patch. (I was not expecting a patch based on 24 to be submitted for integration). I'll continue with the review and try and give you some additional feedback - but this will defintely need rebasing before it gets integrated.

            Show
            Damyon Wiese added a comment - - edited There are 274 files conflicting with integration in this patch. (I was not expecting a patch based on 24 to be submitted for integration). I'll continue with the review and try and give you some additional feedback - but this will defintely need rebasing before it gets integrated.
            Hide
            Andrew Davis added a comment -

            That's got to be some sort of record.

            Show
            Andrew Davis added a comment - That's got to be some sort of record.
            Hide
            Damyon Wiese added a comment -

            Thanks Mark for this code - it is really excellent - in particular the javascript has been well done.

            I did a thorough review in order to get you as much feedback as possible - but the main blocker here is that this is based on 2.4 and needs rebasing on master.

            Comments:

            • This branch is based on Moodle 2.4. It must be rebased onto master before it can be integrated.
            • admin/tool/outcome should not exist - it should use core files for installing db tables etc.
            • core_outcome is not a plugin - do not abuse the config_plugins table (no set_config('enable', 1, 'core_outcome'));
            • Upgrade code should not represent the evolving DB structure from before this is integrated (e.g. "Changing nullability of field description on table outcome to not null"). This should be collapsed to create the correct/final table definitions once.
            • Upgrade steps do not have comments and most of them can probably be removed.
            • Comments - many comments do not meet the coding guidelines.
            • Icons - The icons need reviewing by Barbara - I suspect they will need changes because they use colours and do not follow the neutral gray style. (http://docs.moodle.org/dev/Moodle_icons_2.4)
            • @author tag is in the list of allowed tags: http://docs.moodle.org/dev/Coding_style#PHPDoc
            • Many classes/functions missing phpdocs: e.g.
              • admin/tool/outcome/settingslib.php
              • backup/moodle2/restore_outcome_area_plugin.class.php
              • backup/moodle2/restore_plan_builder.class.php
            • CSS uses direction specific attributes - but no provision for .dir-rtl - makes me think this has not been tested in RTL at all.
            • General comment - but singletons are pointless in php - and I would prefer much simpler static class methods / variables.
            • Once this is updated to master - take care to make sure the hooks for course_created/updated etc are added every where a course can be created/updated (e.g. phpunit generators, web services, bulk upload tools).
            • In general you seem to be assuming that a module can only have one outcome area even though the db structure allows multiple. Consider e.g. workshop where you have separate areas for student marking and teacher marking.
            • Autoloading will remove alot of the need for require_once($CFG->dirroot.'/outcome/lib.php')
            • grade/grading/form/rubric/lib.php has unused global $CFG
            • Outcome javascript is directly using YUI Panels, which means they do not get default enhancements for accessibility, zindex, fullscreen support etc. M.core.dialogue is for this. You have then rolled your own modules for accessible dialogues - we can't maintain these all over Moodle, please add any specific enhancements to M.core.dialogue and use that.
            • The dialogues have different styling to core Moodle dialogues.
            • Outcome javascript modules should be converted to use shifter.
            • javascript comments do not follow the coding style always
            • javascript functions are also not always documented
            • javascript uses click handlers without also listening for keyboard events
            • I like your use of handlebars for templates in YUI
            • Can we install one simple generic outcome set by default?
            • Can we have a generate button to generate a unique id for an outcome set ?
            • There is no fallback for any of the javascript - without javascript you cannot do anything with outcomes. Note - I feel this is actually fine - but we need a policy decision before we can allow it (I created one: MDL-41535)
            • The table at outcome/admin.php?action=outcomeset has lots of empty rows for no reason.
            • I selected an outcome set and clicked OK, I got a warning telling me to click "Add" then "OK". This is annoying - just do it for me please.

            Other things to consider.

            • Should we keep support for "Legacy outcomes" - IMO - no - we should only have one outcomes system in core. This may not be realistic as we have to support upgrades etc - but it still sucks a bit to have 2.
            Show
            Damyon Wiese added a comment - Thanks Mark for this code - it is really excellent - in particular the javascript has been well done. I did a thorough review in order to get you as much feedback as possible - but the main blocker here is that this is based on 2.4 and needs rebasing on master. Comments: This branch is based on Moodle 2.4. It must be rebased onto master before it can be integrated. admin/tool/outcome should not exist - it should use core files for installing db tables etc. core_outcome is not a plugin - do not abuse the config_plugins table (no set_config('enable', 1, 'core_outcome')); Upgrade code should not represent the evolving DB structure from before this is integrated (e.g. "Changing nullability of field description on table outcome to not null"). This should be collapsed to create the correct/final table definitions once. Upgrade steps do not have comments and most of them can probably be removed. Comments - many comments do not meet the coding guidelines. Icons - The icons need reviewing by Barbara - I suspect they will need changes because they use colours and do not follow the neutral gray style. ( http://docs.moodle.org/dev/Moodle_icons_2.4 ) @author tag is in the list of allowed tags: http://docs.moodle.org/dev/Coding_style#PHPDoc Many classes/functions missing phpdocs: e.g. admin/tool/outcome/settingslib.php backup/moodle2/restore_outcome_area_plugin.class.php backup/moodle2/restore_plan_builder.class.php CSS uses direction specific attributes - but no provision for .dir-rtl - makes me think this has not been tested in RTL at all. General comment - but singletons are pointless in php - and I would prefer much simpler static class methods / variables. Once this is updated to master - take care to make sure the hooks for course_created/updated etc are added every where a course can be created/updated (e.g. phpunit generators, web services, bulk upload tools). In general you seem to be assuming that a module can only have one outcome area even though the db structure allows multiple. Consider e.g. workshop where you have separate areas for student marking and teacher marking. Autoloading will remove alot of the need for require_once($CFG->dirroot.'/outcome/lib.php') grade/grading/form/rubric/lib.php has unused global $CFG Outcome javascript is directly using YUI Panels, which means they do not get default enhancements for accessibility, zindex, fullscreen support etc. M.core.dialogue is for this. You have then rolled your own modules for accessible dialogues - we can't maintain these all over Moodle, please add any specific enhancements to M.core.dialogue and use that. The dialogues have different styling to core Moodle dialogues. Outcome javascript modules should be converted to use shifter. javascript comments do not follow the coding style always javascript functions are also not always documented javascript uses click handlers without also listening for keyboard events I like your use of handlebars for templates in YUI Can we install one simple generic outcome set by default? Can we have a generate button to generate a unique id for an outcome set ? There is no fallback for any of the javascript - without javascript you cannot do anything with outcomes. Note - I feel this is actually fine - but we need a policy decision before we can allow it (I created one: MDL-41535 ) The table at outcome/admin.php?action=outcomeset has lots of empty rows for no reason. I selected an outcome set and clicked OK, I got a warning telling me to click "Add" then "OK". This is annoying - just do it for me please. Other things to consider. Should we keep support for "Legacy outcomes" - IMO - no - we should only have one outcomes system in core. This may not be realistic as we have to support upgrades etc - but it still sucks a bit to have 2.
            Hide
            CiBoT added a comment -

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

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

            Damyon,

            Thanks very much for the thorough review! Mark has already started working on the items that we flat out agree with, though some bear further explanation or discussion (I'll take the high-level ones, he'll take the more technical ones). Timing will become an issue here as it will likely take 2-3 actual weeks to have everything we need implemented, and that comes dangerously close to code complete.

            Here are my responses:

            Icons - The icons need reviewing by Barbara - I suspect they will need changes because they use colours and do not follow the neutral gray style.

            I've asked that Jason provide updated monochrome images for PNG and SVG. We should have that soon.

            Can we install one simple generic outcome set by default?

            We could, but I don't think that we should, for the same reasons why we don't include an example course when installing Moodle. There are many, many ways to configure Outcome Sets, and most of them are highly dependent on region. We've supplied the ability to easily load sets from the ASN (public and freely available, but US centric) - a user just needs to download the file and upload to the site. I think it would be more beneficial to make this clearer in the documentation than to include a sample one, and it offers better support for expanding into other regions and formats.

            Can we have a generate button to generate a unique id for an outcome set?

            It's possible, but I don't think it should be required for phase 1. While self-created standards are possible, it's not something that "bubbles up" well - it's better to promote the uses of outcomes from accrediting bodies, such as the ASN, so that they can be used across institutions and even across applications.

            Should we keep support for "Legacy outcomes" - IMO - no - we should only have one outcomes system in core. This may not be realistic as we have to support upgrades etc - but it still sucks a bit to have 2.

            Outcomes were developed with this in mind - the new system is completely independent of the old system. The 2 can be used in parallel but it really should only be for the purposes of migration. We didn't want to force sites to choose one or the other, and building an automatic migration would have been too complex (especially given the low usage of legacy outcomes). At some point in the future, legacy outcomes should be removed from the code, but I imagine that we would want to give some pretty significant timelines (e.g. at least a year) to allow for migration.

            Show
            Kris Stokking added a comment - Damyon, Thanks very much for the thorough review! Mark has already started working on the items that we flat out agree with, though some bear further explanation or discussion (I'll take the high-level ones, he'll take the more technical ones). Timing will become an issue here as it will likely take 2-3 actual weeks to have everything we need implemented, and that comes dangerously close to code complete. Here are my responses: Icons - The icons need reviewing by Barbara - I suspect they will need changes because they use colours and do not follow the neutral gray style. I've asked that Jason provide updated monochrome images for PNG and SVG. We should have that soon. Can we install one simple generic outcome set by default? We could, but I don't think that we should, for the same reasons why we don't include an example course when installing Moodle. There are many, many ways to configure Outcome Sets, and most of them are highly dependent on region. We've supplied the ability to easily load sets from the ASN (public and freely available, but US centric) - a user just needs to download the file and upload to the site. I think it would be more beneficial to make this clearer in the documentation than to include a sample one, and it offers better support for expanding into other regions and formats. Can we have a generate button to generate a unique id for an outcome set? It's possible, but I don't think it should be required for phase 1. While self-created standards are possible, it's not something that "bubbles up" well - it's better to promote the uses of outcomes from accrediting bodies, such as the ASN, so that they can be used across institutions and even across applications. Should we keep support for "Legacy outcomes" - IMO - no - we should only have one outcomes system in core. This may not be realistic as we have to support upgrades etc - but it still sucks a bit to have 2. Outcomes were developed with this in mind - the new system is completely independent of the old system. The 2 can be used in parallel but it really should only be for the purposes of migration. We didn't want to force sites to choose one or the other, and building an automatic migration would have been too complex (especially given the low usage of legacy outcomes). At some point in the future, legacy outcomes should be removed from the code, but I imagine that we would want to give some pretty significant timelines (e.g. at least a year) to allow for migration.
            Hide
            Jason Hardin added a comment -

            For the icon question. What type of icons are these considered? They aren't activity, they aren't block and in the instance they are being used they are also not action icons. These are navigational and informative icons. I am concerned that making the monochromatic will actually hinder the user's visualization of the grouping of the outcomes. I added Barbara Ramiro to the ticket per Kris's request.

            Show
            Jason Hardin added a comment - For the icon question. What type of icons are these considered? They aren't activity, they aren't block and in the instance they are being used they are also not action icons. These are navigational and informative icons. I am concerned that making the monochromatic will actually hinder the user's visualization of the grouping of the outcomes. I added Barbara Ramiro to the ticket per Kris's request.
            Hide
            Mark Nielsen added a comment -

            Hello Damyon Wiese,

            Comments - many comments do not meet the coding guidelines.

            I believe the invalid comments you are referring to are something like, /** @var $activity cm_info */ The reason why I add these is so that the IDE knows that a variable is an instance of class X. I believe this is supported by IDEs like PHPStorm and Netbeans. IMO, this is very important for static analysis, refactoring, ease of coding, etc. Could we add this as an exception to CodeSniffer? Open to suggestions.

            Once this is updated to master - take care to make sure the hooks for course_created/updated etc are added every where a course can be created/updated (e.g. phpunit generators, web services, bulk upload tools).

            Could you please elaborate here? I'm not 100% sure how it is related to outcomes.

            In general you seem to be assuming that a module can only have one outcome area even though the db structure allows multiple. Consider e.g. workshop where you have separate areas for student marking and teacher marking.

            We originally had the ability for activities to customize this, but reverted it to make reporting possible/easier. This doesn't mean that other places in Moodle cannot add outcomes to other areas, but they will not be reported on in our main reports. In the future, we may want to add better customization of the module edit screen for activities.

            javascript uses click handlers without also listening for keyboard events

            Can you please be more specific? I'm not sure where I need to also listen to keyboard events.

            The table at outcome/admin.php?action=outcomeset has lots of empty rows for no reason.

            Can you perhaps provide a screen shot? I'm not seeing empty rows.

            Many classes/functions missing phpdocs: e.g.
            javascript functions are also not always documented
            javascript comments do not follow the coding style always

            I didn't know we were using YUIDoc for JS comments, so I was basically doing a PHPDoc, YUIDoc'ish, whatever came to mind docs Regarding missing docs, are you looking to have docs on everything or just major things? I generally skip PHPDocs that basically just say the obvious about a method/property. I do see that I don't doc some classes so I'll do that for sure.

            General comment - but singletons are pointless in php - and I would prefer much simpler static class methods / variables.

            Regarding static classes, those are not mockable which makes unit testing very difficult. In addition, static classes create a hard dependency on that class. So, if I make a class that depends on my static class, then my class can only use that one implementation. I couldn't pass it a subclass for example. This makes code more tightly coupled and brittle IMO. The list goes on and I have done a lot of reading about it so there is material out there if you want to check it out.

            Regarding singletons, it was a means to an end. What I was trying to accomplish was to create a list of services that act as the "public" API for interacting with outcomes and hide away the implementation details. At the time of writing, Moodle didn't have autoloading so I went with a lib file that was as minimal as possible so users could consume it easily and see what is available. Also, users wouldn't have to dig through the classes directory and see what files they needed to include to get running. With autoloading, I'll be able to take another look at this, EG: users can make instances of the service classes themselves or let them work with a more sensible version of outcome_service_factory.

            In addition, services like these are implemented in Moodle already, like $DB, $OUTPUT, $PAGE. These are singletons in essence even though I can make new instances of them, just like how I can make new instances of the outcome services as well. Correct me if I'm wrong, but Moodle doesn't have a great way to deliver services to other parts of Moodle other than via globals or roll your own. Other projects use things like dependency injection containers to build classes and inject them into dependent classes. This is obviously out of scope for this project, but that's why I rolled my own and did what I did.

            Show
            Mark Nielsen added a comment - Hello Damyon Wiese , Comments - many comments do not meet the coding guidelines. I believe the invalid comments you are referring to are something like, /** @var $activity cm_info */ The reason why I add these is so that the IDE knows that a variable is an instance of class X. I believe this is supported by IDEs like PHPStorm and Netbeans. IMO, this is very important for static analysis, refactoring, ease of coding, etc. Could we add this as an exception to CodeSniffer? Open to suggestions. Once this is updated to master - take care to make sure the hooks for course_created/updated etc are added every where a course can be created/updated (e.g. phpunit generators, web services, bulk upload tools). Could you please elaborate here? I'm not 100% sure how it is related to outcomes. In general you seem to be assuming that a module can only have one outcome area even though the db structure allows multiple. Consider e.g. workshop where you have separate areas for student marking and teacher marking. We originally had the ability for activities to customize this, but reverted it to make reporting possible/easier. This doesn't mean that other places in Moodle cannot add outcomes to other areas, but they will not be reported on in our main reports. In the future, we may want to add better customization of the module edit screen for activities. javascript uses click handlers without also listening for keyboard events Can you please be more specific? I'm not sure where I need to also listen to keyboard events. The table at outcome/admin.php?action=outcomeset has lots of empty rows for no reason. Can you perhaps provide a screen shot? I'm not seeing empty rows. Many classes/functions missing phpdocs: e.g. javascript functions are also not always documented javascript comments do not follow the coding style always I didn't know we were using YUIDoc for JS comments, so I was basically doing a PHPDoc, YUIDoc'ish, whatever came to mind docs Regarding missing docs, are you looking to have docs on everything or just major things? I generally skip PHPDocs that basically just say the obvious about a method/property. I do see that I don't doc some classes so I'll do that for sure. General comment - but singletons are pointless in php - and I would prefer much simpler static class methods / variables. Regarding static classes, those are not mockable which makes unit testing very difficult. In addition, static classes create a hard dependency on that class. So, if I make a class that depends on my static class, then my class can only use that one implementation. I couldn't pass it a subclass for example. This makes code more tightly coupled and brittle IMO. The list goes on and I have done a lot of reading about it so there is material out there if you want to check it out. Regarding singletons, it was a means to an end. What I was trying to accomplish was to create a list of services that act as the "public" API for interacting with outcomes and hide away the implementation details. At the time of writing, Moodle didn't have autoloading so I went with a lib file that was as minimal as possible so users could consume it easily and see what is available. Also, users wouldn't have to dig through the classes directory and see what files they needed to include to get running. With autoloading, I'll be able to take another look at this, EG: users can make instances of the service classes themselves or let them work with a more sensible version of outcome_service_factory. In addition, services like these are implemented in Moodle already, like $DB, $OUTPUT, $PAGE. These are singletons in essence even though I can make new instances of them, just like how I can make new instances of the outcome services as well. Correct me if I'm wrong, but Moodle doesn't have a great way to deliver services to other parts of Moodle other than via globals or roll your own. Other projects use things like dependency injection containers to build classes and inject them into dependent classes. This is obviously out of scope for this project, but that's why I rolled my own and did what I did.
            Hide
            Andrew Davis added a comment - - edited

            javascript uses click handlers without also listening for keyboard events

            Can you please be more specific? I'm not sure where I need to also listen to keyboard events.

            Damyon can provide more details but what I believe he means is that its important for accessibility that a user be able to use moodle with just the keyboard. It should be possible to tab around the interface and activate things without use of the mouse.

            I'm not sure if its the most directly applicable example to what needs doing for this issue but if you look in lib/yui/build/moodle-core-dock/moodle-core-dock.js you can see Javascript that makes it possible to use the dock menu via the keyboard.

            In terms of where in the outcomes code he is talking about I believe he is referring to /grade/grading/form/rubric/js/rubriceditor.js. Its used if you set up an assignment, set it to rubric and choose to define a new rubric. When you move focus to "click to edit criterion" you can tab to "select outcome" and press enter to pop up a dialog. However only the Ok and Cancel buttons can be reached using the keyboard. You currently need to use the mouse to select an outcome.

            Show
            Andrew Davis added a comment - - edited javascript uses click handlers without also listening for keyboard events Can you please be more specific? I'm not sure where I need to also listen to keyboard events. Damyon can provide more details but what I believe he means is that its important for accessibility that a user be able to use moodle with just the keyboard. It should be possible to tab around the interface and activate things without use of the mouse. I'm not sure if its the most directly applicable example to what needs doing for this issue but if you look in lib/yui/build/moodle-core-dock/moodle-core-dock.js you can see Javascript that makes it possible to use the dock menu via the keyboard. In terms of where in the outcomes code he is talking about I believe he is referring to /grade/grading/form/rubric/js/rubriceditor.js. Its used if you set up an assignment, set it to rubric and choose to define a new rubric. When you move focus to "click to edit criterion" you can tab to "select outcome" and press enter to pop up a dialog. However only the Ok and Cancel buttons can be reached using the keyboard. You currently need to use the mouse to select an outcome.
            Hide
            Andrew Davis added a comment -

            Have you been able to rebase this onto master?

            I'd encourage you to make the ongoing work public as much as possible. I'm certainly happy to try and pitch in and help if possible but I'll need access to reasonably up to date code to do that.

            Show
            Andrew Davis added a comment - Have you been able to rebase this onto master? I'd encourage you to make the ongoing work public as much as possible. I'm certainly happy to try and pitch in and help if possible but I'll need access to reasonably up to date code to do that.
            Hide
            Martin Dougiamas added a comment -

            Related policy bug MDL-41535 about the necessity of a non-Javascript fallback should be dealt with soon.

            Show
            Martin Dougiamas added a comment - Related policy bug MDL-41535 about the necessity of a non-Javascript fallback should be dealt with soon.
            Hide
            Kris Stokking added a comment -

            Have you been able to rebase this onto master?

            That's currently in progress as our top priority, we'll post the new changes as soon as they are ready.

            Show
            Kris Stokking added a comment - Have you been able to rebase this onto master? That's currently in progress as our top priority, we'll post the new changes as soon as they are ready.
            Hide
            Mark Nielsen added a comment -

            I have added a few more commits to the branch. Interestingly, our QA just found some accessibility issues with a few of the UIs so I fixed some tabindex values and added some key listeners for enter keys. This should make the JS UIs more accessible without the use of a screen reader (I say without because it was working fine with a screen reader like VoiceOver).

            Today I'll be doing the rebase and going through the list of requested changes. I'll post when I have some solid progress.

            Show
            Mark Nielsen added a comment - I have added a few more commits to the branch. Interestingly, our QA just found some accessibility issues with a few of the UIs so I fixed some tabindex values and added some key listeners for enter keys. This should make the JS UIs more accessible without the use of a screen reader (I say without because it was working fine with a screen reader like VoiceOver). Today I'll be doing the rebase and going through the list of requested changes. I'll post when I have some solid progress.
            Hide
            Andrew Davis added a comment -

            Sorry to bug you. How is the rebase going?

            Show
            Andrew Davis added a comment - Sorry to bug you. How is the rebase going?
            Hide
            Andrew Davis added a comment -

            Regarding this request from Damyon.

            >I selected an outcome set and clicked OK, I got a warning telling me to click "Add" then "OK". This is annoying - just do it for me please.

            Here is a commit with a single string change that makes the reason for that check (as I understand it) more obvious. That may be adequate to alleviate Damyon's concern. Pull in this change if you think its a good idea.

            https://github.com/andyjdavis/moodle/commit/8bc35f8850c06a872a7b94652fbb6bbc13720d98

            Show
            Andrew Davis added a comment - Regarding this request from Damyon. >I selected an outcome set and clicked OK, I got a warning telling me to click "Add" then "OK". This is annoying - just do it for me please. Here is a commit with a single string change that makes the reason for that check (as I understand it) more obvious. That may be adequate to alleviate Damyon's concern. Pull in this change if you think its a good idea. https://github.com/andyjdavis/moodle/commit/8bc35f8850c06a872a7b94652fbb6bbc13720d98
            Hide
            Martin Dougiamas added a comment -

            Ping to Mark/Kris ... anxiously awaiting your progress on this one as time is getting short now.

            Show
            Martin Dougiamas added a comment - Ping to Mark/Kris ... anxiously awaiting your progress on this one as time is getting short now.
            Hide
            Martin Dougiamas added a comment -

            Damyon Wiese can you answer the questions above soon so as not to hold up MR devs much?

            Show
            Martin Dougiamas added a comment - Damyon Wiese can you answer the questions above soon so as not to hold up MR devs much?
            Hide
            Mark Nielsen added a comment - - edited

            Here is an update!

            Latest code can be found here, which is based on Moodle master: https://github.com/moodlerooms/moodle/compare/MDL-40230_outcomes

            Major things left to do:

            • Squish the commits. Please ignore the commit messages besides the first. I'm going to be trying to backport some of these changes to our Moodle so we don't diverge too much.
            • Migrate the rest of tool_outcome to core. All that remains are the install of the db tables and some version bumps. Done
            • JSDoc and converting YUI modules to Shifter. Done
            • Refactor modals to use the core base panel. Done
            • Fix deprecated code. This is events, etc.
            • Add new plugins to standard_plugin_list Done.
            • Some RTL testing/fixes. Done
            Show
            Mark Nielsen added a comment - - edited Here is an update! Latest code can be found here, which is based on Moodle master: https://github.com/moodlerooms/moodle/compare/MDL-40230_outcomes Major things left to do: Squish the commits. Please ignore the commit messages besides the first. I'm going to be trying to backport some of these changes to our Moodle so we don't diverge too much. Migrate the rest of tool_outcome to core. All that remains are the install of the db tables and some version bumps. Done JSDoc and converting YUI modules to Shifter. Done Refactor modals to use the core base panel. Done Fix deprecated code. This is events, etc. Add new plugins to standard_plugin_list Done. Some RTL testing/fixes. Done
            Hide
            Damyon Wiese added a comment -

            Response to Kris: #comment-242691

            Re: Icons - I'll leave the feedback on the icons for Barbara to discuss.

            Re: Simple outcome set by default. I agree for US schools that should be using defined outcome sets this makes sense - but I still think that the complexity will be a barrier for people in other situations and it is not as simple to define the outcomes as the legacy version. This is definitely not a blocker for integration - it's just an opinion.

            Legacy outcomes - I know that you don't want to force this new thing on people that just want the old thing - but IMO having two systems in core is completely confusing for users, and we have to maintain both. I would vote for sooner the better to completely remove the old system. We should even consider putting warnings on all the admin pages for the old ones to mark it as legacy.

            Response to Mark: #comment-242714

            Comments: No - those comments are a point of contention - but they are fine for now and are not the ones I was talking about. I was talking about missing full stops and capitals. (And missing phpdocs in lots of places). moodlecheck and codechecker will warn you about these - please use them and fix any introduced warnings.

            Re: Hooks - adding the calls to outcomes in the course_created/course_updated functions is OK - but for master IMO it would be nice to just observe the events. I was just concerned because I think there has been cases when people assume the rest of the code in Moodle is doing the right thing and using the API always - in this case I think it is safe.

            Re: a module can only have one outcome area
            As long as we are not preventing ourselves from improving this later then it's fine by me.

            "I'm not sure where I need to also listen to keyboard events."

            • Everywhere you are listening for clicks. All interfaces need to be 100% keyboard navigable.

            e.g.
            + this.get(BOX).delegate('click', this._handle_select_outcomes, LINK_ADD, this);
            + this.get(BOX).delegate('click', this._handle_remove_outcome, LINK_DELETE, this);
            This will find all clicks that you can check for keyboard: "git diff origin/MOODLE_24_STABLE |grep click -C 5"

            Yes we are doing yuidoc for new js code.

            Yes we are looking for docs on everything - even if it is obvious.

            Re: singletons
            I just don't like complicated interfaces for things that are simple (e.g. a factory that always returns the same classes with no clear need to ever return different classes). Have a look with the autoloading in 2.6 and see if you think there is a better way - this is easy to change before this is integrated and hard to change it later (Factories are useful if the e.g. return different instances of a class based on config).

            Response to Andrew: #comment-243530

            Thanks for the suggestion - but I actually think it should just work with either button - the chance doing something wrong is pretty low IMO. It is an experience thing - you know what I wanted to do - instead of doing it - you gave me a warning and made me tell you again.

            2 additional comments:

            outcomes/classes/test is an unusual location for tests - the preferred location would be outcomes/tests

            It looks like MDL-41535 is not going to fly based on the comments from Justin - which means this will need non-js fallbacks.

            Show
            Damyon Wiese added a comment - Response to Kris: #comment-242691 Re: Icons - I'll leave the feedback on the icons for Barbara to discuss. Re: Simple outcome set by default. I agree for US schools that should be using defined outcome sets this makes sense - but I still think that the complexity will be a barrier for people in other situations and it is not as simple to define the outcomes as the legacy version. This is definitely not a blocker for integration - it's just an opinion. Legacy outcomes - I know that you don't want to force this new thing on people that just want the old thing - but IMO having two systems in core is completely confusing for users, and we have to maintain both. I would vote for sooner the better to completely remove the old system. We should even consider putting warnings on all the admin pages for the old ones to mark it as legacy. Response to Mark: #comment-242714 Comments: No - those comments are a point of contention - but they are fine for now and are not the ones I was talking about. I was talking about missing full stops and capitals. (And missing phpdocs in lots of places). moodlecheck and codechecker will warn you about these - please use them and fix any introduced warnings. Re: Hooks - adding the calls to outcomes in the course_created/course_updated functions is OK - but for master IMO it would be nice to just observe the events. I was just concerned because I think there has been cases when people assume the rest of the code in Moodle is doing the right thing and using the API always - in this case I think it is safe. Re: a module can only have one outcome area As long as we are not preventing ourselves from improving this later then it's fine by me. "I'm not sure where I need to also listen to keyboard events." Everywhere you are listening for clicks. All interfaces need to be 100% keyboard navigable. e.g. + this.get(BOX).delegate('click', this._handle_select_outcomes, LINK_ADD, this); + this.get(BOX).delegate('click', this._handle_remove_outcome, LINK_DELETE, this); This will find all clicks that you can check for keyboard: "git diff origin/MOODLE_24_STABLE |grep click -C 5" Yes we are doing yuidoc for new js code. Yes we are looking for docs on everything - even if it is obvious. Re: singletons I just don't like complicated interfaces for things that are simple (e.g. a factory that always returns the same classes with no clear need to ever return different classes). Have a look with the autoloading in 2.6 and see if you think there is a better way - this is easy to change before this is integrated and hard to change it later (Factories are useful if the e.g. return different instances of a class based on config). Response to Andrew: #comment-243530 Thanks for the suggestion - but I actually think it should just work with either button - the chance doing something wrong is pretty low IMO. It is an experience thing - you know what I wanted to do - instead of doing it - you gave me a warning and made me tell you again. 2 additional comments: outcomes/classes/test is an unusual location for tests - the preferred location would be outcomes/tests It looks like MDL-41535 is not going to fly based on the comments from Justin - which means this will need non-js fallbacks.
            Hide
            Andrew Davis added a comment -

            Im doing some RTL testing now. I'll post anything I find.

            Show
            Andrew Davis added a comment - Im doing some RTL testing now. I'll post anything I find.
            Hide
            Andrew Davis added a comment - - edited

            Something odd is happening that appears to be limited to this branch. It doesn't happen in the main master branch. When you go to the course settings page the page scroll bar disappears. Its there for a second or two then vanishes.

            I can still scroll using the up/down keys on the keyboard but the scroll wheel on the mouse doesn't work.

            This happens both LTR and RTL.

            Show
            Andrew Davis added a comment - - edited Something odd is happening that appears to be limited to this branch. It doesn't happen in the main master branch. When you go to the course settings page the page scroll bar disappears. Its there for a second or two then vanishes. I can still scroll using the up/down keys on the keyboard but the scroll wheel on the mouse doesn't work. This happens both LTR and RTL.
            Hide
            Mark Nielsen added a comment -

            Yeah, I just found that one Andrew. It has to do with the core dialog moodle-core-notification-dialogue YUI module and that both outcomes and file manager have it as a dependency. You will see that a no-scrolling class is added to the body tag. This is done by the dialogue. I haven't figure out how to fix it just yet, but I have noticed that if you remove either the outcome form element or the file manager form element, then the page scrolls again. Gotta love JS!

            Show
            Mark Nielsen added a comment - Yeah, I just found that one Andrew. It has to do with the core dialog moodle-core-notification-dialogue YUI module and that both outcomes and file manager have it as a dependency. You will see that a no-scrolling class is added to the body tag. This is done by the dialogue. I haven't figure out how to fix it just yet, but I have noticed that if you remove either the outcome form element or the file manager form element, then the page scrolls again. Gotta love JS!
            Hide
            Andrew Davis added a comment -

            RTL looks pretty good. The only funny look thing I encountered was when adding a new outcome to a new outcome set.
            Here is the LTR https://tracker.moodle.org/secure/attachment/34516/AddOutcomeLTR.png
            Here is the RTL https://tracker.moodle.org/secure/attachment/34517/AddOutcomeRTL.png

            Simply switching from divs to spans appears to correct this although I can't be sure what other side effects that will have. I used firebug to turn the first fitemtitle and felement into spans and it made the element label move back to the same line as the text box.

            <div id="fitem_id_outcome_idnumber" class="fitem fitem_ftext">
            <span class="fitemtitle">
            <span class="felement ftext" id="yui_3_9_1_2_1378871354789_574">
            </div>
            <div id="fitem_id_outcome_docnum" class="fitem fitem_ftext">
            <div class="fitemtitle">
            <div class="felement ftext">
            </div>

            After it is the second and unmodified fitemtitle and felement just for the sake of comparison.

            Show
            Andrew Davis added a comment - RTL looks pretty good. The only funny look thing I encountered was when adding a new outcome to a new outcome set. Here is the LTR https://tracker.moodle.org/secure/attachment/34516/AddOutcomeLTR.png Here is the RTL https://tracker.moodle.org/secure/attachment/34517/AddOutcomeRTL.png Simply switching from divs to spans appears to correct this although I can't be sure what other side effects that will have. I used firebug to turn the first fitemtitle and felement into spans and it made the element label move back to the same line as the text box. <div id="fitem_id_outcome_idnumber" class="fitem fitem_ftext"> <span class="fitemtitle"> <span class="felement ftext" id="yui_3_9_1_2_1378871354789_574"> </div> <div id="fitem_id_outcome_docnum" class="fitem fitem_ftext"> <div class="fitemtitle"> <div class="felement ftext"> </div> After it is the second and unmodified fitemtitle and felement just for the sake of comparison.
            Hide
            Kris Stokking added a comment -

            Barbara Ramiro - I've uploaded the new icons (monochrome, in SVG and PNG format) for you

            Show
            Kris Stokking added a comment - Barbara Ramiro - I've uploaded the new icons (monochrome, in SVG and PNG format) for you
            Hide
            Mark Nielsen added a comment -

            Hey Damyon Wiese,

            Thanks for the reviews/comments thus far, really appreciated.

            Re: singletons
            I just don't like complicated interfaces for things that are simple (e.g. a factory that always returns the same classes with no clear need to ever return different classes). Have a look with the autoloading in 2.6 and see if you think there is a better way - this is easy to change before this is integrated and hard to change it later (Factories are useful if the e.g. return different instances of a class based on config).

            Can you please take a look at the latest code? I have changed it slightly and instead of those functions, I'm using \core_outcome\service. This is functionally equivalent as before though. I agree with your comments, unnecessary layers just make it harder to use/understand. The goal of the \core_outcome\service class is not to be a factory, but to provide a single interface to all of the public API for outcomes. This is basically a contract with the rest of Moodle where it is known that backwards compatibility should be maintained unless there are strong and compelling reasons. Also, it helps to hide some of the internals of outcomes so those internals can be refactored freely if necessary. Lastly, it also helps to hide how those service classes are built, which right now doesn't matter much because they are horribly simple (And only simple due to their constructors building their dependencies), but if I needed to share a resource between those services, I could do so easily because I still control their creation.

            If you still feel like this is a bad idea for Moodle, then I can remove \core_outcome\service and just create the service instances where they are used throughout Moodle.

            Cheers, Mark

            Show
            Mark Nielsen added a comment - Hey Damyon Wiese , Thanks for the reviews/comments thus far, really appreciated. Re: singletons I just don't like complicated interfaces for things that are simple (e.g. a factory that always returns the same classes with no clear need to ever return different classes). Have a look with the autoloading in 2.6 and see if you think there is a better way - this is easy to change before this is integrated and hard to change it later (Factories are useful if the e.g. return different instances of a class based on config). Can you please take a look at the latest code? I have changed it slightly and instead of those functions, I'm using \core_outcome\service. This is functionally equivalent as before though. I agree with your comments, unnecessary layers just make it harder to use/understand. The goal of the \core_outcome\service class is not to be a factory, but to provide a single interface to all of the public API for outcomes. This is basically a contract with the rest of Moodle where it is known that backwards compatibility should be maintained unless there are strong and compelling reasons. Also, it helps to hide some of the internals of outcomes so those internals can be refactored freely if necessary. Lastly, it also helps to hide how those service classes are built, which right now doesn't matter much because they are horribly simple (And only simple due to their constructors building their dependencies), but if I needed to share a resource between those services, I could do so easily because I still control their creation. If you still feel like this is a bad idea for Moodle, then I can remove \core_outcome\service and just create the service instances where they are used throughout Moodle. Cheers, Mark
            Hide
            Mark Nielsen added a comment -

            Another update to https://github.com/moodlerooms/moodle/compare/MDL-40230_outcomes

            Please refer to the to do list in prior comment.

            Couple of notes:

            • I rebased the branch again, so you will have to reset or re-check it out.
            • If you have an existing site with my branch, you will need to re-install Moodle or run this query due to the removal of tool_outcome:

              UPDATE mdl_capabilities SET component = 'moodle' WHERE component = 'tool_outcome';
              

            I'm getting pretty close. Hopefully tomorrow I'll have all the major items wrapped up, but I'm not sure how long it'll take to refactor to the core Dialog class and to remove some deprecated code.

            Show
            Mark Nielsen added a comment - Another update to https://github.com/moodlerooms/moodle/compare/MDL-40230_outcomes Please refer to the to do list in prior comment. Couple of notes: I rebased the branch again, so you will have to reset or re-check it out. If you have an existing site with my branch, you will need to re-install Moodle or run this query due to the removal of tool_outcome: UPDATE mdl_capabilities SET component = 'moodle' WHERE component = 'tool_outcome' ; I'm getting pretty close. Hopefully tomorrow I'll have all the major items wrapped up, but I'm not sure how long it'll take to refactor to the core Dialog class and to remove some deprecated code.
            Hide
            Andrew Davis added a comment - - edited

            Hi. Grabbed the latest branch and got an error during upgrade. Trying to figure out whats going on now.

            Error writing to database
             
            More information about this error
            Debug info: ERROR: duplicate key value violates unique constraint "outcomes2_capa_nam_uix"
            DETAIL: Key (name)=(moodle/outcome:mapoutcomes) already exists.
            INSERT INTO outcomes2_capabilities (name,captype,contextlevel,component,riskbitmask) VALUES($1,$2,$3,$4,$5)
            [array (
            'name' => 'moodle/outcome:mapoutcomes',
            'captype' => 'write',
            'contextlevel' => 10,
            'component' => 'moodle',
            'riskbitmask' => 0,
            )]
            Error code: dmlwriteexception
            Stack trace:
             
                line 439 of /lib/dml/moodle_database.php: dml_write_exception thrown
                line 239 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
                line 848 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
                line 900 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->insert_record_raw()
                line 2719 of /lib/accesslib.php: call to pgsql_native_moodle_database->insert_record()
                line 1551 of /lib/upgradelib.php: call to update_capabilities()
                line 341 of /admin/index.php: call to upgrade_core()
            

            Fyi, the database prefix for this install is "outcomes2_".

            UPDATE: I've raised MDL-41770 as Im not sure this is an error being directly caused by the outcomes code. Unless you have evidence to the contrary leave it be and it can be dealt with in that separate issue.

            UPDATE 2: I may possibly be an idiot. Does this (https://tracker.moodle.org/browse/MDL-40230?focusedCommentId=244053&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-244053) explain the error?

            Show
            Andrew Davis added a comment - - edited Hi. Grabbed the latest branch and got an error during upgrade. Trying to figure out whats going on now. Error writing to database   More information about this error Debug info: ERROR: duplicate key value violates unique constraint "outcomes2_capa_nam_uix" DETAIL: Key (name)=(moodle/outcome:mapoutcomes) already exists. INSERT INTO outcomes2_capabilities (name,captype,contextlevel,component,riskbitmask) VALUES($1,$2,$3,$4,$5) [array ( 'name' => 'moodle/outcome:mapoutcomes', 'captype' => 'write', 'contextlevel' => 10, 'component' => 'moodle', 'riskbitmask' => 0, )] Error code: dmlwriteexception Stack trace:   line 439 of /lib/dml/moodle_database.php: dml_write_exception thrown line 239 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end() line 848 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end() line 900 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->insert_record_raw() line 2719 of /lib/accesslib.php: call to pgsql_native_moodle_database->insert_record() line 1551 of /lib/upgradelib.php: call to update_capabilities() line 341 of /admin/index.php: call to upgrade_core() Fyi, the database prefix for this install is "outcomes2_". UPDATE: I've raised MDL-41770 as Im not sure this is an error being directly caused by the outcomes code. Unless you have evidence to the contrary leave it be and it can be dealt with in that separate issue. UPDATE 2: I may possibly be an idiot. Does this ( https://tracker.moodle.org/browse/MDL-40230?focusedCommentId=244053&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-244053 ) explain the error?
            Hide
            Andrew Davis added a comment -

            I am indeed an idiot. I've closed MDL-41770. Move along, nothing to see here.

            Show
            Andrew Davis added a comment - I am indeed an idiot. I've closed MDL-41770 . Move along, nothing to see here.
            Hide
            Andrew Davis added a comment -

            I've just noticed two instances of the same problem. Minor. If you don't have time now we can totally open a new MDL to be done after this issue is integrated.

            On the Activity and Performance report you can click on "Associated content" and on the Coverage report you can click on the number in the activities column to pop up a dialogue that lists relevant activities.

            1) There's no way to click through to those activities.

            2) If you right click the link that opens the dialogue and select "open new tab" you just get the same page.

            Show
            Andrew Davis added a comment - I've just noticed two instances of the same problem. Minor. If you don't have time now we can totally open a new MDL to be done after this issue is integrated. On the Activity and Performance report you can click on "Associated content" and on the Coverage report you can click on the number in the activities column to pop up a dialogue that lists relevant activities. 1) There's no way to click through to those activities. 2) If you right click the link that opens the dialogue and select "open new tab" you just get the same page.
            Hide
            Barbara Ramiro added a comment -

            Hi Jason, i need to see the icons in context to better answer your inquiry. But for the mean time, based on your descriptions, they are neither activity nor action icons, and that they are informative icons, which purpose is the same with the block icons which means it should be 16x16px.

            Show
            Barbara Ramiro added a comment - Hi Jason, i need to see the icons in context to better answer your inquiry. But for the mean time, based on your descriptions, they are neither activity nor action icons, and that they are informative icons, which purpose is the same with the block icons which means it should be 16x16px.
            Hide
            Barbara Ramiro added a comment - - edited

            Hi Kris and Jason, thanks for the icons.

            Here's my feedback:

            1. Dimension should be 16x16 both for svg and png (not 20x20 for svg and 40+x40+ for png) but please refer to my previous comment
            2. Color should be solid #999 (not gradient)
            3. Background should be transparent (not white) and anything white should be transparent
            4. Layers should be compounded to one only (single layer) for better rendering and smaller file size
            5. Objects should contain within the canvas and anything out of canvas should be trimmed
            6. Object on top should have one or two pixel border (good example is the outcome-assessable wherein there is white space that separates the tick mark from the box. The same should be done with the tick box on top of the folder and the front side of the folder to separate it from the back side)

            Cheers (" ,)

            Show
            Barbara Ramiro added a comment - - edited Hi Kris and Jason, thanks for the icons. Here's my feedback: Dimension should be 16x16 both for svg and png (not 20x20 for svg and 40+x40+ for png) but please refer to my previous comment Color should be solid #999 (not gradient) Background should be transparent (not white) and anything white should be transparent Layers should be compounded to one only (single layer) for better rendering and smaller file size Objects should contain within the canvas and anything out of canvas should be trimmed Object on top should have one or two pixel border (good example is the outcome-assessable wherein there is white space that separates the tick mark from the box. The same should be done with the tick box on top of the folder and the front side of the folder to separate it from the back side) Cheers (" ,)
            Hide
            Mark Nielsen added a comment - - edited

            Another update to https://github.com/moodlerooms/moodle/compare/MDL-40230_outcomes

            Latest update includes conversion to core dialogue for the modals. Last thing to do is to find and replace deprecated code. We are getting very close!

            Show
            Mark Nielsen added a comment - - edited Another update to https://github.com/moodlerooms/moodle/compare/MDL-40230_outcomes Latest update includes conversion to core dialogue for the modals. Last thing to do is to find and replace deprecated code. We are getting very close!
            Hide
            Andrew Davis added a comment -

            Hi Mark. Something has gone wrong with the add outcome dialogue. https://tracker.moodle.org/secure/attachment/34589/addoutcome.png There's no JS error.

            Show
            Andrew Davis added a comment - Hi Mark. Something has gone wrong with the add outcome dialogue. https://tracker.moodle.org/secure/attachment/34589/addoutcome.png There's no JS error.
            Hide
            Andrew Davis added a comment - - edited

            Also, on the course settings page if I click "Select outcome sets" I'm just moved back to the top of the page. No dialogue is displayed. No JS errors.

            This may be because I currently have no outcome sets defined. I deleted all of them and encountered the problem in my previous comment while trying to recreate them.

            The activity settings page behaves the same way.

            Show
            Andrew Davis added a comment - - edited Also, on the course settings page if I click "Select outcome sets" I'm just moved back to the top of the page. No dialogue is displayed. No JS errors. This may be because I currently have no outcome sets defined. I deleted all of them and encountered the problem in my previous comment while trying to recreate them. The activity settings page behaves the same way.
            Hide
            Andrew Davis added a comment -

            I'm not sure how important it is to getting this integrated but it appears that new code should not be directly calling add_to_log(). Instead it should be triggering an event which will cause the logging plus possibly other stuff to happen. http://docs.moodle.org/dev/Event_2

            Although the new event system will be in 2.6 I'm not sure whether or not its a good idea to switch the new outcomes system to use new events system this late in the game.

            Show
            Andrew Davis added a comment - I'm not sure how important it is to getting this integrated but it appears that new code should not be directly calling add_to_log(). Instead it should be triggering an event which will cause the logging plus possibly other stuff to happen. http://docs.moodle.org/dev/Event_2 Although the new event system will be in 2.6 I'm not sure whether or not its a good idea to switch the new outcomes system to use new events system this late in the game.
            Hide
            Tim Hunt added a comment -

            Did it not occur to anyone to ask the quiz and question bank maintainer to review the relevant parts of this?

            Please give me time to review this before it is integrated.

            Show
            Tim Hunt added a comment - Did it not occur to anyone to ask the quiz and question bank maintainer to review the relevant parts of this? Please give me time to review this before it is integrated.
            Hide
            Martin Dougiamas added a comment -

            Ah yes sorry Tim, should have pinged you. Please have a look!

            Show
            Martin Dougiamas added a comment - Ah yes sorry Tim, should have pinged you. Please have a look!
            Hide
            Mark Nielsen added a comment -

            Andrew Davis did you happen to try purging Moodle cache and your browser cache? I'm not seeing these problems.

            Show
            Mark Nielsen added a comment - Andrew Davis did you happen to try purging Moodle cache and your browser cache? I'm not seeing these problems.
            Hide
            Mark Nielsen added a comment - - edited

            Another minor update to https://github.com/moodlerooms/moodle/compare/MDL-40230_outcomes - A minor CSS fix and removed deprecated code from outcome/ directory.

            Regarding other deprecated code, I could use some friendly advice:

            • I added an event, events_trigger('question_edited'...). Should this be converted to new event API and if so, any pointers on how to do this? Note, it looks like Quiz (which I added a listener to 'question_edited') doesn't appear to have migrated to the new event system just yet. So, we might be jumping the gun by having me start it.
            • Andrew brought up add_to_log calls. It doesn't look like these have been deprecated yet, but wondering if I need to convert these and any pointers would be appreciated.

            Other than the above two items, I believe I'm all done. Damyon Wiese, if you could be so kind as to give this another look, that would be fantastic.

            Show
            Mark Nielsen added a comment - - edited Another minor update to https://github.com/moodlerooms/moodle/compare/MDL-40230_outcomes - A minor CSS fix and removed deprecated code from outcome/ directory. Regarding other deprecated code, I could use some friendly advice: I added an event, events_trigger('question_edited'...). Should this be converted to new event API and if so, any pointers on how to do this? Note, it looks like Quiz (which I added a listener to 'question_edited') doesn't appear to have migrated to the new event system just yet. So, we might be jumping the gun by having me start it. Andrew brought up add_to_log calls. It doesn't look like these have been deprecated yet, but wondering if I need to convert these and any pointers would be appreciated. Other than the above two items, I believe I'm all done. Damyon Wiese , if you could be so kind as to give this another look, that would be fantastic.
            Hide
            Andrew Davis added a comment -

            did you happen to try purging Moodle cache and your browser cache?

            Clearing the browser cache did the trick.

            Show
            Andrew Davis added a comment - did you happen to try purging Moodle cache and your browser cache? Clearing the browser cache did the trick.
            Hide
            Andrew Davis added a comment -

            Just gave the latest version a run through and didn't spot anything nasty

            Show
            Andrew Davis added a comment - Just gave the latest version a run through and didn't spot anything nasty
            Hide
            Andrew Davis added a comment -

            I've raised MDL-41825. I mentioned this above previously. Its some small improvements we can consider post integration.

            Show
            Andrew Davis added a comment - I've raised MDL-41825 . I mentioned this above previously. Its some small improvements we can consider post integration.
            Hide
            Ankit Agarwal added a comment -

            Hi Mark,
            Re: events

            1. Yes you will have to change the event to use the new system. We have already deprecated events_trigger(), and there will be no old style events in core in 2.6 release. Regarding quiz events, the issue is in progress - MDL-41039.
            2. We probably won't deprecate add_to_log() in 2.6, however imho we shouldn't really be introducing new add_to_log calls in core.

            To help you convert the code, you can have a read through:-
            http://docs.moodle.org/dev/Event_2
            For code samples and examples, have a look at various issues in MDL-39952

            Thanks

            Show
            Ankit Agarwal added a comment - Hi Mark, Re: events Yes you will have to change the event to use the new system. We have already deprecated events_trigger(), and there will be no old style events in core in 2.6 release. Regarding quiz events, the issue is in progress - MDL-41039 . We probably won't deprecate add_to_log() in 2.6, however imho we shouldn't really be introducing new add_to_log calls in core. To help you convert the code, you can have a read through:- http://docs.moodle.org/dev/Event_2 For code samples and examples, have a look at various issues in MDL-39952 Thanks
            Hide
            Mark Nielsen added a comment -

            https://github.com/moodlerooms/moodle/compare/MDL-40230_outcomes has been updated with the new event API. I had to rebase the branch in order to do this.

            Show
            Mark Nielsen added a comment - https://github.com/moodlerooms/moodle/compare/MDL-40230_outcomes has been updated with the new event API. I had to rebase the branch in order to do this.
            Hide
            Andrew Davis added a comment -

            Unfortunately you'll have to rebase again in another 24-48 hours (once the weeks testing is done and weeklies are rolled).

            Show
            Andrew Davis added a comment - Unfortunately you'll have to rebase again in another 24-48 hours (once the weeks testing is done and weeklies are rolled).
            Hide
            Martin Dougiamas added a comment -

            -1 for "Can we install one simple generic outcome set by default?"

            Show
            Martin Dougiamas added a comment - -1 for "Can we install one simple generic outcome set by default?"
            Hide
            Tim Hunt added a comment - - edited

            Review of qu* bits:

            1. Is it intentional that you have $this->quba->record_outcomes($this->attempt->userid); in process_submitted_actions ? I am trying to remember what we discussed originally, and whether outcomes should only be recorded on submit all and finish, or not. I can't remember what the answer was, but presumably you decided to record outcomes ASAP. Is that right?

            Oh, no. I see that is inside in if state == finished block. Sorry.

            2. It is a bit ugly the number of global $CFGs you have had to add. I suppose if you were stuarting now, you would put things in a class that could be autoloaded, but you did not know what when you started, and it is too late to change now.

            3. It is a but poor that you have to do an extra DB query in code blocks like https://github.com/moodlerooms/moodle/compare/5c8860f...INT-4475_outcomes#L69R83 just to get the qtype name - espcially since similar code is duplicated in several places. Can you make a helper function question_get_outcome_area($questionid, $qtype = null); to hold the repeated code. ($qtype = null so you can save a DB query if the caller knows the qtype.)

            4. quiz_question_edited_handler seems to make the module author do a lot of work, most of which is generic and should be the same for all activities using questions. Surely the only quiz-specific bit is "given this question id, tell me all the quizzes that use it". Therefore, it is core_outcomes that should subscribe to the question_editied event, and it should then call a quiz (or whatever) method to return a list of cmids from the questionid.

            5. Where are the developer docs that tell module developers what they should do, like you have done for the quiz?

            6. Do you not need some code in quiz_delete_attempt?

            7. Oh, I see, there is delete_question_outcome_attempts in question/engine/datalib.php. Isn't that a terribly confusing asymmetry with how they are added? Please re-desing the API to be symmetrical, or it is just confusing.

            8. I am not convinced by your excuse in delete_question_outcome_attempts. To start with, excuses like that are an implementation detail of the method, not part of its external API, so it should be a comment in the method, not part of the PHPdocs.

            Second, you can write the query as

            DELETE FROM {outcome_attempts} WHERE outcomeusedareaid IN (
                    SELECT DISTINCT ua.id
                          FROM {outcome_used_areas} ua
                          JOIN {outcome_areas} area ON area.id = ua.outcomeareaid
                          JOIN {context} ctx ON ctx.instanceid = ua.cmid AND ctx.contextlevel = ?
                     LEFT JOIN {question_attempts} qa ON a.itemid = qa.id 
                         WHERE area.area = ?
                           AND ctx.id $sql
                           AND qa.id IS NULL)
            

            Also, please use table alias ctx for the context table, like everywhere else in this file.

            Argh! you cannot fix ctx.contextlevel = CONTEXT_MODULE here. (can you? If it is right, I don't understand and you need a comment to explain.)

            9. I am pretty sure Moodle coding style says do not use the redundant words INNER and OUTER. If Moodle does not specify, then please follow the convention of the file you are editing, which is no INNER or OUTER. http://docs.moodle.org/dev/SQL_coding_style#General_rules

            10 record_outcomes: @param array|int $userids. What? when will this be an array, when will this be an int, what does it all mean?

            11. foreach ($this->questionattempts as $attempt) - Please change the variable name from $attempt to $qa, like everywhere else in the question code.

            12. The triple condition:

                        if (!$attempt->get_state()->is_finished()) {
                            continue;
                        }
                        if (is_null($attempt->get_mark())) {
                            continue; // Don't record unanswered questions.
                        }
                        $question = $attempt->get_question();
                        if (!$question->qtype->is_real_question_type()) {
                            continue; // Don't record non-questions.
                        } 
            

            is an unnecessary mess. It think if (!$qa->get_state()->is_graded()) is all you need.

            11. https://github.com/moodlerooms/moodle/compare/5c8860f...INT-4475_outcomes#L215R221 - the !empty($question->id) seems to imply that you can only add outcomes when editing an existing question. You cannot add them when creating a new question. If so, isn't that terrible usability?

            13. I don't think you should add just a quesion_edited event, without corresponding question added / deleted events.

            14. "// todo: figure out if this is the correct context" Please fix this before this is integrated.

            When you fix these issues, please develop the changes, in the first instance at least, as new commit(s), so that I can review just them. (You may later rebase, if applicable.)

            Meta comment: so, I have revewed a tiny fraction of this patch, and found 14 issues. Extrapolating that is very worrying. (And, I only saw that this was ready for review by chance. This code was finished two months ago, why did you not ask me for a review then?)

            Show
            Tim Hunt added a comment - - edited Review of qu* bits: 1. Is it intentional that you have $this->quba->record_outcomes($this->attempt->userid); in process_submitted_actions ? I am trying to remember what we discussed originally, and whether outcomes should only be recorded on submit all and finish, or not. I can't remember what the answer was, but presumably you decided to record outcomes ASAP. Is that right? Oh, no. I see that is inside in if state == finished block. Sorry. 2. It is a bit ugly the number of global $CFGs you have had to add. I suppose if you were stuarting now, you would put things in a class that could be autoloaded, but you did not know what when you started, and it is too late to change now. 3. It is a but poor that you have to do an extra DB query in code blocks like https://github.com/moodlerooms/moodle/compare/5c8860f...INT-4475_outcomes#L69R83 just to get the qtype name - espcially since similar code is duplicated in several places. Can you make a helper function question_get_outcome_area($questionid, $qtype = null); to hold the repeated code. ($qtype = null so you can save a DB query if the caller knows the qtype.) 4. quiz_question_edited_handler seems to make the module author do a lot of work, most of which is generic and should be the same for all activities using questions. Surely the only quiz-specific bit is "given this question id, tell me all the quizzes that use it". Therefore, it is core_outcomes that should subscribe to the question_editied event, and it should then call a quiz (or whatever) method to return a list of cmids from the questionid. 5. Where are the developer docs that tell module developers what they should do, like you have done for the quiz? 6. Do you not need some code in quiz_delete_attempt? 7. Oh, I see, there is delete_question_outcome_attempts in question/engine/datalib.php. Isn't that a terribly confusing asymmetry with how they are added? Please re-desing the API to be symmetrical, or it is just confusing. 8. I am not convinced by your excuse in delete_question_outcome_attempts. To start with, excuses like that are an implementation detail of the method, not part of its external API, so it should be a comment in the method, not part of the PHPdocs. Second, you can write the query as DELETE FROM {outcome_attempts} WHERE outcomeusedareaid IN ( SELECT DISTINCT ua.id FROM {outcome_used_areas} ua JOIN {outcome_areas} area ON area.id = ua.outcomeareaid JOIN {context} ctx ON ctx.instanceid = ua.cmid AND ctx.contextlevel = ? LEFT JOIN {question_attempts} qa ON a.itemid = qa.id WHERE area.area = ? AND ctx.id $sql AND qa.id IS NULL ) Also, please use table alias ctx for the context table, like everywhere else in this file. Argh! you cannot fix ctx.contextlevel = CONTEXT_MODULE here. (can you? If it is right, I don't understand and you need a comment to explain.) 9. I am pretty sure Moodle coding style says do not use the redundant words INNER and OUTER. If Moodle does not specify, then please follow the convention of the file you are editing, which is no INNER or OUTER. http://docs.moodle.org/dev/SQL_coding_style#General_rules 10 record_outcomes: @param array|int $userids. What? when will this be an array, when will this be an int, what does it all mean? 11. foreach ($this->questionattempts as $attempt) - Please change the variable name from $attempt to $qa, like everywhere else in the question code. 12. The triple condition: if (!$attempt->get_state()->is_finished()) { continue; } if (is_null($attempt->get_mark())) { continue; // Don't record unanswered questions. } $question = $attempt->get_question(); if (!$question->qtype->is_real_question_type()) { continue; // Don't record non-questions. } is an unnecessary mess. It think if (!$qa->get_state()->is_graded()) is all you need. 11. https://github.com/moodlerooms/moodle/compare/5c8860f...INT-4475_outcomes#L215R221 - the !empty($question->id) seems to imply that you can only add outcomes when editing an existing question. You cannot add them when creating a new question. If so, isn't that terrible usability? 13. I don't think you should add just a quesion_edited event, without corresponding question added / deleted events. 14. "// todo: figure out if this is the correct context" Please fix this before this is integrated. When you fix these issues, please develop the changes, in the first instance at least, as new commit(s), so that I can review just them. (You may later rebase, if applicable.) Meta comment: so, I have revewed a tiny fraction of this patch, and found 14 issues. Extrapolating that is very worrying. (And, I only saw that this was ready for review by chance. This code was finished two months ago, why did you not ask me for a review then?)
            Hide
            Andrew Davis added a comment -

            14. "// todo: figure out if this is the correct context" Please fix this before this is integrated.{quote]
            Sorry to jump on the finding things that need doing bandwagon but this comment prompted me to go and look specifically for todos. I count 5 in the new code. They either need to be resolved (if they haven't been already) and the todo removed or additional MDLs should be opened and the to do note amended to include the issue number. http://docs.moodle.org/dev/Coding_style#Using_TODO

            There's a few functions that have phpdoc parameter descriptions but not function descriptions. For example send_export_file() in outcome/classes/service/export_helper.php process_outcome_area($data) doesn't have any phpdocs. If you ctrl+f for " function " you can flick through quite quickly. That will be quicker than me giving you a list to look up.

            +        $filter = $filterrepo->find_one_by(array(
            +            'outcomesetid' => $mform->get_cached_value('outcomesetid'),
            +            'courseid'     => $COURSE->id,
            +        ));
            +        list($filtersql, $filterparams) = $outcomerepo->filter_to_sql($filter); 
            

            find_by_one() does a db->get_record() to get a database record which it uses to build another query. Admittedly the second query is immense in at least once case so I can understand the desire to avoid making it bigger but it would be great to avoid going to the database twice.

            In course_outcomes_sql() in outcome/classes/coverage/coverage_abstract.php there's some SQL that should be split over several lines just for readability. It also uses find_by_course() to get something just to build a second query. This time its a simple one. Is there any way to avoid this?

            Show
            Andrew Davis added a comment - 14. "// todo: figure out if this is the correct context" Please fix this before this is integrated.{quote] Sorry to jump on the finding things that need doing bandwagon but this comment prompted me to go and look specifically for todos. I count 5 in the new code. They either need to be resolved (if they haven't been already) and the todo removed or additional MDLs should be opened and the to do note amended to include the issue number. http://docs.moodle.org/dev/Coding_style#Using_TODO There's a few functions that have phpdoc parameter descriptions but not function descriptions. For example send_export_file() in outcome/classes/service/export_helper.php process_outcome_area($data) doesn't have any phpdocs. If you ctrl+f for " function " you can flick through quite quickly. That will be quicker than me giving you a list to look up. + $filter = $filterrepo->find_one_by(array( + 'outcomesetid' => $mform->get_cached_value('outcomesetid'), + 'courseid' => $COURSE->id, + )); + list($filtersql, $filterparams) = $outcomerepo->filter_to_sql($filter); find_by_one() does a db->get_record() to get a database record which it uses to build another query. Admittedly the second query is immense in at least once case so I can understand the desire to avoid making it bigger but it would be great to avoid going to the database twice. In course_outcomes_sql() in outcome/classes/coverage/coverage_abstract.php there's some SQL that should be split over several lines just for readability. It also uses find_by_course() to get something just to build a second query. This time its a simple one. Is there any way to avoid this?
            Hide
            Mark Nielsen added a comment -

            Hey Andrew Davis - I updated PHPDocs and comments. In regards to the filters, the filter is required in order to generate the subsequent SQL. $outcomerepo->filter_to_sql($filter) is actually quite complex and cannot be done in SQL. So, yes, we need the double hop to the database.

            Show
            Mark Nielsen added a comment - Hey Andrew Davis - I updated PHPDocs and comments. In regards to the filters, the filter is required in order to generate the subsequent SQL. $outcomerepo->filter_to_sql($filter) is actually quite complex and cannot be done in SQL. So, yes, we need the double hop to the database.
            Hide
            Mark Nielsen added a comment -

            Hello Tim Hunt,

            • For point 3, done, created question_get_outcome_area
            • For point 4, I implemented this the way that you suggested it IIRC. The nice benefit with current implementation is that we don't have to load every activity's lib.php file to see if they have a function defined or not.
            • For points 6 and 7, this seems like an appropriate location to remove question related data, because it isn't directly related to a quiz attempt. Could you provide more concrete suggestions if you still feel otherwise?
            • For point 8, You suggested query doesn't work because outcome_attempts is used in the left join. Implemented the other suggestions though.
            • For point 9, done.
            • For point 10, updated PHPDoc.
            • For point 11, done, renamed.
            • For point 12, done.
            • For the second point 11, done, removed !empty($question->id).
            • For point 13, haven't done yet pending on what happens in Point 4. Also, can be done in another ticket so it doesn't hold this one up.
            • For point 14, can you please verify what context I should be using? I just need to know if the user has that capability.
            • General question, outcomes cannot be mapped to questions that are not real. I'm trying to make an array of question types that are not mappable here. Is there a way to programmatically exclude the random question type instead of hard coding it?

            The above code changes can be found here.

            Show
            Mark Nielsen added a comment - Hello Tim Hunt , For point 3, done, created question_get_outcome_area For point 4, I implemented this the way that you suggested it IIRC. The nice benefit with current implementation is that we don't have to load every activity's lib.php file to see if they have a function defined or not. For points 6 and 7, this seems like an appropriate location to remove question related data, because it isn't directly related to a quiz attempt. Could you provide more concrete suggestions if you still feel otherwise? For point 8, You suggested query doesn't work because outcome_attempts is used in the left join. Implemented the other suggestions though. For point 9, done. For point 10, updated PHPDoc. For point 11, done, renamed. For point 12, done. For the second point 11, done, removed !empty($question->id). For point 13, haven't done yet pending on what happens in Point 4. Also, can be done in another ticket so it doesn't hold this one up. For point 14, can you please verify what context I should be using? I just need to know if the user has that capability. General question, outcomes cannot be mapped to questions that are not real. I'm trying to make an array of question types that are not mappable here . Is there a way to programmatically exclude the random question type instead of hard coding it? The above code changes can be found here .
            Hide
            Tim Hunt added a comment -

            4. Ever activity's lib.php is loaded on every page anyway, because of navigation.

            I don't care what vague hint I gave you months ago. The question is, what is the best code we can write here. You were working on the details of the code. You are better placed to judge what works and what doesn't. We should optimising how nice it is to implement this for activity authors.

            6. & 7. If we can delete automatically like this, why can't we create in an equally nice way?

            8. How about:

            DELETE FROM {outcome_attempts} WHERE outcomeusedareaid IN (
                    SELECT DISTINCT ua.id
                          FROM {outcome_used_areas} ua
                          JOIN {outcome_areas} area ON area.id = ua.outcomeareaid
                          JOIN {context} ctx ON ctx.instanceid = ua.cmid AND ctx.contextlevel = ?
                         WHERE area.area = ?
                           AND ctx.id $sql)
            AND NOT EXISTS (
                     SELECT 1 FROM {question_attempts} qa WHERE qa.id = itemid
            )
            

            14. I don't know. This is your code. What capability? What user? What is going on? If you phrase the question clearly, I may be able to help. (Or you may even work it out yourself.)

            General question: Well, note that any question can be used in a non-graded way. That is, you can add the question to the quiz with a maximum mark of 0.

            But, I suppose what you mean is the way we decide which questions are called 'i' in the quiz navigation, rather than being given a question number. That is done by checking question->length.

            Oh, no. Because random questions don't enter into that, but they are relevant here.

            Sorry, I don't understand what is going on here, please try explaining your question more clearly.

            Show
            Tim Hunt added a comment - 4. Ever activity's lib.php is loaded on every page anyway, because of navigation. I don't care what vague hint I gave you months ago. The question is, what is the best code we can write here. You were working on the details of the code. You are better placed to judge what works and what doesn't. We should optimising how nice it is to implement this for activity authors. 6. & 7. If we can delete automatically like this, why can't we create in an equally nice way? 8. How about: DELETE FROM {outcome_attempts} WHERE outcomeusedareaid IN ( SELECT DISTINCT ua.id FROM {outcome_used_areas} ua JOIN {outcome_areas} area ON area.id = ua.outcomeareaid JOIN {context} ctx ON ctx.instanceid = ua.cmid AND ctx.contextlevel = ? WHERE area.area = ? AND ctx.id $sql) AND NOT EXISTS ( SELECT 1 FROM {question_attempts} qa WHERE qa.id = itemid ) 14. I don't know. This is your code. What capability? What user? What is going on? If you phrase the question clearly, I may be able to help. (Or you may even work it out yourself.) General question: Well, note that any question can be used in a non-graded way. That is, you can add the question to the quiz with a maximum mark of 0. But, I suppose what you mean is the way we decide which questions are called 'i' in the quiz navigation, rather than being given a question number. That is done by checking question->length. Oh, no. Because random questions don't enter into that, but they are relevant here. Sorry, I don't understand what is going on here, please try explaining your question more clearly.
            Hide
            Kris Stokking added a comment -

            I don't care what vague hint I gave you months ago. The question is, what is the best code we can write here. You were working on the details of the code. You are better placed to judge what works and what doesn't. We should optimising how nice it is to implement this for activity authors.

            Surely you can see how ridiculous this sounds Tim. We reached out to you early on in the development process to make sure we came up with the best design possible, so there's no reason to turn around and blast us for it when it's been implemented as we discussed - regardless of how you feel now. And there's also no reason to spread FUD with your "extrapolation" when several of the 14 issues are comments directed at yourself and the majority of the others were addressed within the same day.

            We're perfectly willing to work with you on improving the design, but we're not going to tolerate disrespect. If you have issues with the design, feel free to ping us on Skype and give us the same courtesy as we had done with you.

            Show
            Kris Stokking added a comment - I don't care what vague hint I gave you months ago. The question is, what is the best code we can write here. You were working on the details of the code. You are better placed to judge what works and what doesn't. We should optimising how nice it is to implement this for activity authors. Surely you can see how ridiculous this sounds Tim. We reached out to you early on in the development process to make sure we came up with the best design possible, so there's no reason to turn around and blast us for it when it's been implemented as we discussed - regardless of how you feel now. And there's also no reason to spread FUD with your "extrapolation" when several of the 14 issues are comments directed at yourself and the majority of the others were addressed within the same day. We're perfectly willing to work with you on improving the design, but we're not going to tolerate disrespect. If you have issues with the design, feel free to ping us on Skype and give us the same courtesy as we had done with you.
            Hide
            Jason Fowler added a comment -

            There is talk about redeveloping the navigation system, so it is more performance friendly, and has a different UX. Pointing to the current navigation as a a reason why every lib.php can be loaded without performance issues is a mistake. Code changes. Just because it is that way now, doesn't mean it will be that way in the next version of Moodle. I think the current method of NOT loading every lib.php is a good one. It allows for the plans to improve navigations performance to happen without additional costs elsewhere.

            Show
            Jason Fowler added a comment - There is talk about redeveloping the navigation system, so it is more performance friendly, and has a different UX. Pointing to the current navigation as a a reason why every lib.php can be loaded without performance issues is a mistake. Code changes. Just because it is that way now, doesn't mean it will be that way in the next version of Moodle. I think the current method of NOT loading every lib.php is a good one. It allows for the plans to improve navigations performance to happen without additional costs elsewhere.
            Hide
            Mark Nelson added a comment - - edited

            I am sure when people receive an email that Mark Nelson commented on this issue, they will assume it was Mark Nielsen. Sadly, that is not the case.

            Anyway, great work Mark. Thanks for doing this huge task. It will certainly be a welcome addition to Moodle.

            General comment.
            1. Comments should start with a capital and end with a full-stop. I just noticed a few when looking for the event code. This is obviously trivial and not a priority, I just thought I would point it out.
            Events/Logging.
            1. The name of the event should use the term 'updated', not 'edited', see lib/classes/event to see the current names used. The same applies for the description.
            2. In mod/quiz/db/events.php you have added handlerfunction, which already exists, so it is now repeated twice.
            3. I don't think there is a reason to set 'internal' to false here, or is there? I think you can simply remove this and leave the default (which is true).
            4. You use "@copyright 1999 onwards Martin Dougiamas .." for the new file, surely you want to credit Moodlerooms. Martin can't get all the fame after all.
            5. In question/question.php you could probably add some other data to the other field, like the question name (assuming it is available w/o a database call).
            6. Usually I would say that you should not be adding new add_to_log calls, as we will no longer be introducing them in core from now on, but using an event class instead. However, this close to the release I do not think this is required, as we probably won't have all the existing add_to_log calls replaced any ways.
            7. It would be nice if Andrew was able to confirm the event trigger is working as expected.

            Regards,

            Mark

            Show
            Mark Nelson added a comment - - edited I am sure when people receive an email that Mark Nelson commented on this issue, they will assume it was Mark Nielsen. Sadly, that is not the case. Anyway, great work Mark. Thanks for doing this huge task. It will certainly be a welcome addition to Moodle. General comment. Comments should start with a capital and end with a full-stop. I just noticed a few when looking for the event code. This is obviously trivial and not a priority, I just thought I would point it out. Events/Logging. The name of the event should use the term 'updated', not 'edited', see lib/classes/event to see the current names used. The same applies for the description. In mod/quiz/db/events.php you have added handlerfunction, which already exists, so it is now repeated twice. I don't think there is a reason to set 'internal' to false here, or is there? I think you can simply remove this and leave the default (which is true). You use "@copyright 1999 onwards Martin Dougiamas .." for the new file, surely you want to credit Moodlerooms. Martin can't get all the fame after all. In question/question.php you could probably add some other data to the other field, like the question name (assuming it is available w/o a database call). Usually I would say that you should not be adding new add_to_log calls, as we will no longer be introducing them in core from now on, but using an event class instead. However, this close to the release I do not think this is required, as we probably won't have all the existing add_to_log calls replaced any ways. It would be nice if Andrew was able to confirm the event trigger is working as expected. Regards, Mark
            Hide
            Damyon Wiese added a comment -

            Looking at the updated branch.

            Thanks Mark - heaps of work has gone into this and it is looking much improved.

            List of comments:

            1. Focus loops in dialogue.js - yes this is a good idea and Rosie has been working on it in MDL-35926. The solution you have here has a number of issues (doesn't handle shift tab, creates a focusable, empty div with no other purpose), so we should go with the other version IMO. Same goes for setting the focus when the dialogue is shown.

            2. outcome/classes/service.php - IMO - its simple - but is still overkill. Your arguments for keeping it would apply to any class anywhere in Moodle - and if we did this all over, it would a) impact performance, b) be hard to debug when you get a class you weren't expecting. Also - any state in your singletons would not be reset e.g. for phpunit tests (always a danger with static caching). If you really want to keep it this way - you are encouraged to argue the point (it's healthy).

            3. Oneline commit messages should all have "MDL-40230 Outcomes: blah". E.g. "MDL-40230: BACKPORT Remove @author tags" does not say what area it affects.

            4. I see you have already made changes from the suggestions from Tim above - thanks. You have been very responsive about making changes on this huge issue - thankyou.

            5. The change to moodleform::get_form_identifier looks like an important fix for an autoloaded form (why does php have such sucky syntax?). Maybe this could be split into it's own commit for the history.

            6. API design - we don't do this much yet - but - instead of requiring changes in each activity module for things like resetting a course. It would be great if the module would trigger an event and outcomes would observe it and react accordingly.

            mod_assign comments:

            Why does update_outcome_attempts belong in the activity module and not in the gradebook? Assignment already has detailed logic around when to push the grades to the gradebook and outcomes should probably follow the same logic. The way it is now, you have grades being pushed to outcomes, but not the gradebook if blind marking is enabled - this sounds incorrect to me.

            Other minor ones:

            1. Some js functions could do with proper yuidocs
            (grade/grading/form/rubric/js/rubriceditor.js)

            2. code checker Warnings from comments with no trailing punctuation (A few places - here are some examples: grade/grading/form/rubric/js/rubriceditor.js, backup/moodle2/backup_plan_builder.class.php)

            3. Random capitalisation in lang string files. http://docs.moodle.org/dev/Coding_style#Capitals

            4. I think Mark is commenting on add_to_log now.

            5. Multiple @package tags in phpdocs on MoodleQuickForm_map_outcome. We will not allow multiple package tags - in this case it should be just outcomes IMO.

            Other comments - the scrolling thing is a regression in master that should be fixed by MDL-41188.

            Non-js fallbacks: JS is still required for all the outcomes interfaces in this patch. We would like to drop this requirement, but it does not appear that is going to happen (MDL-41535).

            Thanks again, Damyon

            Show
            Damyon Wiese added a comment - Looking at the updated branch. Thanks Mark - heaps of work has gone into this and it is looking much improved. List of comments: 1. Focus loops in dialogue.js - yes this is a good idea and Rosie has been working on it in MDL-35926 . The solution you have here has a number of issues (doesn't handle shift tab, creates a focusable, empty div with no other purpose), so we should go with the other version IMO. Same goes for setting the focus when the dialogue is shown. 2. outcome/classes/service.php - IMO - its simple - but is still overkill. Your arguments for keeping it would apply to any class anywhere in Moodle - and if we did this all over, it would a) impact performance, b) be hard to debug when you get a class you weren't expecting. Also - any state in your singletons would not be reset e.g. for phpunit tests (always a danger with static caching). If you really want to keep it this way - you are encouraged to argue the point (it's healthy). 3. Oneline commit messages should all have " MDL-40230 Outcomes: blah". E.g. " MDL-40230 : BACKPORT Remove @author tags" does not say what area it affects. 4. I see you have already made changes from the suggestions from Tim above - thanks. You have been very responsive about making changes on this huge issue - thankyou. 5. The change to moodleform::get_form_identifier looks like an important fix for an autoloaded form (why does php have such sucky syntax?). Maybe this could be split into it's own commit for the history. 6. API design - we don't do this much yet - but - instead of requiring changes in each activity module for things like resetting a course. It would be great if the module would trigger an event and outcomes would observe it and react accordingly. mod_assign comments: Why does update_outcome_attempts belong in the activity module and not in the gradebook? Assignment already has detailed logic around when to push the grades to the gradebook and outcomes should probably follow the same logic. The way it is now, you have grades being pushed to outcomes, but not the gradebook if blind marking is enabled - this sounds incorrect to me. Other minor ones: 1. Some js functions could do with proper yuidocs (grade/grading/form/rubric/js/rubriceditor.js) 2. code checker Warnings from comments with no trailing punctuation (A few places - here are some examples: grade/grading/form/rubric/js/rubriceditor.js, backup/moodle2/backup_plan_builder.class.php) 3. Random capitalisation in lang string files. http://docs.moodle.org/dev/Coding_style#Capitals 4. I think Mark is commenting on add_to_log now. 5. Multiple @package tags in phpdocs on MoodleQuickForm_map_outcome. We will not allow multiple package tags - in this case it should be just outcomes IMO. Other comments - the scrolling thing is a regression in master that should be fixed by MDL-41188 . Non-js fallbacks: JS is still required for all the outcomes interfaces in this patch. We would like to drop this requirement, but it does not appear that is going to happen ( MDL-41535 ). Thanks again, Damyon
            Hide
            Jason Fowler added a comment -

            Mark, as a frontend team member, Andrew was supposed to be reviewing the interface more than the functioning code itself. From the description:

            The rough peer review process will be as follows:
            1) FRONTEND team will review the UI for basic usability and process.
            2) Then, BACKEND team will review the code for structure and general coding.
            3) Then, someone from integration will do a more detailed review for final integration.

            Show
            Jason Fowler added a comment - Mark, as a frontend team member, Andrew was supposed to be reviewing the interface more than the functioning code itself. From the description: The rough peer review process will be as follows: 1) FRONTEND team will review the UI for basic usability and process. 2) Then, BACKEND team will review the code for structure and general coding. 3) Then, someone from integration will do a more detailed review for final integration.
            Hide
            Martin Dougiamas added a comment -

            Guys, let's try to keep Moodle HQ feedback streamlined for the main developers, and not have too many FRONTEND/BACKEND people posting random thoughts to this issue. I suggest anyone at HQ who is reviewing get together in the office and pool your lists together before posting.

            Show
            Martin Dougiamas added a comment - Guys, let's try to keep Moodle HQ feedback streamlined for the main developers, and not have too many FRONTEND/BACKEND people posting random thoughts to this issue. I suggest anyone at HQ who is reviewing get together in the office and pool your lists together before posting.
            Hide
            Tim Hunt added a comment -

            Kris Stokking, I am sorry my behaviour appears disrespectful to you. Let me say how it appears to me: I come online at 10:30pm and give Mark almost immediate responses so that his work is held up. In addition, I don't treat Mark with kid gloves and patronise him, but instead just get down to the technicalities.

            But to get to the main point, it is not at all ridiculous that the design of a bit of code can be improved during implementation by the person writing the code. That is pretty much the thesis of XP, which I am sure you know about. Do the simplest thing that could possibly work, and then when you have working code, refactor to improve the design, and reduce the technical debt.

            When it comes to an area of code that is part of the external API of your component (in this case what is required of module developers to use outcomes), it is particularly important to get the API right (not least because changing it later probably breaks backwards compatibility).

            I don't think I am saying anything radical or clever here. Just doing the peer reviewers job of pointing out the obvious - but the obvious that only shows up when a fresh pair of eyes looks at the code - but it seems I have noticed something worth addressing here.

            Show
            Tim Hunt added a comment - Kris Stokking , I am sorry my behaviour appears disrespectful to you. Let me say how it appears to me: I come online at 10:30pm and give Mark almost immediate responses so that his work is held up. In addition, I don't treat Mark with kid gloves and patronise him, but instead just get down to the technicalities. But to get to the main point, it is not at all ridiculous that the design of a bit of code can be improved during implementation by the person writing the code. That is pretty much the thesis of XP, which I am sure you know about. Do the simplest thing that could possibly work, and then when you have working code, refactor to improve the design, and reduce the technical debt. When it comes to an area of code that is part of the external API of your component (in this case what is required of module developers to use outcomes), it is particularly important to get the API right (not least because changing it later probably breaks backwards compatibility). I don't think I am saying anything radical or clever here. Just doing the peer reviewers job of pointing out the obvious - but the obvious that only shows up when a fresh pair of eyes looks at the code - but it seems I have noticed something worth addressing here.
            Hide
            Mark Nelson added a comment -

            Jason Fowler how is updating a question then checking that the event was fired not a part of the basic usability and process? If updating a question and triggering the event causes a visible PHP error surely that is a usability issue.

            Show
            Mark Nelson added a comment - Jason Fowler how is updating a question then checking that the event was fired not a part of the basic usability and process? If updating a question and triggering the event causes a visible PHP error surely that is a usability issue.
            Hide
            Andrew Davis added a comment -

            This issue has become a little confused at least in my mind. Feedback has been provided by a number of people. That feedback has been acted on to a varying degree. Mark Nielsen correct me if I have any of this wrong.

            Feedback from:

            Andrew Davis - resolved.

            Tim Hunt - It sounds like the items at https://tracker.moodle.org/browse/MDL-40230?focusedCommentId=245196&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-245196 have been mostly but not entirely resolved.

            Damyon Wiese - Mostly resolved. https://tracker.moodle.org/browse/MDL-40230?focusedCommentId=245298&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-245298 contains a few issues that I don't think have been dealt with yet.

            Mark Nelson - Raised a few more issues that I don't think have been dealt with. https://tracker.moodle.org/browse/MDL-40230?focusedCommentId=245293&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-245293

            I'll provide some commits that resolves some of the clean up type issues here. Mark Nielson, you can use squish them into yours or disregard them in favour of your own fixes as you see fit.

            Show
            Andrew Davis added a comment - This issue has become a little confused at least in my mind. Feedback has been provided by a number of people. That feedback has been acted on to a varying degree. Mark Nielsen correct me if I have any of this wrong. Feedback from: Andrew Davis - resolved. Tim Hunt - It sounds like the items at https://tracker.moodle.org/browse/MDL-40230?focusedCommentId=245196&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-245196 have been mostly but not entirely resolved. Damyon Wiese - Mostly resolved. https://tracker.moodle.org/browse/MDL-40230?focusedCommentId=245298&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-245298 contains a few issues that I don't think have been dealt with yet. Mark Nelson - Raised a few more issues that I don't think have been dealt with. https://tracker.moodle.org/browse/MDL-40230?focusedCommentId=245293&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-245293 I'll provide some commits that resolves some of the clean up type issues here. Mark Nielson, you can use squish them into yours or disregard them in favour of your own fixes as you see fit.
            Hide
            Andrew Davis added a comment -

            Here are some commits that deal with most of Mark Nelson's points.

            Renamed question_edited to question_updated. https://github.com/andyjdavis/moodle/commit/09dadb5f8d65aadfdff53a3d0464b93b6de20fe9

            Duplicate handler entry. https://github.com/andyjdavis/moodle/commit/3453b1798ec840ff8e15f42b8e48f5d029f2de9a

            Copyright notice going to martin instead of moodlerooms. https://github.com/andyjdavis/moodle/commit/7dc6f6efe6ecccbd88ba806523759475561d3502

            I have verified that quiz_question_updated_handler() is called when a question is edited.

            The remaining issues from Mark Nelson are:

            Using "internal" in mod/quiz/db/events.php and whether that line can just be removed.

            In question/question.php you could probably add some other data to the other field, like the question name (assuming it is available w/o a database call).

            Show
            Andrew Davis added a comment - Here are some commits that deal with most of Mark Nelson's points. Renamed question_edited to question_updated. https://github.com/andyjdavis/moodle/commit/09dadb5f8d65aadfdff53a3d0464b93b6de20fe9 Duplicate handler entry. https://github.com/andyjdavis/moodle/commit/3453b1798ec840ff8e15f42b8e48f5d029f2de9a Copyright notice going to martin instead of moodlerooms. https://github.com/andyjdavis/moodle/commit/7dc6f6efe6ecccbd88ba806523759475561d3502 I have verified that quiz_question_updated_handler() is called when a question is edited. The remaining issues from Mark Nelson are: Using "internal" in mod/quiz/db/events.php and whether that line can just be removed. In question/question.php you could probably add some other data to the other field, like the question name (assuming it is available w/o a database call).
            Show
            Andrew Davis added a comment - Removed extra @package tags. https://github.com/andyjdavis/moodle/commit/834ea22d1097e13589919dbc780c24c6cd3bb5eb
            Hide
            Mark Nielsen added a comment - - edited

            Another round of updates to the branch:

            • Freshly rebased
            • Cherry-picked Andrew's changes
            • Reverted accessibility changes to dialog. Will depend on MDL-35926 for it now.
            • Added some YUIDoc
            • Removed interal from the quiz observer. Mark Nelson: I still don't quite understand what this flag actually means. I did look at the docs but that didn't help.

            Let me know if there are any other required changes, otherwise, let's open new MDL for any fallout that can be fixed during maintenance. Once everything checks out, then I can rebase and squish the branch and get it ready for integration.

            Cheers and Thanks!

            Show
            Mark Nielsen added a comment - - edited Another round of updates to the branch: Freshly rebased Cherry-picked Andrew's changes Reverted accessibility changes to dialog. Will depend on MDL-35926 for it now. Added some YUIDoc Removed interal from the quiz observer. Mark Nelson : I still don't quite understand what this flag actually means. I did look at the docs but that didn't help. Let me know if there are any other required changes, otherwise, let's open new MDL for any fallout that can be fixed during maintenance. Once everything checks out, then I can rebase and squish the branch and get it ready for integration. Cheers and Thanks!
            Hide
            Damyon Wiese added a comment - - edited

            Thanks Mark - the policy issue is decided now - so this can proceed as long as it meets AAA level accessibility - this is not entirely up to this issue as the dialogue changes I'm sure will be required (which are being worked on by Rosie) so should not block integration.

            Of the issues I raised and we discussed last night - I am satisfied that all of my major points have been addressed. One minor point that has not been addressed is the capitalisation of the language files.

            To clarify this point, the policy is:

            Capitals should only be used when:
             
            starting a sentence, or
            starting a proper name, like Moodle.
            

            Outcomes is not a proper name.
            Examples (there are many):

            New Outcomes -> New outcomes
            Outcome Unique ID is required. -> Outcome unique ID is required.
            "Course Settings</a> page." -> "Course settings</a> page."
            Completion Marking -> Completion marking
            Average Grade -> Average grade
            Activity and Performance -> Activity and performance
            Unmapped Quiz Questions -> Unmapped quiz questions

            This is almost there - thanks again.

            Show
            Damyon Wiese added a comment - - edited Thanks Mark - the policy issue is decided now - so this can proceed as long as it meets AAA level accessibility - this is not entirely up to this issue as the dialogue changes I'm sure will be required (which are being worked on by Rosie) so should not block integration. Of the issues I raised and we discussed last night - I am satisfied that all of my major points have been addressed. One minor point that has not been addressed is the capitalisation of the language files. To clarify this point, the policy is: Capitals should only be used when:   starting a sentence, or starting a proper name, like Moodle. Outcomes is not a proper name. Examples (there are many): New Outcomes -> New outcomes Outcome Unique ID is required. -> Outcome unique ID is required. "Course Settings</a> page." -> "Course settings</a> page." Completion Marking -> Completion marking Average Grade -> Average grade Activity and Performance -> Activity and performance Unmapped Quiz Questions -> Unmapped quiz questions This is almost there - thanks again.
            Hide
            Mark Nelson added a comment -

            Hi Mark, the event code is looking fine now. However, I should have noticed the issue with the location of the event earlier. Even though question is a sub-system, the event class should be located in lib/classes/event, not question/classes/event. I apologise for not picking this up during my previous code review.

            Show
            Mark Nelson added a comment - Hi Mark, the event code is looking fine now. However, I should have noticed the issue with the location of the event earlier. Even though question is a sub-system, the event class should be located in lib/classes/event, not question/classes/event. I apologise for not picking this up during my previous code review.
            Hide
            Mark Nielsen added a comment -

            Updated branch with event class move and capitalization fixes. Please take another look, thanks!

            Show
            Mark Nielsen added a comment - Updated branch with event class move and capitalization fixes. Please take another look, thanks!
            Hide
            Tim Hunt added a comment -

            I just discussed some things with Mark Nielsen on Skype. The key points are:

            1. It is worth considering the query

            DELETE FROM {outcome_attempts} WHERE outcomeusedareaid IN (
                    SELECT DISTINCT ua.id
                          FROM {outcome_used_areas} ua
                          JOIN {outcome_areas} area ON area.id = ua.outcomeareaid
                          JOIN {context} ctx ON ctx.instanceid = ua.cmid AND ctx.contextlevel = ?
                         WHERE area.area = ?
                           AND ctx.id $sql)
            AND NOT EXISTS (
                     SELECT 1 FROM {question_attempts} qa WHERE qa.id = itemid
            

            for delete_question_outcome_attempts. That should work on all DBs, though there is a possible concern about performance.

            2. It is still the case that requring the question_udpated handler in mod/quiz requires the quiz to get involved in something that is more related to outcomes and questions. To put it another way, if code like this is requried in mod_quiz, then it is also required in add-ons like mod_qpractice and mod_offlinequiz. That means we need developer docs to tell the developers of those modules what they need to do, and that can be added as a page linked from http://docs.moodle.org/dev/Using_the_question_engine_from_module.

            3. Rather than use is_real_question_type to determine if a question type supports outcomes, we should add a new method supports_outcomes() to the qeustion_type class. (By default, it can return $this->is_real_question_type()) so that qtype developers don't have to do anything, but it is clearer to do it that way.

            Also two trivial points:

            4. Some of the PHP docs are still hard to understand if you don't fully understand what the outcomes system is trying to do. E.g. the comment on record_outcomes.

            5. There are a few codechecker issues in the code outside the outcomes folder. (Specific ones I noticed: No newline at end of qusetionlib.php, and PHP-doc style comment in the middle of delete_question_outcome_attempts.)

            Show
            Tim Hunt added a comment - I just discussed some things with Mark Nielsen on Skype. The key points are: 1. It is worth considering the query DELETE FROM {outcome_attempts} WHERE outcomeusedareaid IN ( SELECT DISTINCT ua.id FROM {outcome_used_areas} ua JOIN {outcome_areas} area ON area.id = ua.outcomeareaid JOIN {context} ctx ON ctx.instanceid = ua.cmid AND ctx.contextlevel = ? WHERE area.area = ? AND ctx.id $sql) AND NOT EXISTS ( SELECT 1 FROM {question_attempts} qa WHERE qa.id = itemid for delete_question_outcome_attempts. That should work on all DBs, though there is a possible concern about performance. 2. It is still the case that requring the question_udpated handler in mod/quiz requires the quiz to get involved in something that is more related to outcomes and questions. To put it another way, if code like this is requried in mod_quiz, then it is also required in add-ons like mod_qpractice and mod_offlinequiz. That means we need developer docs to tell the developers of those modules what they need to do, and that can be added as a page linked from http://docs.moodle.org/dev/Using_the_question_engine_from_module . 3. Rather than use is_real_question_type to determine if a question type supports outcomes, we should add a new method supports_outcomes() to the qeustion_type class. (By default, it can return $this->is_real_question_type()) so that qtype developers don't have to do anything, but it is clearer to do it that way. Also two trivial points: 4. Some of the PHP docs are still hard to understand if you don't fully understand what the outcomes system is trying to do. E.g. the comment on record_outcomes. 5. There are a few codechecker issues in the code outside the outcomes folder. (Specific ones I noticed: No newline at end of qusetionlib.php, and PHP-doc style comment in the middle of delete_question_outcome_attempts.)
            Hide
            Mark Nelson added a comment -

            Tim, I don't remember talking to you on skype?

            Mark, all looking good now. However, I would remove the '@subpackage questionbank' and rename 'moodlecore' to 'core'. Renaming 'moodlecore' to 'core' should be done for all newly introduced files that use this term. Also, if Andrew Davis would be so kind to retest the event trigger that would be very much appreciated.

            Show
            Mark Nelson added a comment - Tim, I don't remember talking to you on skype? Mark, all looking good now. However, I would remove the '@subpackage questionbank' and rename 'moodlecore' to 'core'. Renaming 'moodlecore' to 'core' should be done for all newly introduced files that use this term. Also, if Andrew Davis would be so kind to retest the event trigger that would be very much appreciated.
            Hide
            Mark Nielsen added a comment -

            Updated the branch with Tim's and Mr. Nelson's suggestions. I feel like we are almost there!

            Show
            Mark Nielsen added a comment - Updated the branch with Tim's and Mr. Nelson's suggestions. I feel like we are almost there!
            Hide
            Mark Nielsen added a comment -

            FYI, I did add the query from Tim's post and it does delete the correct records even though I don't fully understand how it works. I did remove the DISTINCT keyword because that caused the query to use a temporary table when I ran explain on it. So, that should fix any performance problems with the query AFAIK.

            Show
            Mark Nielsen added a comment - FYI, I did add the query from Tim's post and it does delete the correct records even though I don't fully understand how it works. I did remove the DISTINCT keyword because that caused the query to use a temporary table when I ran explain on it. So, that should fix any performance problems with the query AFAIK.
            Hide
            Andrew Davis added a comment -

            Doing some final testing. Found one quirk. If you have time to fix this now that's great. Otherwise, let me know and I'll open a new MDL.

            If I open up the outcomes related URLs before outcomes are disabled (or otherwise come to possess the URLs for a site where outcomes are disabled) I'm not locked out of the course level outcomes pages. The site level outcomes pages all return an error if you try to access them once outcomes are disabled but the following pages all load as if outcomes were enabled.

            Note: the user still requires the relevant capabilities to access these pages. Its just that a user with those capabilities can still access these pages even when the outcome system is meant to have been disabled.

            Course outcome sets: /outcome/course.php?contextid=15

            Completion marking: /outcome/course.php?action=report_marking_table&contextid=15

            Activity and performance: /outcome/course.php?action=report_course_performance_table&contextid=15&forceoutcomesetid=5

            Coverage: /outcome/course.php?action=report_course_coverage_table&contextid=15&forceoutcomesetid=5

            Unmapped content items and quiz questions: /outcome/course.php?action=report_course_unmapped_table&contextid=15

            Show
            Andrew Davis added a comment - Doing some final testing. Found one quirk. If you have time to fix this now that's great. Otherwise, let me know and I'll open a new MDL. If I open up the outcomes related URLs before outcomes are disabled (or otherwise come to possess the URLs for a site where outcomes are disabled) I'm not locked out of the course level outcomes pages. The site level outcomes pages all return an error if you try to access them once outcomes are disabled but the following pages all load as if outcomes were enabled. Note: the user still requires the relevant capabilities to access these pages. Its just that a user with those capabilities can still access these pages even when the outcome system is meant to have been disabled. Course outcome sets: /outcome/course.php?contextid=15 Completion marking: /outcome/course.php?action=report_marking_table&contextid=15 Activity and performance: /outcome/course.php?action=report_course_performance_table&contextid=15&forceoutcomesetid=5 Coverage: /outcome/course.php?action=report_course_coverage_table&contextid=15&forceoutcomesetid=5 Unmapped content items and quiz questions: /outcome/course.php?action=report_course_unmapped_table&contextid=15
            Hide
            Tim Hunt added a comment -

            I think we are basically good to go for integration. My remaining question is, should we rebase the branch down to fewer commits or not? I don't know. I would an integrator like to express an opinion?

            (Also, it goes on making no sense to me that events belonging to core subsystems are all in lib/classes, not question/classes, group/classes, etc. That seems to go against years of hard work to make core components more like separate plugins, but this is not the place to discuss that.)

            Show
            Tim Hunt added a comment - I think we are basically good to go for integration. My remaining question is, should we rebase the branch down to fewer commits or not? I don't know. I would an integrator like to express an opinion? (Also, it goes on making no sense to me that events belonging to core subsystems are all in lib/classes, not question/classes, group/classes, etc. That seems to go against years of hard work to make core components more like separate plugins, but this is not the place to discuss that.)
            Hide
            Mark Nielsen added a comment -

            Got another update to address latest concern by Andrew Davis. Also found that M.core.exception, etc no long displays by default. So, set visible to true on those.

            I will be doing squishing of commits because most of them are nonsensical. I'll do this once we look good for submitting to integration.

            Show
            Mark Nielsen added a comment - Got another update to address latest concern by Andrew Davis . Also found that M.core.exception, etc no long displays by default. So, set visible to true on those. I will be doing squishing of commits because most of them are nonsensical. I'll do this once we look good for submitting to integration.
            Hide
            Tim Hunt added a comment -

            Just spotted: we need testing instructions here.

            Show
            Tim Hunt added a comment - Just spotted: we need testing instructions here.
            Hide
            Andrew Davis added a comment -

            Hi Tim. The testing instructions are in an attached spreadsheet. I've expanded the testing instructions on this issue to detail how it see it being done.

            Show
            Andrew Davis added a comment - Hi Tim. The testing instructions are in an attached spreadsheet. I've expanded the testing instructions on this issue to detail how it see it being done.
            Hide
            Andrew Davis added a comment -

            I was pre-emptively running through some of the testing spreadsheet and noticed that some of the tests require a student to have at least partial access to the outcome reports. Students are currently entirely locked out of the outcome reports. There are two possibilities:

            1) The testing spreadsheet is wrong and the code is correct. In which case the spreadsheet just needs to be reviewed and a decent sized chunk of the tests simply removed.

            2) Student's should actually be able to see one or more of the reports. Should this be the case, as Mark is presumably fast asleep right now, I've gone ahead and prepared a commit that attempts to address this. https://github.com/andyjdavis/moodle/commit/62b1d5871c6ad16862c41be736387d0dae989944 Treat my changes here with the deepest suspicion pending review by Mark.

            Right now the code uses "moodle/grade:edit" (which students won't have) to regulate access and there wasn't another capability that appeared to be an appropriate substitute. The above commit adds "moodle/outcome:view".

            Similarly the testing spreadsheet mentions a "my stream tab" and a "Instructor Student Completion Marking report" that I don't think exist and which can be stripped out of the testing document. Mark, can you clarify the situation?

            Show
            Andrew Davis added a comment - I was pre-emptively running through some of the testing spreadsheet and noticed that some of the tests require a student to have at least partial access to the outcome reports. Students are currently entirely locked out of the outcome reports. There are two possibilities: 1) The testing spreadsheet is wrong and the code is correct. In which case the spreadsheet just needs to be reviewed and a decent sized chunk of the tests simply removed. 2) Student's should actually be able to see one or more of the reports. Should this be the case, as Mark is presumably fast asleep right now, I've gone ahead and prepared a commit that attempts to address this. https://github.com/andyjdavis/moodle/commit/62b1d5871c6ad16862c41be736387d0dae989944 Treat my changes here with the deepest suspicion pending review by Mark. Right now the code uses "moodle/grade:edit" (which students won't have) to regulate access and there wasn't another capability that appeared to be an appropriate substitute. The above commit adds "moodle/outcome:view". Similarly the testing spreadsheet mentions a "my stream tab" and a "Instructor Student Completion Marking report" that I don't think exist and which can be stripped out of the testing document. Mark, can you clarify the situation?
            Hide
            Mark Nielsen added a comment -

            Hello Andrew, thanks for your continued work on this!

            1) The testing spreadsheet is wrong and the code is correct. In which case the spreadsheet just needs to be reviewed and a decent sized chunk of the tests simply removed.

            Sort of a middle ground. This is a feature we were going to implement, but due to time constraints, we have not implemented it yet. Your suggested patch is close, but I don't think we want students to see the coverage report and we might just link directly to the performance report. Also, currently the performance report doesn't show just the current user's information. So, still some things to be thought about and implemented.

            Show
            Mark Nielsen added a comment - Hello Andrew, thanks for your continued work on this! 1) The testing spreadsheet is wrong and the code is correct. In which case the spreadsheet just needs to be reviewed and a decent sized chunk of the tests simply removed. Sort of a middle ground. This is a feature we were going to implement, but due to time constraints, we have not implemented it yet. Your suggested patch is close, but I don't think we want students to see the coverage report and we might just link directly to the performance report. Also, currently the performance report doesn't show just the current user's information. So, still some things to be thought about and implemented.
            Hide
            Andrew Davis added a comment - - edited

            Mark, do you think is ready to go for integration? Do you have any outstanding things to do? If you're ready by all means rebase and submit this for integration.

            I'm writing testing instructions for this issue now. They will be based on that spreadsheet but won't refer to it. It includes too much future functionality to make it practical to use for testing. Apologies for not reading it closer previously and not realizing it wasn't adequate for testing current functionality.

            UPDATE: I've added testing instructions that I believe cover the new functionality fairly well. All in 27 easy steps.

            Show
            Andrew Davis added a comment - - edited Mark, do you think is ready to go for integration? Do you have any outstanding things to do? If you're ready by all means rebase and submit this for integration. I'm writing testing instructions for this issue now. They will be based on that spreadsheet but won't refer to it. It includes too much future functionality to make it practical to use for testing. Apologies for not reading it closer previously and not realizing it wasn't adequate for testing current functionality. UPDATE: I've added testing instructions that I believe cover the new functionality fairly well. All in 27 easy steps.
            Hide
            Mark Nielsen added a comment -

            Nothing like a fresh rebase and squish in the morning!

            Let's submit this for integration! Thanks you Andrew Davis for your continued work and attention on this!

            Show
            Mark Nielsen added a comment - Nothing like a fresh rebase and squish in the morning! Let's submit this for integration! Thanks you Andrew Davis for your continued work and attention on this!
            Hide
            Tim Hunt added a comment -

            I feel this deserves a movie cliche: http://www.youtube.com/watch?v=1koa2xAxCAw

            Show
            Tim Hunt added a comment - I feel this deserves a movie cliche: http://www.youtube.com/watch?v=1koa2xAxCAw
            Hide
            Andrew Davis added a comment -

            Submitting for integration.

            Show
            Andrew Davis added a comment - Submitting for integration.
            Hide
            Damyon Wiese added a comment -

            (phew)

            Show
            Damyon Wiese added a comment - (phew)
            Hide
            Dan Poltawski added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            Dan Poltawski added a comment -

            Hi Everyone,

            I'm just getting my teeth into this at the moment (reading the docs etc), but it looks like the code hasn't been rebased, i've done a bit of conflict resolution, but its tricky:
            See the git-show output here (note thats not exactly a diff):
            https://gist.github.com/danpoltawski/78558e067fcf788278a8
            https://github.com/danpoltawski/moodle/commit/6e02a624e30db9fba1f167c884499c8bd17762ee

            It'd be good if someone could rebase this (or check my version of the conflcit resolution).

            Show
            Dan Poltawski added a comment - Hi Everyone, I'm just getting my teeth into this at the moment (reading the docs etc), but it looks like the code hasn't been rebased, i've done a bit of conflict resolution, but its tricky: See the git-show output here (note thats not exactly a diff): https://gist.github.com/danpoltawski/78558e067fcf788278a8 https://github.com/danpoltawski/moodle/commit/6e02a624e30db9fba1f167c884499c8bd17762ee It'd be good if someone could rebase this (or check my version of the conflcit resolution).
            Hide
            Damyon Wiese added a comment -

            Your merge looks OK to me.

            Show
            Damyon Wiese added a comment - Your merge looks OK to me.
            Hide
            Dan Poltawski added a comment -

            Failure on MSSQL:

            outcome_model_outcome_repository_test::test_find_by_area_and_filter
            dml_read_exception: Error reading from database (Column 'phpu_outcome.outcomesetid' is invalid in the select list because it is not contained in either an aggregate function or the GROUP BY clause.
             
                        SELECT o.*
                          FROM phpu_outcome o
                               LEFT JOIN phpu_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = ?) LEFT JOIN phpu_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = ?)
                    INNER JOIN phpu_outcome_area_outcomes ao ON o.id = ao.outcomeid
                    INNER JOIN phpu_outcome_areas a ON a.id = ao.outcomeareaid
                         WHERE o.outcomesetid = ? AND (edulevels.value = ? AND subjects.value = ?) AND a.id = ? AND o.deleted = ? AND o.assessable = ?
                      GROUP BY o.id
             
            [array (
              0 => 'edulevels',
              1 => 'subjects',
              2 => '1',
              3 => '10',
              4 => 'Math',
              5 => '1',
              6 => 0,
              7 => 1,
            )])
             
            /Users/danp/git/integration/lib/dml/moodle_database.php:448
            /Users/danp/git/integration/lib/dml/mssql_native_moodle_database.php:242
            /Users/danp/git/integration/lib/dml/mssql_native_moodle_database.php:715
            /Users/danp/git/integration/outcome/classes/model/outcome_repository.php:272
            /Users/danp/git/integration/outcome/tests/model/outcome_repository_test.php:171
            /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76
             
            To re-run:
             vendor/bin/phpunit outcome_model_outcome_repository_test outcome/tests/model/outcome_repository_test.php
            

            Show
            Dan Poltawski added a comment - Failure on MSSQL: outcome_model_outcome_repository_test::test_find_by_area_and_filter dml_read_exception: Error reading from database (Column 'phpu_outcome.outcomesetid' is invalid in the select list because it is not contained in either an aggregate function or the GROUP BY clause.   SELECT o.* FROM phpu_outcome o LEFT JOIN phpu_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = ?) LEFT JOIN phpu_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = ?) INNER JOIN phpu_outcome_area_outcomes ao ON o.id = ao.outcomeid INNER JOIN phpu_outcome_areas a ON a.id = ao.outcomeareaid WHERE o.outcomesetid = ? AND (edulevels.value = ? AND subjects.value = ?) AND a.id = ? AND o.deleted = ? AND o.assessable = ? GROUP BY o.id   [array ( 0 => 'edulevels', 1 => 'subjects', 2 => '1', 3 => '10', 4 => 'Math', 5 => '1', 6 => 0, 7 => 1, )])   /Users/danp/git/integration/lib/dml/moodle_database.php:448 /Users/danp/git/integration/lib/dml/mssql_native_moodle_database.php:242 /Users/danp/git/integration/lib/dml/mssql_native_moodle_database.php:715 /Users/danp/git/integration/outcome/classes/model/outcome_repository.php:272 /Users/danp/git/integration/outcome/tests/model/outcome_repository_test.php:171 /Users/danp/git/integration/lib/phpunit/classes/advanced_testcase.php:76   To re-run: vendor/bin/phpunit outcome_model_outcome_repository_test outcome/tests/model/outcome_repository_test.php
            Hide
            Dan Poltawski added a comment -

            Hi Everyone

            Another reviewer in here with more comments... please bare with me.. My integration review is still very much ongoing, but just wanted to feedback my thoughts in progress:

            Overall, the code is looking really really nice to me. Great job on this huge change

            Testing:

            1. We need to clairfy the situation regarding testing here. What is going on with that testing spreadsheet? It seems like there is a lot of functionality that needs testing here and the instructions in the issue don't seem sufficient for that.

            2. I've added 'acceptance test required' as we don't have any behat tests here.

            Outcome fields

            3. I would like to clarify with Martin about having these top level 'outcome set' fields like provider and region. It seems to be me very specific (and probably a bit US-centric), so I would like to be certain that these top level fields are sutiable and not just going to be applicable to US insitutions.

            4. I also feel like the terminology 'doc number' in the outcome field seems unusual to me (but i'm by no means an expert in this area).

            5. I find it a bit strange that the first item we have for an outcome is unique ID and the help says: 'The unique ID should be a globally unique value. Meaning it should even be unique across Moodle sites'. In general i'd say most Moodle users wouldn't consider any other moodle sites at all?

            UI

            6. I am a bit concerned that many of the UI screens feel a little bit raw in my short amount of time testing.

              • Strange spacing here:
              • Lack of styling/clue how to be interacted with here:
              • I have been lead into quite a few dead ends e.g. 'Add an outcome' on course page, click to add outcome then get a popup which asks you to ask the admin to add an outcome. (In that paritcular example it says the admin needs to add outcomes or the outcome needs to be mapped to the course - surely it should be able to tell which - and if there are no outcomes maybe we shouldn't be showing it in the form?)

            7. I find it very unusual the way we have these javascript popups within the forms - like adding outcomes to an outcome set or adding an outcome to an activity. Also we are using words where traditionally we would probably use 'action icons' for edit/delete etc.

            Migration/Upgrade

            8. Its not so clear from the spec or the code i've seen so far what we are doing here. It seems like upgrade of existing outcomes is not tackeled? You say in your doc that you plan to run both concurrently - and i see the switch exists for this, but is that what we are doing here in core too?

            Code (mostly trivial things so far)

            9. Cross-Database testing is my major concern at this point, there are a lot of complex queries which need testing on all dbs and espsecially things which aren't covered by the unit tests.

            10. There are lots of lang strings which should probably by AMOS CPY'ed from other lang files, the lang file also needs to be ordered alphabetically (it greatly helps reduce the chance of introducing duplicate strings)

            11. (just intruiged) SplObjectStorage is used quite a lot - i've not really seen it used in Moodle before - haven't noticed the particular reason for using it yet?

            12. It would be nice if we had some sample import files as fixtures for unit testing.

            Show
            Dan Poltawski added a comment - Hi Everyone Another reviewer in here with more comments... please bare with me.. My integration review is still very much ongoing, but just wanted to feedback my thoughts in progress: Overall, the code is looking really really nice to me. Great job on this huge change Testing: 1. We need to clairfy the situation regarding testing here. What is going on with that testing spreadsheet? It seems like there is a lot of functionality that needs testing here and the instructions in the issue don't seem sufficient for that. 2. I've added 'acceptance test required' as we don't have any behat tests here. Outcome fields 3. I would like to clarify with Martin about having these top level 'outcome set' fields like provider and region. It seems to be me very specific (and probably a bit US-centric), so I would like to be certain that these top level fields are sutiable and not just going to be applicable to US insitutions. 4. I also feel like the terminology 'doc number' in the outcome field seems unusual to me (but i'm by no means an expert in this area). 5. I find it a bit strange that the first item we have for an outcome is unique ID and the help says: 'The unique ID should be a globally unique value. Meaning it should even be unique across Moodle sites'. In general i'd say most Moodle users wouldn't consider any other moodle sites at all? UI 6. I am a bit concerned that many of the UI screens feel a little bit raw in my short amount of time testing. Strange spacing here: Lack of styling/clue how to be interacted with here: I have been lead into quite a few dead ends e.g. 'Add an outcome' on course page, click to add outcome then get a popup which asks you to ask the admin to add an outcome. (In that paritcular example it says the admin needs to add outcomes or the outcome needs to be mapped to the course - surely it should be able to tell which - and if there are no outcomes maybe we shouldn't be showing it in the form?) 7. I find it very unusual the way we have these javascript popups within the forms - like adding outcomes to an outcome set or adding an outcome to an activity. Also we are using words where traditionally we would probably use 'action icons' for edit/delete etc. Migration/Upgrade 8. Its not so clear from the spec or the code i've seen so far what we are doing here. It seems like upgrade of existing outcomes is not tackeled? You say in your doc that you plan to run both concurrently - and i see the switch exists for this, but is that what we are doing here in core too? Code (mostly trivial things so far) 9. Cross-Database testing is my major concern at this point, there are a lot of complex queries which need testing on all dbs and espsecially things which aren't covered by the unit tests. 10. There are lots of lang strings which should probably by AMOS CPY'ed from other lang files, the lang file also needs to be ordered alphabetically (it greatly helps reduce the chance of introducing duplicate strings) 11. (just intruiged) SplObjectStorage is used quite a lot - i've not really seen it used in Moodle before - haven't noticed the particular reason for using it yet? 12. It would be nice if we had some sample import files as fixtures for unit testing.
            Hide
            Kris Stokking added a comment -

            Hey Dan,

            Thanks very much for the review, I'll answer what I can:

            Testing

            1. I think I will need to defer to Andrew Davis on this - originally we had pointed users to the spreadsheet, but that was changed. I understand there was some confusion around outcomes reports that ended up not being applicable in the final version - we can remove that from the list of testing instructions in the spreadsheet if that is confusing.
            2. Good point - we have not started writing Behat tests for any of our features yet as we've only just included Moodle 2.5 in our development branch. Is this required for new features to be integrated? Is there a checklist of integration requirements that we can refer to so that there's nothing else missing?

            Outcome fields

            The new version of outcomes is intended to work with standards that are created outside of the institution/course and imported into the site (although they certainly support manual creation if desired). The provider field is simply the organization that has created the outcome set. The provider, region, doc number and GUID fields are critical for Outcomes - they facilitate the import process, ensure that outcomes can be referred to both inside and outside of Moodle, and help admins understand where the outcomes originated.

            Our hope is that standards authors or community developers will write import plugins so that outcome data will be easily imported and populated. In the case where they are not, those fields can simply be ignored except for the GUID. I can definitely understand where the help text might be confusing for some admin users that only work on a single Moodle site. In that case, the Unique ID functions similar to course shortname for example. I'll work on updating the help text appropriately.

            6. Something definitely looks funky with the styles - there should be spacing in between the links, and the outcome set should show a folder icon next to it which is expanded to show the nested outcomes underneath.

            I would also agree with you about the messaging when no outcomes are available on the site or in the course. We'll make that messaging a bit more intuitive.

            7. I'm not sure what the issue is with using modals other than they aren't widely used in Moodle. They are very effective for content creation and help avoid the need for full page reloads. Regarding icons: We can replace the text of action links to clean the interface up.

            8. I need to update http://docs.moodle.org/dev/Migration_and_Technical_Issues with more details from my previous comment.

            12. We have developed import plugins for the Achievement Standards Network, which is an open standards provider. The files are available for import from http://asn.jesandco.org/resources/ASNJurisdiction

            Show
            Kris Stokking added a comment - Hey Dan, Thanks very much for the review, I'll answer what I can: Testing 1. I think I will need to defer to Andrew Davis on this - originally we had pointed users to the spreadsheet, but that was changed. I understand there was some confusion around outcomes reports that ended up not being applicable in the final version - we can remove that from the list of testing instructions in the spreadsheet if that is confusing. 2. Good point - we have not started writing Behat tests for any of our features yet as we've only just included Moodle 2.5 in our development branch. Is this required for new features to be integrated? Is there a checklist of integration requirements that we can refer to so that there's nothing else missing? Outcome fields The new version of outcomes is intended to work with standards that are created outside of the institution/course and imported into the site (although they certainly support manual creation if desired). The provider field is simply the organization that has created the outcome set. The provider, region, doc number and GUID fields are critical for Outcomes - they facilitate the import process, ensure that outcomes can be referred to both inside and outside of Moodle, and help admins understand where the outcomes originated. Our hope is that standards authors or community developers will write import plugins so that outcome data will be easily imported and populated. In the case where they are not, those fields can simply be ignored except for the GUID. I can definitely understand where the help text might be confusing for some admin users that only work on a single Moodle site. In that case, the Unique ID functions similar to course shortname for example. I'll work on updating the help text appropriately. 6. Something definitely looks funky with the styles - there should be spacing in between the links, and the outcome set should show a folder icon next to it which is expanded to show the nested outcomes underneath. I would also agree with you about the messaging when no outcomes are available on the site or in the course. We'll make that messaging a bit more intuitive. 7. I'm not sure what the issue is with using modals other than they aren't widely used in Moodle. They are very effective for content creation and help avoid the need for full page reloads. Regarding icons: We can replace the text of action links to clean the interface up. 8. I need to update http://docs.moodle.org/dev/Migration_and_Technical_Issues with more details from my previous comment . 12. We have developed import plugins for the Achievement Standards Network, which is an open standards provider. The files are available for import from http://asn.jesandco.org/resources/ASNJurisdiction
            Hide
            Dan Poltawski added a comment -

            Hi Everyone,

            I'm afraid I think that we have just a bit too many issues with this remaining in order to integrate this week. Overall I think the code is looking great, but breakage on some databases and the clean theme UI breing broken makes this just a bit too much. I also want to clarify with MD a few points. Here is my summmary:

            Issues to fix before integration

            1. Cross-DB compatibilty. The code is breaking all over the place on MSSQL (see examples below).
            2. UI is broken on the clean theme (bootstrapbase needs addressing).
            3. Strings need ordering and AMOS CPY commands so translators don't need to retranslate everything.
            4. core_component_testcase::test_get_core_subsystems

            Clarifcations for integration

            1. I feel very unusure about these top level fields on the outcome sets and outcomes, I see its been in the spec. So I just want to get a +1 about the pieces from MD to confirm. Examples:
              1. I find it strange that we will be storing things like region within an individual moodle install, it seems very niche to me to need to be able to distinguish.
              2. The need to add unique ids and doc number seems cumbersome. It almost seems like its only geared up towards importing outcome sets frm standards. It might just be that these fields would make more sense to me if they were named like standard moodle names (like idnumber, shortname etc).
            2. I want to confirm the upgrade/migration situation with MD, I am not certain the situation we are ending up with here was clear from the spec.
            3. Testing plan.

            Niggling UI things spotted in brief testing:

            1. The outcomes link appears in navigation before outcomes are mapped to the course, and the only way to get to it is to go to the course settings. I don't think we should have dead links like that.
            2. The outcomes selection pane is available on activites before outcomes are mapped to a course and it doesn't give a clear idea of how to solve that problem.
            3. The outcomes admin page:
              1. It seems like level/subject should be displayed on that tree structure, else its hard to see that information at a glance.
              2. The interface for rearranging the outcomes in the tree structure seems hard to understand
            4. Going to Course outcome reports leads to dead ends with error messages when students aren't enrolled.


            MSSQL errors

            Adding outcome to activity:

            Column 'mdl_outcome.outcomesetid' is invalid in the select list because it is not contained in either an aggregate function or the GROUP BY clause. SELECT o.* FROM mdl_outcome o LEFT JOIN mdl_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = ?) LEFT JOIN mdl_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = ?) WHERE o.outcomesetid = ? AND (edulevels.value = ? AND subjects.value = ?) AND o.deleted = ? AND o.assessable = ? GROUP BY o.id [array ( 0 => 'edulevels', 1 => 'subjects', 2 => '1', 3 => 'KS4', 4 => 'Computing', 5 => 0, 6 => 1, )] Error code: dmlreadexception
            Stack trace:
            * line 448 of /lib/dml/moodle_database.php: dml_read_exception thrown
            * line 242 of /lib/dml/mssql_native_moodle_database.php: call to moodle_database->query_end()
            * line 715 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->query_end()
            * line 221 of /outcome/classes/model/outcome_repository.php: call to mssql_native_moodle_database->get_recordset_sql()
            * line 188 of /outcome/classes/controller/mapping_ajax_controller.php: call to core_outcome\model\outcome_repository->find_by_filter()
            * line ? of unknownfile: call to core_outcome\controller\mapping_ajax_controller->get_mappable_outcomes_action()
            * line 149 of /outcome/classes/controller/kernel.php: call to call_user_func()
            * line 110 of /outcome/classes/controller/kernel.php: call to core_outcome\controller\kernel->execute_callback()
            * line 58 of /outcome/ajax.php: call to core_outcome\controller\kernel->handle()
            

            On umapped activities and quiz questions:

             Cannot resolve the collation conflict between "Latin1_General_BIN" and "Latin1_General_CS_AS" in the equal to operation.
             
            SELECT cm.id, COUNT(u.id) areacount
            FROM mdl_course_modules cm
            INNER JOIN mdl_modules mods ON mods.id = cm.module AND mods.visible = ?
            LEFT JOIN mdl_outcome_areas a ON a.itemid = cm.id AND a.area = ? AND component = CAST(? AS NVARCHAR(255)) + CAST(mods.name AS NVARCHAR(255)) 
            LEFT JOIN mdl_outcome_used_areas u ON u.cmid = cm.id
            WHERE cm.course = ? AND a.id IS NULL
            GROUP BY cm.id
            [array (
            0 => 1,
            1 => 'mod',
            2 => 'mod_',
            3 => '2',
            )]
            Error code: dmlreadexception
             
            Stack trace:
            line 448 of /lib/dml/moodle_database.php: dml_read_exception thrown
            line 242 of /lib/dml/mssql_native_moodle_database.php: call to moodle_database->query_end()
            line 715 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->query_end()
            line 749 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->get_recordset_sql()
            line 61 of /outcome/support/mod/classes/coverage.php: call to mssql_native_moodle_database->get_records_sql()
            line 113 of /outcome/support/mod/classes/coverage.php: call to outcomesupport_mod\coverage->get_unmapped_activities()
            line 174 of /outcome/renderer.php: call to outcomesupport_mod\coverage->get_unmapped_content()
            line 241 of /outcome/classes/controller/report_controller.php: call to core_outcome_renderer->outcome_course_unmapped()
            line ? of unknownfile: call to core_outcome\controller\report_controller->report_course_unmapped_table_action()
            line 149 of /outcome/classes/controller/kernel.php: call to call_user_func()
            line 110 of /outcome/classes/controller/kernel.php: call to core_outcome\controller\kernel->execute_callback()
            line 53 of /outcome/course.php: call to core_outcome\controller\kernel->handle()
            

            On completion marking:

            Debug info: Column 'mdl_outcome.docnum' is invalid in the select list because it is not contained in either an aggregate function or the GROUP BY clause.
            SELECT TOP 50
            o.id, o.docnum, o.description, t1.resources, t2.activities, t3.questions, t3.questionsused
            FROM 
            mdl_outcome o
            LEFT JOIN mdl_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = ?) LEFT JOIN mdl_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = ?)
            LEFT JOIN (
            SELECT o.id, COUNT(used.id) resources
            FROM mdl_outcome o
            INNER JOIN mdl_outcome_area_outcomes ao ON o.id = ao.outcomeid
            INNER JOIN mdl_outcome_areas areas ON areas.id = ao.outcomeareaid
            INNER JOIN mdl_outcome_used_areas used ON areas.id = used.outcomeareaid
            INNER JOIN mdl_course_modules cm ON cm.id = used.cmid AND cm.course = ?
            INNER JOIN mdl_modules mods ON mods.id = cm.module AND mods.name IN (?,?,?,?,?,?,?)
            GROUP BY o.id
            ) t1
            ON t1.id = o.id
             
            LEFT JOIN (
            SELECT o.id, COUNT(DISTINCT cm2.id) activities
            FROM mdl_outcome o
            INNER JOIN mdl_outcome_area_outcomes ao ON o.id = ao.outcomeid
            INNER JOIN mdl_outcome_areas areas2 ON areas2.id = ao.outcomeareaid
            INNER JOIN mdl_outcome_used_areas used2 ON areas2.id = used2.outcomeareaid
            INNER JOIN mdl_course_modules cm2 ON cm2.id = used2.cmid AND cm2.course = ?
            INNER JOIN mdl_modules mods2 ON mods2.id = cm2.module AND mods2.name IN (?,?,?,?,?,?,?,?,?,?,?,?,?)
            GROUP BY o.id
            ) t2
            ON t2.id = o.id
             
            LEFT JOIN (
            SELECT o.id, COUNT(DISTINCT ao.id) questions, COUNT(DISTINCT cm3.id) questionsused
            FROM mdl_outcome o
            INNER JOIN mdl_outcome_area_outcomes ao ON o.id = ao.outcomeid
            INNER JOIN mdl_outcome_areas areas3 ON areas3.id = ao.outcomeareaid AND areas3.area = ?
            AND areas3.component COLLATE Latin1_General_CS_AS LIKE ? ESCAPE '\'
            INNER JOIN mdl_question q ON q.id = areas3.itemid
            INNER JOIN mdl_question_categories qc ON qc.id = q.category
            INNER JOIN mdl_context ctx ON (ctx.id = qc.contextid
            AND (ctx.id = ? OR ctx.path COLLATE Latin1_General_CS_AS LIKE ? ESCAPE '\' OR ctx.id = ?))
            LEFT JOIN mdl_outcome_used_areas used3 ON areas3.id = used3.outcomeareaid
            LEFT JOIN mdl_course_modules cm3 ON cm3.id = used3.cmid AND cm3.course = ?
            WHERE (ctx.id != ? OR (ctx.id = ? AND cm3.id IS NOT NULL))
            GROUP BY o.id
            ) t3
            ON t3.id = o.id
             
            WHERE o.outcomesetid = ? AND (edulevels.value = ? AND subjects.value = ?) AND o.deleted = ? AND o.assessable = ? GROUP BY o.id, t1.resources, t2.activities, t3.questions, t3.questionsused
            ORDER BY description ASC
            [array (
            0 => 'edulevels',
            1 => 'subjects',
            2 => '2',
            3 => 'book',
            4 => 'resource',
            5 => 'folder',
            6 => 'imscp',
            7 => 'label',
            8 => 'page',
            9 => 'url',
            10 => '2',
            11 => 'assign',
            12 => 'chat',
            13 => 'choice',
            14 => 'data',
            15 => 'lti',
            16 => 'forum',
            17 => 'glossary',
            18 => 'lesson',
            19 => 'quiz',
            20 => 'scorm',
            21 => 'survey',
            22 => 'wiki',
            23 => 'workshop',
            24 => 'qtype',
            25 => 'qtype_%',
            26 => '15',
            27 => '/1/3/15/%',
            28 => '1',
            29 => '2',
            30 => '1',
            31 => '1',
            32 => '1',
            33 => 'KS4',
            34 => 'Computing',
            35 => 0,
            36 => 1,
            )]
            Error code: dmlreadexception
            Stack trace:
            line 448 of /lib/dml/moodle_database.php: dml_read_exception thrown
            line 242 of /lib/dml/mssql_native_moodle_database.php: call to moodle_database->query_end()
            line 715 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->query_end()
            line 749 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->get_recordset_sql()
            line 1419 of /lib/tablelib.php: call to mssql_native_moodle_database->get_records_sql()
            line 1439 of /lib/tablelib.php: call to table_sql->query_db()
            line 165 of /outcome/renderer.php: call to table_sql->out()
            line 221 of /outcome/classes/controller/report_controller.php: call to core_outcome_renderer->outcome_course_coverage()
            line ? of unknownfile: call to core_outcome\controller\report_controller->report_course_coverage_table_action()
            line 149 of /outcome/classes/controller/kernel.php: call to call_user_func()
            line 110 of /outcome/classes/controller/kernel.php: call to core_outcome\controller\kernel->execute_callback()
            line 53 of /outcome/course.php: call to core_outcome\controller\kernel->handle()
            

            Show
            Dan Poltawski added a comment - Hi Everyone, I'm afraid I think that we have just a bit too many issues with this remaining in order to integrate this week. Overall I think the code is looking great, but breakage on some databases and the clean theme UI breing broken makes this just a bit too much. I also want to clarify with MD a few points. Here is my summmary: Issues to fix before integration Cross-DB compatibilty. The code is breaking all over the place on MSSQL (see examples below). UI is broken on the clean theme (bootstrapbase needs addressing). Strings need ordering and AMOS CPY commands so translators don't need to retranslate everything. core_component_testcase::test_get_core_subsystems Clarifcations for integration I feel very unusure about these top level fields on the outcome sets and outcomes, I see its been in the spec. So I just want to get a +1 about the pieces from MD to confirm. Examples: I find it strange that we will be storing things like region within an individual moodle install, it seems very niche to me to need to be able to distinguish. The need to add unique ids and doc number seems cumbersome. It almost seems like its only geared up towards importing outcome sets frm standards. It might just be that these fields would make more sense to me if they were named like standard moodle names (like idnumber, shortname etc). I want to confirm the upgrade/migration situation with MD, I am not certain the situation we are ending up with here was clear from the spec. Testing plan. Niggling UI things spotted in brief testing: The outcomes link appears in navigation before outcomes are mapped to the course, and the only way to get to it is to go to the course settings. I don't think we should have dead links like that. The outcomes selection pane is available on activites before outcomes are mapped to a course and it doesn't give a clear idea of how to solve that problem. The outcomes admin page: It seems like level/subject should be displayed on that tree structure, else its hard to see that information at a glance. The interface for rearranging the outcomes in the tree structure seems hard to understand Going to Course outcome reports leads to dead ends with error messages when students aren't enrolled. – MSSQL errors Adding outcome to activity: Column 'mdl_outcome.outcomesetid' is invalid in the select list because it is not contained in either an aggregate function or the GROUP BY clause. SELECT o.* FROM mdl_outcome o LEFT JOIN mdl_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = ?) LEFT JOIN mdl_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = ?) WHERE o.outcomesetid = ? AND (edulevels.value = ? AND subjects.value = ?) AND o.deleted = ? AND o.assessable = ? GROUP BY o.id [array ( 0 => 'edulevels', 1 => 'subjects', 2 => '1', 3 => 'KS4', 4 => 'Computing', 5 => 0, 6 => 1, )] Error code: dmlreadexception Stack trace: * line 448 of /lib/dml/moodle_database.php: dml_read_exception thrown * line 242 of /lib/dml/mssql_native_moodle_database.php: call to moodle_database->query_end() * line 715 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->query_end() * line 221 of /outcome/classes/model/outcome_repository.php: call to mssql_native_moodle_database->get_recordset_sql() * line 188 of /outcome/classes/controller/mapping_ajax_controller.php: call to core_outcome\model\outcome_repository->find_by_filter() * line ? of unknownfile: call to core_outcome\controller\mapping_ajax_controller->get_mappable_outcomes_action() * line 149 of /outcome/classes/controller/kernel.php: call to call_user_func() * line 110 of /outcome/classes/controller/kernel.php: call to core_outcome\controller\kernel->execute_callback() * line 58 of /outcome/ajax.php: call to core_outcome\controller\kernel->handle() On umapped activities and quiz questions: Cannot resolve the collation conflict between "Latin1_General_BIN" and "Latin1_General_CS_AS" in the equal to operation.   SELECT cm.id, COUNT(u.id) areacount FROM mdl_course_modules cm INNER JOIN mdl_modules mods ON mods.id = cm.module AND mods.visible = ? LEFT JOIN mdl_outcome_areas a ON a.itemid = cm.id AND a.area = ? AND component = CAST(? AS NVARCHAR(255)) + CAST(mods.name AS NVARCHAR(255)) LEFT JOIN mdl_outcome_used_areas u ON u.cmid = cm.id WHERE cm.course = ? AND a.id IS NULL GROUP BY cm.id [array ( 0 => 1, 1 => 'mod', 2 => 'mod_', 3 => '2', )] Error code: dmlreadexception   Stack trace: line 448 of /lib/dml/moodle_database.php: dml_read_exception thrown line 242 of /lib/dml/mssql_native_moodle_database.php: call to moodle_database->query_end() line 715 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->query_end() line 749 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->get_recordset_sql() line 61 of /outcome/support/mod/classes/coverage.php: call to mssql_native_moodle_database->get_records_sql() line 113 of /outcome/support/mod/classes/coverage.php: call to outcomesupport_mod\coverage->get_unmapped_activities() line 174 of /outcome/renderer.php: call to outcomesupport_mod\coverage->get_unmapped_content() line 241 of /outcome/classes/controller/report_controller.php: call to core_outcome_renderer->outcome_course_unmapped() line ? of unknownfile: call to core_outcome\controller\report_controller->report_course_unmapped_table_action() line 149 of /outcome/classes/controller/kernel.php: call to call_user_func() line 110 of /outcome/classes/controller/kernel.php: call to core_outcome\controller\kernel->execute_callback() line 53 of /outcome/course.php: call to core_outcome\controller\kernel->handle() On completion marking: Debug info: Column 'mdl_outcome.docnum' is invalid in the select list because it is not contained in either an aggregate function or the GROUP BY clause. SELECT TOP 50 o.id, o.docnum, o.description, t1.resources, t2.activities, t3.questions, t3.questionsused FROM mdl_outcome o LEFT JOIN mdl_outcome_metadata edulevels ON (o.id = edulevels.outcomeid AND edulevels.name = ?) LEFT JOIN mdl_outcome_metadata subjects ON (o.id = subjects.outcomeid AND subjects.name = ?) LEFT JOIN ( SELECT o.id, COUNT(used.id) resources FROM mdl_outcome o INNER JOIN mdl_outcome_area_outcomes ao ON o.id = ao.outcomeid INNER JOIN mdl_outcome_areas areas ON areas.id = ao.outcomeareaid INNER JOIN mdl_outcome_used_areas used ON areas.id = used.outcomeareaid INNER JOIN mdl_course_modules cm ON cm.id = used.cmid AND cm.course = ? INNER JOIN mdl_modules mods ON mods.id = cm.module AND mods.name IN (?,?,?,?,?,?,?) GROUP BY o.id ) t1 ON t1.id = o.id   LEFT JOIN ( SELECT o.id, COUNT(DISTINCT cm2.id) activities FROM mdl_outcome o INNER JOIN mdl_outcome_area_outcomes ao ON o.id = ao.outcomeid INNER JOIN mdl_outcome_areas areas2 ON areas2.id = ao.outcomeareaid INNER JOIN mdl_outcome_used_areas used2 ON areas2.id = used2.outcomeareaid INNER JOIN mdl_course_modules cm2 ON cm2.id = used2.cmid AND cm2.course = ? INNER JOIN mdl_modules mods2 ON mods2.id = cm2.module AND mods2.name IN (?,?,?,?,?,?,?,?,?,?,?,?,?) GROUP BY o.id ) t2 ON t2.id = o.id   LEFT JOIN ( SELECT o.id, COUNT(DISTINCT ao.id) questions, COUNT(DISTINCT cm3.id) questionsused FROM mdl_outcome o INNER JOIN mdl_outcome_area_outcomes ao ON o.id = ao.outcomeid INNER JOIN mdl_outcome_areas areas3 ON areas3.id = ao.outcomeareaid AND areas3.area = ? AND areas3.component COLLATE Latin1_General_CS_AS LIKE ? ESCAPE '\' INNER JOIN mdl_question q ON q.id = areas3.itemid INNER JOIN mdl_question_categories qc ON qc.id = q.category INNER JOIN mdl_context ctx ON (ctx.id = qc.contextid AND (ctx.id = ? OR ctx.path COLLATE Latin1_General_CS_AS LIKE ? ESCAPE '\' OR ctx.id = ?)) LEFT JOIN mdl_outcome_used_areas used3 ON areas3.id = used3.outcomeareaid LEFT JOIN mdl_course_modules cm3 ON cm3.id = used3.cmid AND cm3.course = ? WHERE (ctx.id != ? OR (ctx.id = ? AND cm3.id IS NOT NULL)) GROUP BY o.id ) t3 ON t3.id = o.id   WHERE o.outcomesetid = ? AND (edulevels.value = ? AND subjects.value = ?) AND o.deleted = ? AND o.assessable = ? GROUP BY o.id, t1.resources, t2.activities, t3.questions, t3.questionsused ORDER BY description ASC [array ( 0 => 'edulevels', 1 => 'subjects', 2 => '2', 3 => 'book', 4 => 'resource', 5 => 'folder', 6 => 'imscp', 7 => 'label', 8 => 'page', 9 => 'url', 10 => '2', 11 => 'assign', 12 => 'chat', 13 => 'choice', 14 => 'data', 15 => 'lti', 16 => 'forum', 17 => 'glossary', 18 => 'lesson', 19 => 'quiz', 20 => 'scorm', 21 => 'survey', 22 => 'wiki', 23 => 'workshop', 24 => 'qtype', 25 => 'qtype_%', 26 => '15', 27 => '/1/3/15/%', 28 => '1', 29 => '2', 30 => '1', 31 => '1', 32 => '1', 33 => 'KS4', 34 => 'Computing', 35 => 0, 36 => 1, )] Error code: dmlreadexception Stack trace: line 448 of /lib/dml/moodle_database.php: dml_read_exception thrown line 242 of /lib/dml/mssql_native_moodle_database.php: call to moodle_database->query_end() line 715 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->query_end() line 749 of /lib/dml/mssql_native_moodle_database.php: call to mssql_native_moodle_database->get_recordset_sql() line 1419 of /lib/tablelib.php: call to mssql_native_moodle_database->get_records_sql() line 1439 of /lib/tablelib.php: call to table_sql->query_db() line 165 of /outcome/renderer.php: call to table_sql->out() line 221 of /outcome/classes/controller/report_controller.php: call to core_outcome_renderer->outcome_course_coverage() line ? of unknownfile: call to core_outcome\controller\report_controller->report_course_coverage_table_action() line 149 of /outcome/classes/controller/kernel.php: call to call_user_func() line 110 of /outcome/classes/controller/kernel.php: call to core_outcome\controller\kernel->execute_callback() line 53 of /outcome/course.php: call to core_outcome\controller\kernel->handle()
            Hide
            CiBoT added a comment -

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

            Show
            CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            Andrew Davis added a comment - - edited

            There is an "Error reading from database" error thrown when restoring a course containing outcomes. I have been able to narrow down what is causing the error. As soon as I add an outcome to the course (even if its not associated with any activities) if you backup then restore into a new course the restore will fail.

            UPDATE: I had just done a fresh install and forgot to turn on debugging. There is more info. I'm trying to fix this now.

            Debug info: ERROR: invalid input syntax for integer: ""
            SELECT * FROM outcomes4_backup_ids_temp WHERE backupid = $1 AND itemname = $2 AND itemid = $3
            [array (
            0 => 'ac390b980e41b4eac41a1f8c0fe1cc18',
            1 => 'outcome_used_set',
            2 => '',
            )]
            Error code: dmlreadexception
            Stack trace:

            line 448 of /lib/dml/moodle_database.php: dml_read_exception thrown
            line 239 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
            line 743 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
            line 1437 of /lib/dml/moodle_database.php: call to pgsql_native_moodle_database->get_records_sql()
            line 1409 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql()
            line 1388 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
            line 273 of /backup/util/dbops/restore_dbops.class.php: call to moodle_database->get_record()
            line 1573 of /backup/util/dbops/restore_dbops.class.php: call to restore_dbops::set_backup_ids_cached()
            line 181 of /backup/util/plan/restore_structure_step.class.php: call to restore_dbops::set_backup_ids_record()
            line 1727 of /backup/moodle2/restore_stepslib.php: call to restore_structure_step->set_mapping()
            line 137 of /backup/util/plan/restore_structure_step.class.php: call to restore_course_outcome_structure_step->process_outcome_used_set()
            line 103 of /backup/util/helper/restore_structure_parser_processor.class.php: call to restore_structure_step->process()
            line 151 of /backup/util/xml/parser/processors/grouped_parser_processor.class.php: call to restore_structure_parser_processor->dispatch_chunk()
            line 91 of /backup/util/helper/restore_structure_parser_processor.class.php: call to grouped_parser_processor->postprocess_chunk()
            line 148 of /backup/util/xml/parser/processors/simplified_parser_processor.class.php: call to restore_structure_parser_processor->postprocess_chunk()
            line 92 of /backup/util/xml/parser/processors/progressive_parser_processor.class.php: call to simplified_parser_processor->process_chunk()
            line 186 of /backup/util/xml/parser/progressive_parser.class.php: call to progressive_parser_processor->receive_chunk()
            line 274 of /backup/util/xml/parser/progressive_parser.class.php: call to progressive_parser->publish()
            line ? of unknownfile: call to progressive_parser->end_tag()
            line 175 of /backup/util/xml/parser/progressive_parser.class.php: call to xml_parse()
            line 154 of /backup/util/xml/parser/progressive_parser.class.php: call to progressive_parser->parse()
            line 110 of /backup/util/plan/restore_structure_step.class.php: call to progressive_parser->process()
            line 181 of /backup/util/plan/base_task.class.php: call to restore_structure_step->execute()
            line 177 of /backup/util/plan/base_plan.class.php: call to base_task->execute()
            line 167 of /backup/util/plan/restore_plan.class.php: call to base_plan->execute()
            line 343 of /backup/controller/restore_controller.class.php: call to restore_plan->execute()
            line 184 of /backup/util/ui/restore_ui.class.php: call to restore_controller->execute_plan()
            line 77 of /backup/restore.php: call to restore_ui->execute()

            Show
            Andrew Davis added a comment - - edited There is an "Error reading from database" error thrown when restoring a course containing outcomes. I have been able to narrow down what is causing the error. As soon as I add an outcome to the course (even if its not associated with any activities) if you backup then restore into a new course the restore will fail. UPDATE: I had just done a fresh install and forgot to turn on debugging. There is more info. I'm trying to fix this now. Debug info: ERROR: invalid input syntax for integer: "" SELECT * FROM outcomes4_backup_ids_temp WHERE backupid = $1 AND itemname = $2 AND itemid = $3 [array ( 0 => 'ac390b980e41b4eac41a1f8c0fe1cc18', 1 => 'outcome_used_set', 2 => '', )] Error code: dmlreadexception Stack trace: line 448 of /lib/dml/moodle_database.php: dml_read_exception thrown line 239 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end() line 743 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end() line 1437 of /lib/dml/moodle_database.php: call to pgsql_native_moodle_database->get_records_sql() line 1409 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql() line 1388 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select() line 273 of /backup/util/dbops/restore_dbops.class.php: call to moodle_database->get_record() line 1573 of /backup/util/dbops/restore_dbops.class.php: call to restore_dbops::set_backup_ids_cached() line 181 of /backup/util/plan/restore_structure_step.class.php: call to restore_dbops::set_backup_ids_record() line 1727 of /backup/moodle2/restore_stepslib.php: call to restore_structure_step->set_mapping() line 137 of /backup/util/plan/restore_structure_step.class.php: call to restore_course_outcome_structure_step->process_outcome_used_set() line 103 of /backup/util/helper/restore_structure_parser_processor.class.php: call to restore_structure_step->process() line 151 of /backup/util/xml/parser/processors/grouped_parser_processor.class.php: call to restore_structure_parser_processor->dispatch_chunk() line 91 of /backup/util/helper/restore_structure_parser_processor.class.php: call to grouped_parser_processor->postprocess_chunk() line 148 of /backup/util/xml/parser/processors/simplified_parser_processor.class.php: call to restore_structure_parser_processor->postprocess_chunk() line 92 of /backup/util/xml/parser/processors/progressive_parser_processor.class.php: call to simplified_parser_processor->process_chunk() line 186 of /backup/util/xml/parser/progressive_parser.class.php: call to progressive_parser_processor->receive_chunk() line 274 of /backup/util/xml/parser/progressive_parser.class.php: call to progressive_parser->publish() line ? of unknownfile: call to progressive_parser->end_tag() line 175 of /backup/util/xml/parser/progressive_parser.class.php: call to xml_parse() line 154 of /backup/util/xml/parser/progressive_parser.class.php: call to progressive_parser->parse() line 110 of /backup/util/plan/restore_structure_step.class.php: call to progressive_parser->process() line 181 of /backup/util/plan/base_task.class.php: call to restore_structure_step->execute() line 177 of /backup/util/plan/base_plan.class.php: call to base_task->execute() line 167 of /backup/util/plan/restore_plan.class.php: call to base_plan->execute() line 343 of /backup/controller/restore_controller.class.php: call to restore_plan->execute() line 184 of /backup/util/ui/restore_ui.class.php: call to restore_controller->execute_plan() line 77 of /backup/restore.php: call to restore_ui->execute()
            Hide
            Andrew Davis added a comment -

            That took a long time to track down but the fix was simple. Outcome set IDs weren't being put in the backup file. Here is the fix. https://github.com/andyjdavis/moodle/commit/f722316eb391504a5e3b928e0f7da24958e354b2

            Show
            Andrew Davis added a comment - That took a long time to track down but the fix was simple. Outcome set IDs weren't being put in the backup file. Here is the fix. https://github.com/andyjdavis/moodle/commit/f722316eb391504a5e3b928e0f7da24958e354b2
            Hide
            Andrew Davis added a comment -

            Has there been any progress on this? I installed MS SQL over the weekend and will try to figure out the MS SQL failures if no one else has.

            Show
            Andrew Davis added a comment - Has there been any progress on this? I installed MS SQL over the weekend and will try to figure out the MS SQL failures if no one else has.
            Hide
            Martin Dougiamas added a comment -

            After looking closely at this today there are some rather serious problems even beyond what Dan mentioned above that are currently blockers for integration. More details here soon.

            Show
            Martin Dougiamas added a comment - After looking closely at this today there are some rather serious problems even beyond what Dan mentioned above that are currently blockers for integration. More details here soon.
            Hide
            Andrew Davis added a comment -

            After a review by the front end team there is a big list of stuff to do. We're going to divert members of the front end team to resolve as many of these as possible.

            The biggest two items:

            1) Errors under MS SQL. I'm trying to resolve these now.

            2) The relationship between the assignment outcomes and rubric outcomes needs to be clarified. It makes sense that, when creating a rubric, you should be limited to the outcomes selected for the assignment however the rubric may be used in multiple assignments... At a minimum the user should be warned if they select an outcome for the rubric that is not associated with the activity. Quite likely they should receive an error. Should outcomes associations simply be copied from the rubric to the associated activity?

            The rest:

            The unique ID help string (both for outcome set and individual outcome) is unclear.

            The outcome assessable help item is unclear.

            In Clean, the create outcome dialogue is skinny.

            In Clean, the outcome hierarchy does not display correctly. (You can use an outcome set from http://asn.jesandco.org/resources/ASNJurisdiction/CCSS to test)

            Import format is missing help. What is Moodle general format?

            In assignment settings:

            • the outcome set(s) display collapsed initially
            • dialogue cannot be moved or resized
            • no sorting or filtering of the available outcomes (trying to find a particular outcome from a large set is horrible)
            • no select all/none

            On the rubric creation page, the scrollbar jumps down the page after selecting an outcome.

            Outcomes do not appear on the assignment grading page.

            The "associated content" type dialogues, for example the pop ups on the coverage report:

            • No way to click through to the activities etc.
            • Do we even need a popup Vs just listing things in the table?
            Show
            Andrew Davis added a comment - After a review by the front end team there is a big list of stuff to do. We're going to divert members of the front end team to resolve as many of these as possible. The biggest two items: 1) Errors under MS SQL. I'm trying to resolve these now. 2) The relationship between the assignment outcomes and rubric outcomes needs to be clarified. It makes sense that, when creating a rubric, you should be limited to the outcomes selected for the assignment however the rubric may be used in multiple assignments... At a minimum the user should be warned if they select an outcome for the rubric that is not associated with the activity. Quite likely they should receive an error. Should outcomes associations simply be copied from the rubric to the associated activity? The rest: The unique ID help string (both for outcome set and individual outcome) is unclear. The outcome assessable help item is unclear. In Clean, the create outcome dialogue is skinny. In Clean, the outcome hierarchy does not display correctly. (You can use an outcome set from http://asn.jesandco.org/resources/ASNJurisdiction/CCSS to test) Import format is missing help. What is Moodle general format? In assignment settings: the outcome set(s) display collapsed initially dialogue cannot be moved or resized no sorting or filtering of the available outcomes (trying to find a particular outcome from a large set is horrible) no select all/none On the rubric creation page, the scrollbar jumps down the page after selecting an outcome. Outcomes do not appear on the assignment grading page. The "associated content" type dialogues, for example the pop ups on the coverage report: No way to click through to the activities etc. Do we even need a popup Vs just listing things in the table?
            Hide
            Rossiani Wijaya added a comment -

            Additional note from Andrew's list above.

            Outcome settings:

            • Need styling improvement in Clean theme, such as adding icon to action.

            In assignment settings:

            • add cursor feedback
            • make the list moveable
            • change dialogbox title to 'Add outcome sets'

            On report page:

            • the last column should just use 'associate content' as header, instead of string for each column.
            • check report table accessibility.

            Some suggestion during scrum:

            • allow adding multiple outcome at once for rubric critrion
            • outcome items should be removed from list once it has been added to criterion in rubric.
            • in course outcome settings , if subject list only contain 1 item, it should be the selected by default.
            • in course outcome settings, change the string for add button to save.

            Add link to Outcome documentations.

            Show
            Rossiani Wijaya added a comment - Additional note from Andrew's list above. Outcome settings: Need styling improvement in Clean theme, such as adding icon to action. In assignment settings: add cursor feedback make the list moveable change dialogbox title to 'Add outcome sets' On report page: the last column should just use 'associate content' as header, instead of string for each column. check report table accessibility. Some suggestion during scrum: allow adding multiple outcome at once for rubric critrion outcome items should be removed from list once it has been added to criterion in rubric. in course outcome settings , if subject list only contain 1 item, it should be the selected by default. in course outcome settings, change the string for add button to save. Add link to Outcome documentations.
            Hide
            Andrew Nicols added a comment -

            Here's my notes from that meeting - apologies if these clash with Andrew Davis (or Rossiani Wijaya's):

            Various issues with theme clean:
            • icons missing in the Add outcome screens (see )
            • dialogue width too narrow
            • dialogue heading cut off (see )
            Outcomes management
            • Form change detection missing - you can make selections for outcomes, and accidentally navigate away
              • "Add new outcome set Import outcome set" - confusing (see )
            • Is it possible to select the actions (add, edit, delete) with a touchscreen device? (The action links are hidden) (see )
            • Interface generally confusing in some situations
            • Could do with an introduction at the intro screen ()
            • Action links table column duplicate the content (find a better way?) ()
            • Title for "Mapped courses" longer than dialogue ()
            • Critical: It's possible to delete an entire outcome without any confirmation! We should perhaps prevent deletion if the outcome has any uses at all. (You can undo, but only until you navigate away)
            • When defining a new Outcome set, if you add an outcome at the same time, you can define an outcome with the same unique ID as the outcome set. You can't do this when defining a new outcome after saving the outcome set ()
            • No indication of required fields when adding a new outcome (Unable to render embedded object: File (management-outcome-add_required.png) not found.)
            • Use of "Doc number" and "Description" in outcome hierarchy is potentially confusing and they're not separated from one another (, , , )
            Import screens
            • Can we not autodetect the format
            • Check: What happens if you specify one format and use another?
            • Missing help on import screen to explain:
              • file formats
            Outcome selection
            • Course level:
              • Add button confusing - it's for adding an additional not for adding the current
              • Lexical sorting for third field at course level selection
            • Activity level:
              • Two ways to select outcomes - both in the Grading Rubrics, and in the Outcomes section (Very confusing)
              • Would be nice to have a "Select all/none" option
              • Sorting - can this be improved?
              • No indication that items are selectable at all (e.g. empty tickbox vs. ticked tickbox) (see )
            Accessibility
            • Missing hover changes
            • Use of colours to indicate enabled/disabled - affects colour blind users who do not otherwise use accessibility tools like screen readers
            Miscellaneous Dialogue issues
            • Missing drag/drop of dialogues
            • Some dialogues missing resize
            • Ok/Cancel => Unclear as to which is which sometimes. Perhaps change to 'Save / Cancel' or more appropriate
            • Dialogue should be full screen for selection?
            • Issues with small screens (11" MacbookAir for example) (see )
            • Some dialogues opening seems to cause the scroll position to change
            Rubrics
            • Should the rubric selection allow you to select multiple outcomes and create rows for all that you select rather than present you with a dialogue you need to keep opening and closing
            • Rubrics should inherit from assignment's outcome settings by default
            • It's possible to apply a rubric which has unrelated outcomes to work (e.g. a rubric template with outcomes from Kindergarten Maths can be applied to an assignment which is limited to 11th Grade Maths)
            • CHECK: Are rubric settings provided in any of the formats, and if so why aren't we using them?
            Help files and language strings
            • Need review / rewrite / clarification in various places
            • "General Moodle Format" - what is this and is naming appropriate?
            • "Select outcomes" plural used, even when screen only accepts a single outcome
            • "Unique ID" help confusing and re-used between main outcome name, and it's children (is this intentional or confusing?)
            Reports
            • "Associated content (X)" - can it just be a number? (see , and )
            • Associated content's dialogue - "Content" field (see )
            Miscellaneous
            • Should Outcomes be in a separate mod_form setting to Grades? (needs clarification)
            • CHECK RTL compatibility
            Show
            Andrew Nicols added a comment - Here's my notes from that meeting - apologies if these clash with Andrew Davis (or Rossiani Wijaya 's): Various issues with theme clean: icons missing in the Add outcome screens (see ) dialogue width too narrow dialogue heading cut off (see ) Outcomes management Form change detection missing - you can make selections for outcomes, and accidentally navigate away "Add new outcome set Import outcome set" - confusing (see ) Is it possible to select the actions (add, edit, delete) with a touchscreen device? (The action links are hidden) (see ) Interface generally confusing in some situations Could do with an introduction at the intro screen ( ) Action links table column duplicate the content (find a better way?) ( ) Title for "Mapped courses" longer than dialogue ( ) Critical: It's possible to delete an entire outcome without any confirmation! We should perhaps prevent deletion if the outcome has any uses at all. (You can undo, but only until you navigate away) When defining a new Outcome set, if you add an outcome at the same time, you can define an outcome with the same unique ID as the outcome set. You can't do this when defining a new outcome after saving the outcome set ( ) No indication of required fields when adding a new outcome ( Unable to render embedded object: File (management-outcome-add_required.png) not found. ) Use of "Doc number" and "Description" in outcome hierarchy is potentially confusing and they're not separated from one another ( , , , ) Import screens Can we not autodetect the format Check: What happens if you specify one format and use another? Missing help on import screen to explain: file formats Outcome selection Course level: Add button confusing - it's for adding an additional not for adding the current Lexical sorting for third field at course level selection Activity level: Two ways to select outcomes - both in the Grading Rubrics, and in the Outcomes section (Very confusing) Would be nice to have a "Select all/none" option Sorting - can this be improved? No indication that items are selectable at all (e.g. empty tickbox vs. ticked tickbox) (see ) Accessibility Missing hover changes Use of colours to indicate enabled/disabled - affects colour blind users who do not otherwise use accessibility tools like screen readers Miscellaneous Dialogue issues Missing drag/drop of dialogues Some dialogues missing resize Ok/Cancel => Unclear as to which is which sometimes. Perhaps change to 'Save / Cancel' or more appropriate Dialogue should be full screen for selection? Issues with small screens (11" MacbookAir for example) (see ) Some dialogues opening seems to cause the scroll position to change Rubrics Should the rubric selection allow you to select multiple outcomes and create rows for all that you select rather than present you with a dialogue you need to keep opening and closing Rubrics should inherit from assignment's outcome settings by default It's possible to apply a rubric which has unrelated outcomes to work (e.g. a rubric template with outcomes from Kindergarten Maths can be applied to an assignment which is limited to 11th Grade Maths) CHECK: Are rubric settings provided in any of the formats, and if so why aren't we using them? Help files and language strings Need review / rewrite / clarification in various places "General Moodle Format" - what is this and is naming appropriate? "Select outcomes" plural used, even when screen only accepts a single outcome "Unique ID" help confusing and re-used between main outcome name, and it's children (is this intentional or confusing?) Reports "Associated content (X)" - can it just be a number? (see , and ) Associated content's dialogue - "Content" field (see ) Miscellaneous Should Outcomes be in a separate mod_form setting to Grades? (needs clarification) CHECK RTL compatibility
            Hide
            Tim Hunt added a comment -

            Re "no sorting or filtering of the available outcomes (trying to find a particular outcome from a large set is horrible)"

            If these are being selected from a standard HTML select, then you might want to steal my filtering code from https://github.com/moodleou/moodle-tool_editrolesbycap. Specifically https://github.com/moodleou/moodle-tool_editrolesbycap/blob/master/yui/capabilityformfield/capabilityformfield.js

            If it in not a standard HTML select, then you might still be able to reuse part of it.

            Show
            Tim Hunt added a comment - Re "no sorting or filtering of the available outcomes (trying to find a particular outcome from a large set is horrible)" If these are being selected from a standard HTML select, then you might want to steal my filtering code from https://github.com/moodleou/moodle-tool_editrolesbycap . Specifically https://github.com/moodleou/moodle-tool_editrolesbycap/blob/master/yui/capabilityformfield/capabilityformfield.js If it in not a standard HTML select, then you might still be able to reuse part of it.
            Hide
            Andrew Nicols added a comment -

            Cheers Tim - I was also wondering about whether http://yuilibrary.com/yui/docs/api/classes/AutoCompleteList.html may be suitable. There's a demo of that in MDL-37325.

            Show
            Andrew Nicols added a comment - Cheers Tim - I was also wondering about whether http://yuilibrary.com/yui/docs/api/classes/AutoCompleteList.html may be suitable. There's a demo of that in MDL-37325 .
            Hide
            Andrew Nicols added a comment -

            Adding screenshot to demonstrate the lack of colour acuity for selection:

            Show
            Andrew Nicols added a comment - Adding screenshot to demonstrate the lack of colour acuity for selection:
            Hide
            Andrew Nicols added a comment -
            Outcome management:
            • Can only click on the icon to expand categories, not the text;
            • 'Doc number' is unclear - needs renaming? Comes from http://standards.jesandco.org/wiki/ASN_Vocabulary#Statement_Notation
            • Would be nice to display what you're adding an out come to in the dialogue
            • Move system is clunky
            • What format is the export? Is this the 'General Moodle Format'?
            • Adding an outcome set to a course does not trigger formchangechecker
            Course module -> Edit settings -> Select outcomes
            • Tab takes you to the next outcome; enter selects - how does this work for accessibility. Selecting the 'OK/Save' button whilst in the middle of a list?
            Course -> Edit settings -> Outcomes
            • list of outcome sets is curtailed ()
            • list of education levels not lexically sorted ()
            Course administration -> Outcomes
            • Why are reports listed in Outcomes instead of Reports?
            • No explanation of the various reports (e.g. Completion - what's it for?)
            Course administration -> Outcomes -> Completion marking
            • Users are not lexically sorted in filter list ()
            Course administration -> Outcomes -> Coverage
            • Should Questions which are included in a quiz also appear in the Questions section e.g. if you apply outcomes to a whole quiz, should that not cover every question within that quiz? Questions are included twice if the question has outcomes against ir ather than the quiz.
            • Explanation of italicised 'Content' in the 'Activities' popup () - it seems to mean that the quiz contains content which has an outcome but that the quiz itself does not but there's no key
            Course administration -> Outcomes -> Unmapped content items and quiz questions
            • No H1 level heading on the 'Unmapped content items and quiz questions' page
            • Mapping an unmapped item takes you to the cmod editing page - unclear as to intention
            • 'Nothing to display' - unclear. Also centered for some reason
            Show
            Andrew Nicols added a comment - Outcome management: Can only click on the icon to expand categories, not the text; 'Doc number' is unclear - needs renaming? Comes from http://standards.jesandco.org/wiki/ASN_Vocabulary#Statement_Notation Would be nice to display what you're adding an out come to in the dialogue Move system is clunky What format is the export? Is this the 'General Moodle Format'? Adding an outcome set to a course does not trigger formchangechecker Course module -> Edit settings -> Select outcomes Tab takes you to the next outcome; enter selects - how does this work for accessibility. Selecting the 'OK/Save' button whilst in the middle of a list? Course -> Edit settings -> Outcomes list of outcome sets is curtailed ( ) list of education levels not lexically sorted ( ) Course administration -> Outcomes Why are reports listed in Outcomes instead of Reports? No explanation of the various reports (e.g. Completion - what's it for?) Course administration -> Outcomes -> Completion marking Users are not lexically sorted in filter list ( ) Course administration -> Outcomes -> Coverage Should Questions which are included in a quiz also appear in the Questions section e.g. if you apply outcomes to a whole quiz, should that not cover every question within that quiz? Questions are included twice if the question has outcomes against ir ather than the quiz. Explanation of italicised 'Content' in the 'Activities' popup ( ) - it seems to mean that the quiz contains content which has an outcome but that the quiz itself does not but there's no key Course administration -> Outcomes -> Unmapped content items and quiz questions No H1 level heading on the 'Unmapped content items and quiz questions' page Mapping an unmapped item takes you to the cmod editing page - unclear as to intention 'Nothing to display' - unclear. Also centered for some reason
            Hide
            Andrew Nicols added a comment -

            Just pointing out that some of the things we're raising are still listed as open in http://docs.moodle.org/dev/Outcomes_Specification_Change_Log but they don't seem to be covered by either stage 1 or stage 2.

            That said, Stage 2 (which should be covered by this issue) is meant to include things like Student views (see http://docs.moodle.org/dev/Outcomes_Student_Specification) but I can't see any of that interface in Moodle as a student. I see looking at the comments on this issue that Andrew Davis suggests access to the reports but Mark Nielsen rejects the change in the following comment (for valid reasons). Should it still be included in the specification then?

            Show
            Andrew Nicols added a comment - Just pointing out that some of the things we're raising are still listed as open in http://docs.moodle.org/dev/Outcomes_Specification_Change_Log but they don't seem to be covered by either stage 1 or stage 2. That said, Stage 2 (which should be covered by this issue) is meant to include things like Student views (see http://docs.moodle.org/dev/Outcomes_Student_Specification ) but I can't see any of that interface in Moodle as a student. I see looking at the comments on this issue that Andrew Davis suggests access to the reports but Mark Nielsen rejects the change in the following comment (for valid reasons). Should it still be included in the specification then?
            Hide
            Martin Dougiamas added a comment -

            Sorry for the flurry of comments above from separate people, I expected FRONTEND guys to merge these into one list. I'll collate them into a single list.

            Also, I'm personally very surprised we've got some of these bigger issues here at this late stage, seems the smaller coding issues took a long time to clear away.

            Show
            Martin Dougiamas added a comment - Sorry for the flurry of comments above from separate people, I expected FRONTEND guys to merge these into one list. I'll collate them into a single list. Also, I'm personally very surprised we've got some of these bigger issues here at this late stage, seems the smaller coding issues took a long time to clear away.
            Hide
            Martin Dougiamas added a comment -

            I'm putting things together for the review in this document here:

            https://docs.google.com/document/d/1I_W6DgURKREYdX7FhF0ftTdmYXG3AUvNwCy4hASqQyk/edit?usp=sharing

            Show
            Martin Dougiamas added a comment - I'm putting things together for the review in this document here: https://docs.google.com/document/d/1I_W6DgURKREYdX7FhF0ftTdmYXG3AUvNwCy4hASqQyk/edit?usp=sharing
            Hide
            Andrew Davis added a comment -

            Here is a fix to make the new outcomes system work under MS SQL. https://github.com/andyjdavis/moodle/commit/2603f5a715bfeaf5b086340a37e33eee602ae72f
            The fix to outcome/classes/model/outcome_repository.php is slightly bizarre as the SQL is used by multiple different functions in queries like "select outcome.* from..." I'll revisit this and see if I can cut that down to a smaller list of specific fields.

            Just so its not lost in the comments, here is the fix for making courses containing outcomes backup and restore successfully. https://github.com/andyjdavis/moodle/commit/f722316eb391504a5e3b928e0f7da24958e354b2

            I'll get the outcomes branch rebased onto latest master and provide a branch that others can pull to assist with bug fixing.

            Show
            Andrew Davis added a comment - Here is a fix to make the new outcomes system work under MS SQL. https://github.com/andyjdavis/moodle/commit/2603f5a715bfeaf5b086340a37e33eee602ae72f The fix to outcome/classes/model/outcome_repository.php is slightly bizarre as the SQL is used by multiple different functions in queries like "select outcome.* from..." I'll revisit this and see if I can cut that down to a smaller list of specific fields. Just so its not lost in the comments, here is the fix for making courses containing outcomes backup and restore successfully. https://github.com/andyjdavis/moodle/commit/f722316eb391504a5e3b928e0f7da24958e354b2 I'll get the outcomes branch rebased onto latest master and provide a branch that others can pull to assist with bug fixing.
            Hide
            Martin Dougiamas added a comment -

            OK, so after much more thought, review and considerable regret I've decided to bump this out of the 2.6 cycle and into 2.7.

            It is clear now that there is simply too much still to do on this and too little time left before the 2.6 release. A half-baked Outcomes system in 2.6 would not make many people happy, and it gives the team more time now to polish the many other new features.

            Thanks for all the work by all devs on the Outcomes code so far, and sorry to those who were eagerly awaiting these features in 2.6.

            We'll get the whole team stuck into this early in the 2.7 cycle and we'll be able to get a much more comprehensive system into 2.7, one that has a clear UI, automatic outcomes completion, student competency lists and strong connections to badges. You will like it.

            Show
            Martin Dougiamas added a comment - OK, so after much more thought, review and considerable regret I've decided to bump this out of the 2.6 cycle and into 2.7. It is clear now that there is simply too much still to do on this and too little time left before the 2.6 release. A half-baked Outcomes system in 2.6 would not make many people happy, and it gives the team more time now to polish the many other new features. Thanks for all the work by all devs on the Outcomes code so far, and sorry to those who were eagerly awaiting these features in 2.6. We'll get the whole team stuck into this early in the 2.7 cycle and we'll be able to get a much more comprehensive system into 2.7, one that has a clear UI, automatic outcomes completion, student competency lists and strong connections to badges. You will like it.
            Hide
            Mark Nielsen added a comment -

            I think a lot of these theme problems are because I added an outcome.css to the base theme but I didn't add it to the bootstrap theme.

            Show
            Mark Nielsen added a comment - I think a lot of these theme problems are because I added an outcome.css to the base theme but I didn't add it to the bootstrap theme.
            Hide
            Sam Hemelryk added a comment -

            Just noting that I've rebased your work here Mark on top of master (post 2.6 release) and have added a commit to start addressing the issues/notices that are now appearing due to changes landed for 2.6.

            Repo: git://github.com/samhemelryk/moodle.git
            Branch: 40230-27
            Diff: http://icodedthat.com/40230-27

            Changes are as follows:

            • Re-bumped version numbers
            • Split upgrade steps by table for resilience
            • Added plugininfo classes for outcomesuppor, outcomeimport, outcomeexport plugin types.
            • Fixed component unit test count.
            • Fixed usage of fullname without all required fields.

            Will continue to look and play with this over the next couple of days myself as I've been tasked to this issue also.

            Regards
            Sam

            Show
            Sam Hemelryk added a comment - Just noting that I've rebased your work here Mark on top of master (post 2.6 release) and have added a commit to start addressing the issues/notices that are now appearing due to changes landed for 2.6. Repo: git://github.com/samhemelryk/moodle.git Branch: 40230-27 Diff: http://icodedthat.com/40230-27 Changes are as follows: Re-bumped version numbers Split upgrade steps by table for resilience Added plugininfo classes for outcomesuppor, outcomeimport, outcomeexport plugin types. Fixed component unit test count. Fixed usage of fullname without all required fields. Will continue to look and play with this over the next couple of days myself as I've been tasked to this issue also. Regards Sam
            Hide
            Andrew Davis added a comment -

            Here is a document that attempts to detail the remaining work around the outcomes system. http://docs.moodle.org/dev/2.7_Outcomes_scope

            Show
            Andrew Davis added a comment - Here is a document that attempts to detail the remaining work around the outcomes system. http://docs.moodle.org/dev/2.7_Outcomes_scope
            Hide
            Andrew Davis added a comment -

            A minor tweak for consideration.

            There has been some confusion around "Moodle General Format". It is an XML based format for outcome importing and exporting. Here is a commit that renames it to "Moodle XML Format".

            Currently when you export an outcome it automatically gives you an XML file as this is the only export format that exists. If you then go to import your XML file I feel like it would be helpful to have "XML" in the format name.

            https://github.com/andyjdavis/moodle/commit/2951d5311c88d8d456cba5204420c3602a9a6b7d

            Show
            Andrew Davis added a comment - A minor tweak for consideration. There has been some confusion around "Moodle General Format". It is an XML based format for outcome importing and exporting. Here is a commit that renames it to "Moodle XML Format". Currently when you export an outcome it automatically gives you an XML file as this is the only export format that exists. If you then go to import your XML file I feel like it would be helpful to have "XML" in the format name. https://github.com/andyjdavis/moodle/commit/2951d5311c88d8d456cba5204420c3602a9a6b7d
            Hide
            Martin Dougiamas added a comment -

            We already have other XML formats in Moodle.

            Let's call it something like Moodle Outcomes v1.

            If you must, it could be Moodle Outcomes XML v1 with something like a .mox extension.

            Show
            Martin Dougiamas added a comment - We already have other XML formats in Moodle. Let's call it something like Moodle Outcomes v1. If you must, it could be Moodle Outcomes XML v1 with something like a .mox extension.
            Hide
            Jason Fowler added a comment -

            +1 for .mox and Moodle Outcomes XML (MOXML) v1

            Show
            Jason Fowler added a comment - +1 for .mox and Moodle Outcomes XML (MOXML) v1
            Hide
            Jason Fowler added a comment -

            ah, only problem with the .mox extension is if this file is provided by a state school board that hasn't used Moodle to create it, it will likely come as an XML file, with .xml as the extension. This could cause some confusion.

            Show
            Jason Fowler added a comment - ah, only problem with the .mox extension is if this file is provided by a state school board that hasn't used Moodle to create it, it will likely come as an XML file, with .xml as the extension. This could cause some confusion.
            Hide
            Martin Dougiamas added a comment -

            I assume we can accept both for import if that's an issue.

            Show
            Martin Dougiamas added a comment - I assume we can accept both for import if that's an issue.
            Hide
            Sam Hemelryk added a comment -

            Hi Mark,

            I hope its OK I've taken a bit of initiative and have set up a Moodle fork on bitbucket on which we can collaboratively work on the code.

            The three branches I mentioned earlier based upon your work have already being pushed up there as well.
            I've made Andrew an admin on the repo and if you could please let me know your bitbucket account name I'll make you one too. That way we can grant devs write access as they require it and work on this in a central location if you are happy with that.
            If you'd rather work in your own repo thats not a prob, let me know as you make changes (I'll keep an eye out as well) and I'll pull our combined work as required.

            This way when outcomes is made the main focus of the frontend team we'll all have a common place to work from.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Mark, I hope its OK I've taken a bit of initiative and have set up a Moodle fork on bitbucket on which we can collaboratively work on the code. *Repo: https://bitbucket.org/samhemelryk/moodle-outcomes* git remote add bitbucket-outcomes https://bitbucket.org/samhemelryk/moodle-outcomes && git remote set-url --push bitbucket-outcomes git@bitbucket.org:samhemelryk/moodle-outcomes.git The three branches I mentioned earlier based upon your work have already being pushed up there as well. I've made Andrew an admin on the repo and if you could please let me know your bitbucket account name I'll make you one too. That way we can grant devs write access as they require it and work on this in a central location if you are happy with that. If you'd rather work in your own repo thats not a prob, let me know as you make changes (I'll keep an eye out as well) and I'll pull our combined work as required. This way when outcomes is made the main focus of the frontend team we'll all have a common place to work from. Cheers Sam
            Hide
            Martin Dougiamas added a comment -

            Note that I plan to use this to put up a public demo site soon, and to start a discussion on moodle.org to talk more about the relationship between rubrics and outcomes and assessment. I've been looking into it a lot this week and it's a hornet's nest so, I think we really need a lot of feedback from teachers on this stuff.

            Show
            Martin Dougiamas added a comment - Note that I plan to use this to put up a public demo site soon, and to start a discussion on moodle.org to talk more about the relationship between rubrics and outcomes and assessment. I've been looking into it a lot this week and it's a hornet's nest so, I think we really need a lot of feedback from teachers on this stuff.
            Hide
            Michael Aherne added a comment - - edited

            It would be very useful if the export functionality could support InLOC format: http://wiki.teria.no/display/inloc/Home, since it's a Europe-wide specification.

            Show
            Michael Aherne added a comment - - edited It would be very useful if the export functionality could support InLOC format: http://wiki.teria.no/display/inloc/Home , since it's a Europe-wide specification.
            Hide
            Andrew Davis added a comment -

            Sam and I have been spending time experimenting and discussing this issue. I feel like I am flapping around a bit not really achieving much so I am going to open a bunch of sub-tasks to get things out of my head.

            Show
            Andrew Davis added a comment - Sam and I have been spending time experimenting and discussing this issue. I feel like I am flapping around a bit not really achieving much so I am going to open a bunch of sub-tasks to get things out of my head.
            Hide
            Andrew Davis added a comment -

            I have added sub-tasks that I think cover most of the pending outcomes work. There are quite a few very small issues mentioned above that can be dealt with over time.

            Show
            Andrew Davis added a comment - I have added sub-tasks that I think cover most of the pending outcomes work. There are quite a few very small issues mentioned above that can be dealt with over time.
            Hide
            Mark Pearson added a comment -

            after much more thought, review and considerable regret I've decided to bump this out of the 2.6 cycle and into 2.7.

            So what is the status of Outcomes in the 2.7 code base? I'm looking forward to giving this a whirl but I'm concerned that progress has stalled and I'm wondering whether it'll ready to go.

            Show
            Mark Pearson added a comment - after much more thought, review and considerable regret I've decided to bump this out of the 2.6 cycle and into 2.7. So what is the status of Outcomes in the 2.7 code base? I'm looking forward to giving this a whirl but I'm concerned that progress has stalled and I'm wondering whether it'll ready to go.
            Hide
            Elizabeth Dalton added a comment -

            Apparently Outcomes didn't make it into Moodle 2.7: http://docs.moodle.org/27/en/New_features

            What happened? Is there anything the user/admin community can do to help? This is looking like a really promising feature and I was hoping to be able to use it within the next year.

            Show
            Elizabeth Dalton added a comment - Apparently Outcomes didn't make it into Moodle 2.7: http://docs.moodle.org/27/en/New_features What happened? Is there anything the user/admin community can do to help? This is looking like a really promising feature and I was hoping to be able to use it within the next year.
            Hide
            Mark Pearson added a comment -

            On 14/Oct/13 Martin Dougiamas wrote:

            We'll get the whole team stuck into this early in the 2.7 cycle and we'll be able to get a much more comprehensive system into 2.7, one that has a clear UI, automatic outcomes completion, student competency lists and strong connections to badges. You will like it.

            I'm with Elizabeth. Version 2.7 is released and Outcomes do not seem to be included. What is going on ? As someone wrote elsewhere, let's not make perfection the enemy of the good.

            Show
            Mark Pearson added a comment - On 14/Oct/13 Martin Dougiamas wrote: We'll get the whole team stuck into this early in the 2.7 cycle and we'll be able to get a much more comprehensive system into 2.7, one that has a clear UI, automatic outcomes completion, student competency lists and strong connections to badges. You will like it. I'm with Elizabeth. Version 2.7 is released and Outcomes do not seem to be included. What is going on ? As someone wrote elsewhere, let's not make perfection the enemy of the good.
            Hide
            Andrew Davis added a comment - - edited

            Here is the moodle room's code rebased onto current master. There is also a second commit containing assorted bug fixes that I recorded above. https://github.com/andyjdavis/moodle/compare/master...moodleroomsoutcomes

            Note that I have not yet testedd this new version. There were a very large number of conflicts so the chances of me having performed the rebase flawlessly are slim. In particular there were instances where the outcomes changes included changes to code that was removed as part of 2.7 development.

            When working on this if something is broken it is worth consulting the original diff (https://github.com/moodlerooms/moodle/compare/MDL-40230_outcomes) to see if I have inadvertently removed code that is required.

            There was previously a lot of discussion which ended with the consensus that there was little desire to attach outcomes to rubric items and individual quiz questions and little consensus on how exactly rubric/quiz question level outcomes would integrate with outcomes attached to the activity. I am now planning on producing another commit that strips out that code. My suspicion is that that process will chop out most of the code that might have been damaged during the rebase. We can reexamine the idea of attaching outcomes to rubric items and quiz questions once we have a better idea of how that should work in practice.

            Show
            Andrew Davis added a comment - - edited Here is the moodle room's code rebased onto current master. There is also a second commit containing assorted bug fixes that I recorded above. https://github.com/andyjdavis/moodle/compare/master...moodleroomsoutcomes Note that I have not yet testedd this new version. There were a very large number of conflicts so the chances of me having performed the rebase flawlessly are slim. In particular there were instances where the outcomes changes included changes to code that was removed as part of 2.7 development. When working on this if something is broken it is worth consulting the original diff ( https://github.com/moodlerooms/moodle/compare/MDL-40230_outcomes ) to see if I have inadvertently removed code that is required. There was previously a lot of discussion which ended with the consensus that there was little desire to attach outcomes to rubric items and individual quiz questions and little consensus on how exactly rubric/quiz question level outcomes would integrate with outcomes attached to the activity. I am now planning on producing another commit that strips out that code. My suspicion is that that process will chop out most of the code that might have been damaged during the rebase. We can reexamine the idea of attaching outcomes to rubric items and quiz questions once we have a better idea of how that should work in practice.
            Hide
            Elizabeth Dalton added a comment - - edited

            Pardon, what discussion and consensus by whom? We've been relying on being able to attach outcomes to rubric items in our plans. (More correctly, our understanding was that it would be possible to attach rubric items/rows to outcomes, and build a rubric for an assignment from the items belonging to the outcomes attached to the assignment.)

            Show
            Elizabeth Dalton added a comment - - edited Pardon, what discussion and consensus by whom? We've been relying on being able to attach outcomes to rubric items in our plans. (More correctly, our understanding was that it would be possible to attach rubric items/rows to outcomes, and build a rubric for an assignment from the items belonging to the outcomes attached to the assignment.)
            Hide
            Martin Dougiamas added a comment -

            There's been a little miscommunication here, I think. I think Andrew was simply focussing on the easier parts first and decided to split the code up to help him do that and to push this along.

            No-one's saying that we'll not have outcome links at low levels. It's just that those interfaces are not well worked out at all yet, and more work needs to be done there. The current code allows outcomes to be attached to the Assignment activity itself INDEPENDENTLY of the ones attached to rubrics, which is obviously likely to be highly confusing. This is what stopped Outcomes going straight into core initially and I don't know of a solution yet.

            Having looked at a lot of outcome statements earlier this year, there are very few that I could see being realistically used in an actual rubric. In fact a few teachers have been telling me it doesn't make sense at all, and that they would continue to make and re-use their own rubrics, which surprised me until I took a closer look at what typical secondary/tertiary outcomes look like. On the other hand I've heard there are at least some educational contexts that COULD use it, such as vocational training, so I do believe we should include this in the model, even if most people would be happy with activity/course level outcomes and competencies.

            Elizabeth, I would appreciate some actual detailed complete use cases from you and others to describe what you are hoping for (using ACTUAL outcome statements and ACTUAL rubric + quiz examples).

            Show
            Martin Dougiamas added a comment - There's been a little miscommunication here, I think. I think Andrew was simply focussing on the easier parts first and decided to split the code up to help him do that and to push this along. No-one's saying that we'll not have outcome links at low levels. It's just that those interfaces are not well worked out at all yet, and more work needs to be done there. The current code allows outcomes to be attached to the Assignment activity itself INDEPENDENTLY of the ones attached to rubrics, which is obviously likely to be highly confusing. This is what stopped Outcomes going straight into core initially and I don't know of a solution yet. Having looked at a lot of outcome statements earlier this year, there are very few that I could see being realistically used in an actual rubric. In fact a few teachers have been telling me it doesn't make sense at all, and that they would continue to make and re-use their own rubrics, which surprised me until I took a closer look at what typical secondary/tertiary outcomes look like. On the other hand I've heard there are at least some educational contexts that COULD use it, such as vocational training, so I do believe we should include this in the model, even if most people would be happy with activity/course level outcomes and competencies. Elizabeth, I would appreciate some actual detailed complete use cases from you and others to describe what you are hoping for (using ACTUAL outcome statements and ACTUAL rubric + quiz examples).
            Hide
            Elizabeth Dalton added a comment -

            Martin, thank you for responding. I will provide detailed use cases shortly.

            Show
            Elizabeth Dalton added a comment -