Merge branch 'MDL-46946-master-reqcusfield' of git://github.com/mudrd…

…8mz/moodle

admin/tool/monitor/classes/task/check_subscriptions.php

@@ -199,7 +199,7 @@ public function execute() {
      */
     protected function is_user_setup($user) {
         if (!isset($this->userssetupcache[$user->id])) {
-            $this->userssetupcache[$user->id] = !user_not_fully_set_up($user);
+            $this->userssetupcache[$user->id] = !user_not_fully_set_up($user, true);
         }
         return $this->userssetupcache[$user->id];
     }

auth/ldap/auth.php

@@ -1771,7 +1771,7 @@ function ntlmsso_finish() {
             unset_cache_flag($this->pluginconfig.'/ntlmsess', $key);
 
             // Redirection
-            if (user_not_fully_set_up($USER)) {
+            if (user_not_fully_set_up($USER, true)) {
                 $urltogo = $CFG->wwwroot.'/user/edit.php';
                 // We don't delete $SESSION->wantsurl yet, so we get there later
             } else if (isset($SESSION->wantsurl) and (strpos($SESSION->wantsurl, $CFG->wwwroot) === 0)) {

auth/mnet/auth.php

@@ -263,7 +263,7 @@ function confirm_mnet_session($token, $remotepeer) {
             exit;
         }
 
-        if (user_not_fully_set_up($remoteuser)) {
+        if (user_not_fully_set_up($remoteuser, false)) {
             print_error('notenoughidpinfo', 'mnet');
             exit;
         }

auth/shibboleth/index.php

@@ -54,7 +54,7 @@
                 && $user = authenticate_user_login($frm->username, $frm->password)) {
             complete_user_login($user);
 
-            if (user_not_fully_set_up($USER)) {
+            if (user_not_fully_set_up($USER, true)) {
                 $urltogo = $CFG->wwwroot.'/user/edit.php?id='.$USER->id.'&course='.SITEID;
                 // We don't delete $SESSION->wantsurl yet, so we get there later
 

lib/moodlelib.php

@@ -2644,8 +2644,17 @@ function require_login($courseorid = null, $autologinguest = true, $cm = null, $
         }
     }
 
-    // Check that the user account is properly set up.
-    if (user_not_fully_set_up($USER)) {
+    // Check that the user account is properly set up. If we can't redirect to
+    // edit their profile, perform just the lax check. It will allow them to
+    // use filepicker on the profile edit page.
+
+    if ($preventredirect) {
+        $usernotfullysetup = user_not_fully_set_up($USER, false);
+    } else {
+        $usernotfullysetup = user_not_fully_set_up($USER, true);
+    }
+
+    if ($usernotfullysetup) {
         if ($preventredirect) {
             throw new require_login_exception('User not fully set-up');
         }
@@ -3160,14 +3169,39 @@ function update_user_login_times() {
 /**
  * Determines if a user has completed setting up their account.
  *
+ * The lax mode (with $strict = false) has been introduced for special cases
+ * only where we want to skip certain checks intentionally. This is valid in
+ * certain mnet or ajax scenarios when the user cannot / should not be
+ * redirected to edit their profile. In most cases, you should perform the
+ * strict check.
+ *
  * @param stdClass $user A {@link $USER} object to test for the existence of a valid name and email
+ * @param bool $strict Be more strict and assert id and custom profile fields set, too
  * @return bool
  */
-function user_not_fully_set_up($user) {
+function user_not_fully_set_up($user, $strict = true) {
+    global $CFG;
+    require_once($CFG->dirroot.'/user/profile/lib.php');
+
     if (isguestuser($user)) {
         return false;
     }
-    return (empty($user->firstname) or empty($user->lastname) or empty($user->email) or over_bounce_threshold($user));
+
+    if (empty($user->firstname) or empty($user->lastname) or empty($user->email) or over_bounce_threshold($user)) {
+        return true;
+    }
+
+    if ($strict) {
+        if (empty($user->id)) {
+            // Strict mode can be used with existing accounts only.
+            return true;
+        }
+        if (!profile_has_required_custom_fields_set($user->id)) {
+            return true;
+        }
+    }
+
+    return false;
 }
 
 /**

login/lib.php

@@ -285,7 +285,7 @@ function core_login_generate_password_reset ($user) {
 function core_login_get_return_url() {
     global $CFG, $SESSION, $USER;
     // Prepare redirection.
-    if (user_not_fully_set_up($USER)) {
+    if (user_not_fully_set_up($USER, true)) {
         $urltogo = $CFG->wwwroot.'/user/edit.php';
         // We don't delete $SESSION->wantsurl yet, so we get there later.
 

user/edit.php

@@ -72,9 +72,11 @@
 // User interests separated by commas.
 $user->interests = core_tag_tag::get_item_tags_array('core', 'user', $user->id);
 
-// Remote users cannot be edited.
+// Remote users cannot be edited. Note we have to perform the strict user_not_fully_set_up() check.
+// Otherwise the remote user could end up in endless loop between user/view.php and here.
+// Required custom fields are not supported in MNet environment anyway.
 if (is_mnet_remote_user($user)) {
-    if (user_not_fully_set_up($user)) {
+    if (user_not_fully_set_up($user, true)) {
         $hostwwwroot = $DB->get_field('mnet_host', 'wwwroot', array('id' => $user->mnethostid));
         print_error('usernotfullysetup', 'mnet', '', $hostwwwroot);
     }

user/editlib.php

@@ -76,7 +76,7 @@ function useredit_setup_preference_page($userid, $courseid) {
 
     // Remote users cannot be edited.
     if (is_mnet_remote_user($user)) {
-        if (user_not_fully_set_up($user)) {
+        if (user_not_fully_set_up($user, false)) {
             $hostwwwroot = $DB->get_field('mnet_host', 'wwwroot', array('id' => $user->mnethostid));
             print_error('usernotfullysetup', 'mnet', '', $hostwwwroot);
         }

user/profile/lib.php

@@ -662,3 +662,30 @@ function profile_view($user, $context, $course = null) {
     $event->trigger();
 }
 
+/**
+ * Does the user have all required custom fields set?
+ *
+ * Internal, to be exclusively used by {@link user_not_fully_set_up()} only.
+ *
+ * Note that if users have no way to fill a required field via editing their
+ * profiles (e.g. the field is not visible or it is locked), we still return true.
+ * So this is actually checking if we should redirect the user to edit their
+ * profile, rather than whether there is a value in the database.
+ *
+ * @param int $userid
+ * @return bool
+ */
+function profile_has_required_custom_fields_set($userid) {
+    global $DB;
+
+    $sql = "SELECT f.id
+              FROM {user_info_field} f
+         LEFT JOIN {user_info_data} d ON (d.fieldid = f.id AND d.userid = ?)
+             WHERE f.required = 1 AND f.visible > 0 AND f.locked = 0 AND d.id IS NULL";
+
+    if ($DB->record_exists_sql($sql, [$userid])) {
+        return false;
+    }
+
+    return true;
+}

user/tests/profilelib_test.php

@@ -143,4 +143,71 @@ public function test_profile_view() {
 
     }
 
+    /**
+     * Test that {@link user_not_fully_set_up()} takes required custom fields into account.
+     */
+    public function test_profile_has_required_custom_fields_set() {
+        global $CFG, $DB;
+        require_once($CFG->dirroot.'/mnet/lib.php');
+
+        $this->resetAfterTest();
+
+        // Add a required, visible, unlocked custom field.
+        $DB->insert_record('user_info_field', ['shortname' => 'house', 'name' => 'House', 'required' => 1,
+            'visible' => 1, 'locked' => 0, 'categoryid' => 1, 'datatype' => 'text']);
+
+        // Add an optional, visible, unlocked custom field.
+        $DB->insert_record('user_info_field', ['shortname' => 'pet', 'name' => 'Pet', 'required' => 0,
+            'visible' => 1, 'locked' => 0, 'categoryid' => 1, 'datatype' => 'text']);
+
+        // Add required but invisible custom field.
+        $DB->insert_record('user_info_field', ['shortname' => 'secretid', 'name' => 'Secret ID', 'required' => 1,
+            'visible' => 0, 'locked' => 0, 'categoryid' => 1, 'datatype' => 'text']);
+
+        // Add required but locked custom field.
+        $DB->insert_record('user_info_field', ['shortname' => 'muggleborn', 'name' => 'Muggle-born', 'required' => 1,
+            'visible' => 1, 'locked' => 1, 'categoryid' => 1, 'datatype' => 'checkbox']);
+
+        // Create some student accounts.
+        $hermione = $this->getDataGenerator()->create_user();
+        $harry = $this->getDataGenerator()->create_user();
+        $ron = $this->getDataGenerator()->create_user();
+        $draco = $this->getDataGenerator()->create_user();
+
+        // Hermione has all available custom fields filled (of course she has).
+        profile_save_data((object)['id' => $hermione->id, 'profile_field_house' => 'Gryffindor']);
+        profile_save_data((object)['id' => $hermione->id, 'profile_field_pet' => 'Crookshanks']);
+
+        // Harry has only the optional field filled.
+        profile_save_data((object)['id' => $harry->id, 'profile_field_pet' => 'Hedwig']);
+
+        // Draco has only the required field filled.
+        profile_save_data((object)['id' => $draco->id, 'profile_field_house' => 'Slytherin']);
+
+        // Only students with required fields filled should be considered as fully set up in the default (strict) mode.
+        $this->assertFalse(user_not_fully_set_up($hermione));
+        $this->assertFalse(user_not_fully_set_up($draco));
+        $this->assertTrue(user_not_fully_set_up($harry));
+        $this->assertTrue(user_not_fully_set_up($ron));
+
+        // In the lax mode, students do not need to have required fields filled.
+        $this->assertFalse(user_not_fully_set_up($hermione, false));
+        $this->assertFalse(user_not_fully_set_up($draco, false));
+        $this->assertFalse(user_not_fully_set_up($harry, false));
+        $this->assertFalse(user_not_fully_set_up($ron, false));
+
+        // Lack of required core field is seen as a problem in either mode.
+        unset($hermione->email);
+        $this->assertTrue(user_not_fully_set_up($hermione, true));
+        $this->assertTrue(user_not_fully_set_up($hermione, false));
+
+        // When confirming remote MNet users, we do not have custom fields available.
+        $roamingharry = mnet_strip_user($harry, ['firstname', 'lastname', 'email']);
+        $roaminghermione = mnet_strip_user($hermione, ['firstname', 'lastname', 'email']);
+
+        $this->assertTrue(user_not_fully_set_up($roamingharry, true));
+        $this->assertFalse(user_not_fully_set_up($roamingharry, false));
+        $this->assertTrue(user_not_fully_set_up($roaminghermione, true));
+        $this->assertTrue(user_not_fully_set_up($roaminghermione, false));
+    }
 }