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

Wrong validation in upload course

XMLWordPrintable

    • 3
    • HQ 2024 Sprint I1.3 Moppies

      1. Try to upload a course with this csv:

        shortname,fullname,category_idnumber,enrolment_1,enrolment_1_role
        C1,Course 1,CAT1,self,student

      1. Go to created course and delete self enrolment method
      2. Try to upload same csv again (set to update existing course, use csv data only)
      3. Observe the exception 

        Undefined array key "expirynotify" 

       

       

      This function looks wrong:

      public function validate_plugin_data_context(array $enrolmentdata, ?int $courseid = null) : ?lang_string {
          if ($courseid) {
              $enrolmentdata += ['courseid' => $courseid, 'id' => 0, 'status' => ENROL_INSTANCE_ENABLED];
              $instance = (object)[
                  'id' => null,
                  'courseid' => $courseid,
                  'status' => $enrolmentdata['status'],
                  'type' => $this->get_name(),
              ];
              $formerrors = $this->edit_instance_validation($enrolmentdata, [], $instance, context_course::instance($courseid));
              if (!empty($formerrors)) {
                  $errors = array_map(fn($key) => "{$key}: {$formerrors[$key]}", array_keys($formerrors));
                  return new lang_string('errorcannotcreateorupdateenrolment', 'tool_uploadcourse', $errors);
              }
          }
          return null;
      } 

      It calls edit_instance_validation inside, but those validation will be too aggressive. Its fine to omit some data in csv when uploading. For example for self enrolment you can skip setting expirynotify (some default will be used), but then inside edit_instance_validation it will fail since it will try since expirynotify is not set:

      if ($data['expirynotify'] > 0 and $data['expirythreshold'] < 86400) {
          $errors['expirythreshold'] = get_string('errorthresholdlow', 'core_enrol');
      }
       

      similarly to other plugins

      Also function name suggest that we are checking smth in some context. But default case doesnt' deal with context check at all. So using generic checks here feels out of place

            sarjona Sara Arjona (@sarjona)
            ilyatregubov Ilya Tregubov
            Carlos Escobedo Carlos Escobedo
            Jun Pataleta Jun Pataleta
            CiBoT CiBoT
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

              Created:
              Updated:
              Resolved:

                Estimated:
                Original Estimate - Not Specified
                Not Specified
                Remaining:
                Remaining Estimate - 0 minutes
                0m
                Logged:
                Time Spent - 5 hours, 6 minutes
                5h 6m

                  Error rendering 'clockify-timesheets-time-tracking-reports:timer-sidebar'. Please contact your Jira administrators.