Moodle
  1. Moodle
  2. MDL-26516

Teacher problems setting user overrides in quiz (calendar error)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.1
    • Fix Version/s: None
    • Component/s: Calendar, Quiz
    • Labels:
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE
    • Rank:
      16250

      Description

      Detected @ MDLQA-697, after some debugging I was able to get simple steps to reproduce it:

      • As teacher, create quiz, set start/end dates for it.
      • Navigation -> user overrides and create one.
      • Error happens:
      Sorry, but you do not currently have permissions to update calendar event
      
      More information about this error
      
      Stack trace:
      line 421 of /lib/setuplib.php: moodle_exception thrown
      line 1969 of /calendar/lib.php: call to print_error()
      line 2379 of /calendar/lib.php: call to calendar_event->update()
      line 1290 of /mod/quiz/lib.php: call to calendar_event::create()
      line 172 of /mod/quiz/overrideedit.php: call to quiz_update_events()
      

      It seems that the quiz is trying to create user events, (with eventyped = 'close' ???) and calendar_add_event_allowed() is returning one big false, hence the error above. This is the structure of the event:

      stdClass Object
      (
          [description] => xxxxxxxxxxxx
          [courseid] => 0
          [groupid] => 0
          [userid] => 5
          [modulename] => quiz
          [instance] => 1
          [timestart] => 1445001300
          [timeduration] => 0
          [visible] => 1
          [eventtype] => close
          [name] => One Quiz - Override (Quiz closes)
      )
      

      Really I'm not sure if the mistake is in the quiz, and how the event is created or in calendar and how event-creation perms are checked. It seems that there are some strange course-user dark logic (implications) there.

      Ciao

        Issue Links

          Activity

          Hide
          Martin Dougiamas added a comment -

          Yeah I can't see why events need to be updated at all for this page.

          Show
          Martin Dougiamas added a comment - Yeah I can't see why events need to be updated at all for this page.
          Hide
          Rossiani Wijaya added a comment -

          Adding Sam and Tim as watcher and to review the patch.

          The above error occurs because adding calendar event required moodle/calendar:manageentries capability, which student doesn't has.

          I created a patch to allow teacher to create a user event for student.

          Patch is available at: https://github.com/rwijaya/moodle/compare/master...MDL-26516_m20.

          Thanks
          Rosie

          Show
          Rossiani Wijaya added a comment - Adding Sam and Tim as watcher and to review the patch. The above error occurs because adding calendar event required moodle/calendar:manageentries capability, which student doesn't has. I created a patch to allow teacher to create a user event for student. Patch is available at: https://github.com/rwijaya/moodle/compare/master...MDL-26516_m20 . Thanks Rosie
          Hide
          Tim Hunt added a comment -

          This patch is definitely wrong. You have added a check that the user is allowed to manage their own entries.

          The question is, is the teacher allowed to managed entries for this student.

          Martin, to explain:

          1. Quiz open and close dates are automatically added to the calendar as events.
          2. In Moodle 2.0 we have a new feature (much voted for) where teachers can set different open and close dates for different groups or individual students (quiz settings overrides).
          3. Therefore, the quiz creates group and user calendar events for groups and individuals who have been given an override.

          It seems that the permissions checks in the calendar code cannot cope with this

          Show
          Tim Hunt added a comment - This patch is definitely wrong. You have added a check that the user is allowed to manage their own entries. The question is, is the teacher allowed to managed entries for this student. Martin, to explain: Quiz open and close dates are automatically added to the calendar as events. In Moodle 2.0 we have a new feature (much voted for) where teachers can set different open and close dates for different groups or individual students (quiz settings overrides). Therefore, the quiz creates group and user calendar events for groups and individuals who have been given an override. It seems that the permissions checks in the calendar code cannot cope with this
          Hide
          Charles Fulton added a comment -

          I don't think it's a permissions issue: note the event id. I've verified that it's set to zero as well in my dev instance, and I think it's because of line 1226 in mod/quiz/lib.php ($event->courseid = ($userid) ? 0 : $quiz->course. That courseid is used for the capability checks. Maybe the solution is to pass courseid as a separate, optional variable to calendar_add_event_allowed() and use it for capability checks?

          Show
          Charles Fulton added a comment - I don't think it's a permissions issue: note the event id. I've verified that it's set to zero as well in my dev instance, and I think it's because of line 1226 in mod/quiz/lib.php ($event->courseid = ($userid) ? 0 : $quiz->course . That courseid is used for the capability checks. Maybe the solution is to pass courseid as a separate, optional variable to calendar_add_event_allowed() and use it for capability checks?
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Well, there are 2 points here:

          1) The calendar/lib is really cheat[ed/able] with tricks like, "I set the course (or the user) to 0 so, then, this (dark magic) things happen" and similar. That requires rework of the API for sure (more if also we need to add proper support for repeated events and TZs). But this is out of the scope of this bug IMO.

          2) The calendar/lib checks permissions at really different contexts (system, course) based on the crazy dark magic decided in 1), and this is our real problem. The calendar_edit_event_allowed() function expects real course and events are being created without them. So... there are 3 alternatives:

          a) We enforce courseid to be also passed. But this is 1) and sure it has other implications.
          b) We calculate courseid (from modulename/instance) and let calendar_edit_event_allowed() to know about it (by new param or by $event->courseid itself).
          c) We modify a bit calendar/lib, so $event->context is introduced / calculated and apply for it in calendar_edit_event_allowed()

          My vote goes, surely, to c) Add basic ->context support and use it in calendar_edit_event_allowed()

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Well, there are 2 points here: 1) The calendar/lib is really cheat [ed/able] with tricks like, "I set the course (or the user) to 0 so, then, this (dark magic) things happen" and similar. That requires rework of the API for sure (more if also we need to add proper support for repeated events and TZs). But this is out of the scope of this bug IMO. 2) The calendar/lib checks permissions at really different contexts (system, course) based on the crazy dark magic decided in 1), and this is our real problem. The calendar_edit_event_allowed() function expects real course and events are being created without them. So... there are 3 alternatives: a) We enforce courseid to be also passed. But this is 1) and sure it has other implications. b) We calculate courseid (from modulename/instance) and let calendar_edit_event_allowed() to know about it (by new param or by $event->courseid itself). c) We modify a bit calendar/lib, so $event->context is introduced / calculated and apply for it in calendar_edit_event_allowed() My vote goes, surely, to c) Add basic ->context support and use it in calendar_edit_event_allowed() Ciao
          Hide
          Rossiani Wijaya added a comment -

          Eloy's recommendation (through jabber):

          (18:46:01) stronk7@contiento.com: I think we need to be a bit more sure about all the combinations:

          UI->user event
          UI->activity event
          UI->group event
          UI->course event
          UI->site event
          module->custom event without user/group info (like chat, assignment)
          module->custom event with group info (quiz group overrides)
          module->custom event with user info (quiz user overrides)
          Any other event type not in the list above (check if there are)

          And, before putting any code....

          1) have properly defined which context we are going to associate to each one
          2) have properly defined in which context we are going to check each one
          3) see if 1) and 2) 100x100 match

          Then code.
          (18:46:48) stronk7@contiento.com: just to be 100x100 sure we don't leave any combination out from the equation.
          (18:47:54) stronk7@contiento.com: Surely your code is 99% ok. But it would be great to have all the cases above explicitly defined and then coded (easier for later review / change if necessary).
          ....
          (18:53:09) stronk7@contiento.com: wow, feedback module, also using custom events... put translated lang strings into DB! crazy!
          (18:54:32) stronk7@contiento.com: hah, and quiz too, lol.
          (18:54:51) stronk7@contiento.com: yeah, i think if we complete the list above we'll be safer

          Show
          Rossiani Wijaya added a comment - Eloy's recommendation (through jabber): (18:46:01) stronk7@contiento.com: I think we need to be a bit more sure about all the combinations: UI->user event UI->activity event UI->group event UI->course event UI->site event module->custom event without user/group info (like chat, assignment) module->custom event with group info (quiz group overrides) module->custom event with user info (quiz user overrides) Any other event type not in the list above (check if there are) And, before putting any code.... 1) have properly defined which context we are going to associate to each one 2) have properly defined in which context we are going to check each one 3) see if 1) and 2) 100x100 match Then code. (18:46:48) stronk7@contiento.com: just to be 100x100 sure we don't leave any combination out from the equation. (18:47:54) stronk7@contiento.com: Surely your code is 99% ok. But it would be great to have all the cases above explicitly defined and then coded (easier for later review / change if necessary). .... (18:53:09) stronk7@contiento.com: wow, feedback module, also using custom events... put translated lang strings into DB! crazy! (18:54:32) stronk7@contiento.com: hah, and quiz too, lol. (18:54:51) stronk7@contiento.com: yeah, i think if we complete the list above we'll be safer
          Show
          Rossiani Wijaya added a comment - work-in-progress patch: https://github.com/rwijaya/moodle/compare/master...MDL-26516_m20_calevent
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Rosie,

          some comments about your patch above, numbered for reference:

          1) What are all those $data->context->type = sys | ins | course... ? Do we need them for anything?

          2) Your patch modifies the original "open", "close"... custom events to something completely different - the results of get_eventtype() - how will that affect the activities?

          3) Why do we need to perform those clone() + new calendar_event() in different places? Cannot we, simply calculate the context in constructor or so (where the rest of event attributes are calculated?).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Rosie, some comments about your patch above, numbered for reference: 1) What are all those $data->context->type = sys | ins | course... ? Do we need them for anything? 2) Your patch modifies the original "open", "close"... custom events to something completely different - the results of get_eventtype() - how will that affect the activities? 3) Why do we need to perform those clone() + new calendar_event() in different places? Cannot we, simply calculate the context in constructor or so (where the rest of event attributes are calculated?). Ciao
          Hide
          Rossiani Wijaya added a comment -

          Hi Eloy, thank you for reviewing the patch.

          1) I'm using $data->context-type to test the result so i know that it retrieves the correct info. I will remove those lines for final patch.

          2) According to the docs, there are 4 event types for calendar events: course, group, user and site. get_eventype() is used to define which event type is use for activity. Therefore, each calendar event can be easily identify.

          3) Some of event attributes are set as standard object and I was planning to calculate the context value just before it is being used.
          The clone and new caledar_event are used to sure that $event is an object of calendar_event. However, I realized I could initialize it in calendar_event->create.

          Creating new patch to address the above comments:
          1. revert back eventtype value to 'open', 'close', etc
          2. create a protected calculate_context(). This function is being used to set context properties from constructor.
          note: please ignore $data->context->type. i'm just using it as a test.

          New patch is available at: https://github.com/rwijaya/moodle/compare/master...MDL-26516_m20_calevent_wip3

          Thanks
          Rosie

          Show
          Rossiani Wijaya added a comment - Hi Eloy, thank you for reviewing the patch. 1) I'm using $data->context-type to test the result so i know that it retrieves the correct info. I will remove those lines for final patch. 2) According to the docs, there are 4 event types for calendar events: course, group, user and site. get_eventype() is used to define which event type is use for activity. Therefore, each calendar event can be easily identify. 3) Some of event attributes are set as standard object and I was planning to calculate the context value just before it is being used. The clone and new caledar_event are used to sure that $event is an object of calendar_event. However, I realized I could initialize it in calendar_event->create. Creating new patch to address the above comments: 1. revert back eventtype value to 'open', 'close', etc 2. create a protected calculate_context(). This function is being used to set context properties from constructor. note: please ignore $data->context->type. i'm just using it as a test. New patch is available at: https://github.com/rwijaya/moodle/compare/master...MDL-26516_m20_calevent_wip3 Thanks Rosie
          Show
          Rossiani Wijaya added a comment - new diff url: https://github.com/rwijaya/moodle/compare/MOODLE_20_STABLE...MDL-26516_m20_calevent_wip3
          Hide
          Sam Hemelryk added a comment -

          Hi Rosie,

          I've had a look over the patch now and noted the following things:

          calendar/lib.php:1412
          calendar_edit_event_allowed now uses $event->context, however this function is called within calendar_get_upcoming and that function doesn't set up $event->context, or translate the event into a calendar_event instance.
          Easiest way to test this is to create an assignment within a course then log in as someone who doesn't have the manageentries cap in the system context, you will get a pile of notices.

          calendar/lib.php:1768
          @var object is incorrect now, the object class has been deprecated and stdClass is now used.

          calendar/lib.php:1879
          The calculate_context needs a little more work, it is used once and is meant to return a context for $data->context however the internals of the function set $data->context then return that.
          Plus the check to make sure that $data is wrong as you proceed to treat $data as an object for the return even if it is not an object.
          Because the function is new an only used in one place I think it would be sensible to type hint it as a stdClass and not allow the null e.g. protected function calculate_context(stdClass $data)
          The phpdocs for that function need to be tidied up + a bit of commenting within the if..else tree would be nice.

          You could also use $this->properties->context to replace some of the get_context_calls that are still in the calendar_event class. At a glance the only exception is when it is fetching the users context for users events (we have system context there).

          p.s. don't forget to remove those context->type calls Eloy mentioned

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Rosie, I've had a look over the patch now and noted the following things: calendar/lib.php:1412 calendar_edit_event_allowed now uses $event->context, however this function is called within calendar_get_upcoming and that function doesn't set up $event->context, or translate the event into a calendar_event instance. Easiest way to test this is to create an assignment within a course then log in as someone who doesn't have the manageentries cap in the system context, you will get a pile of notices. calendar/lib.php:1768 @var object is incorrect now, the object class has been deprecated and stdClass is now used. calendar/lib.php:1879 The calculate_context needs a little more work, it is used once and is meant to return a context for $data->context however the internals of the function set $data->context then return that. Plus the check to make sure that $data is wrong as you proceed to treat $data as an object for the return even if it is not an object. Because the function is new an only used in one place I think it would be sensible to type hint it as a stdClass and not allow the null e.g. protected function calculate_context(stdClass $data) The phpdocs for that function need to be tidied up + a bit of commenting within the if..else tree would be nice. You could also use $this->properties->context to replace some of the get_context_calls that are still in the calendar_event class. At a glance the only exception is when it is fetching the users context for users events (we have system context there). p.s. don't forget to remove those context->type calls Eloy mentioned Cheers Sam
          Hide
          Rossiani Wijaya added a comment -

          Thanks Sam for reviewing.

          create new patch to fix the above comments:
          diff url: https://github.com/rwijaya/moodle/compare/MOODLE_20_STABLE...MDL-26516_m20_calevent_wip4

          Note: more testing is needed for this issue.

          Show
          Rossiani Wijaya added a comment - Thanks Sam for reviewing. create new patch to fix the above comments: diff url: https://github.com/rwijaya/moodle/compare/MOODLE_20_STABLE...MDL-26516_m20_calevent_wip4 Note: more testing is needed for this issue.
          Hide
          Rossiani Wijaya added a comment -

          Fixed and create Pull request for this issue. PULL-694 & PULL-695

          Resolve

          note:
          I notice an issue in group override. When deleting a group override, it removes from quiz but event is still available in calendar. Creating a new issue to fix group override: MDL-27230.

          Show
          Rossiani Wijaya added a comment - Fixed and create Pull request for this issue. PULL-694 & PULL-695 Resolve note: I notice an issue in group override. When deleting a group override, it removes from quiz but event is still available in calendar. Creating a new issue to fix group override: MDL-27230 .

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: