Moodle
  1. Moodle
  2. MDL-37792

Conditional Resource based on Profile Interest with a non-existant string

    Details

    • Testing Instructions:
      Hide

      0. Ensure that conditional availability is enabled in server settings.
      1. Edit any activity.
      2. Find the part of the form about user conditions.
      3. Look at the first dropdown in 'User field'.

      Verify that, although a number of user fields are listed, 'Interests' is not one of them.

      Show
      0. Ensure that conditional availability is enabled in server settings. 1. Edit any activity. 2. Find the part of the form about user conditions. 3. Look at the first dropdown in 'User field'. Verify that, although a number of user fields are listed, 'Interests' is not one of them.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-37792-master

      Description

      Created a conditional activity 'Label' and set it to only be visible to users who had put a particular string into their Interests field in their User Profile.

      There were no users who had that string in the field.

      This created a nasty loop where I could not refresh or move onto another page without the error message popping up.

      Unfortunately I was not smart enough to grab a screen shot before I reverted to a nice fresh install of Moodle.

      I'm guessing for those who have a backup, if this happens a back up to something prior to creating this logic loop would work.

      Better would be to have Moodle not enact the condition until the condition was met.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            Thanks for reporting this.

            Could you please add more information to your report such as replication instructions, error messages and screenshots. If you can determine the cause of the problem in code, or even determine a solution, that will be very valuable.

            Show
            Michael de Raadt added a comment - Thanks for reporting this. Could you please add more information to your report such as replication instructions, error messages and screenshots. If you can determine the cause of the problem in code, or even determine a solution, that will be very valuable.
            Hide
            Michael de Raadt added a comment -

            I've set this as a minor security issue for now. If it is possible to bring down a server using replicatable steps, then it should stay that way.

            Show
            Michael de Raadt added a comment - I've set this as a minor security issue for now. If it is possible to bring down a server using replicatable steps, then it should stay that way.
            Hide
            Susannah Brown added a comment - - edited

            The four basic steps to replicate the bug have been uploaded as screen shots.

            The text of the error is-:
            "Coding error detected, it must be fixed by a programmer: Requested user profile field does not exist"

            Show
            Susannah Brown added a comment - - edited The four basic steps to replicate the bug have been uploaded as screen shots. The text of the error is-: "Coding error detected, it must be fixed by a programmer: Requested user profile field does not exist"
            Hide
            Michael de Raadt added a comment -

            Thanks for adding some more information. I've taken the screenshots out of your zip to make them easier to view.

            Show
            Michael de Raadt added a comment - Thanks for adding some more information. I've taken the screenshots out of your zip to make them easier to view.
            Hide
            Michael de Raadt added a comment -

            I was able to replicate the problem. With caching involved, it's not a simple matter of removing something from the DB to work around the problem.

            Sam: it would be great if you could look at this.

            Show
            Michael de Raadt added a comment - I was able to replicate the problem. With caching involved, it's not a simple matter of removing something from the DB to work around the problem. Sam: it would be great if you could look at this.
            Hide
            Sam Marshall added a comment -

            Hi - sorry I have been very busy for the last few weeks.

            I doubt it brings down the server but it probably kills the course - this probably doesn't count as a security issue since you need editing permissions to that course and there are other ways to kill the course (like deleting it). However, I'll confirm that when I work out the fix.

            Show
            Sam Marshall added a comment - Hi - sorry I have been very busy for the last few weeks. I doubt it brings down the server but it probably kills the course - this probably doesn't count as a security issue since you need editing permissions to that course and there are other ways to kill the course (like deleting it). However, I'll confirm that when I work out the fix.
            Hide
            Sam Marshall added a comment -

            I can confirm:

            1. This does not kill the server, only the course.

            2. The problem is caused because somebody included interests on the list of supported user profile fields, but there is actually no 'interests' field (it's something complicated in the tags table). In other words the user interface that lets users select the feature exist is present, but it was never implemented.

            3. It would be possible to implement this but I don't think there is going to be a frequent need for conditions on the 'interests' non-field. As a result, I propose to fix the problem by simply removing the 'interests' field from the list so that users can't choose it.

            Show
            Sam Marshall added a comment - I can confirm: 1. This does not kill the server, only the course. 2. The problem is caused because somebody included interests on the list of supported user profile fields, but there is actually no 'interests' field (it's something complicated in the tags table). In other words the user interface that lets users select the feature exist is present, but it was never implemented. 3. It would be possible to implement this but I don't think there is going to be a frequent need for conditions on the 'interests' non-field. As a result, I propose to fix the problem by simply removing the 'interests' field from the list so that users can't choose it.
            Hide
            Sam Marshall added a comment -

            Requesting peer review. To recap my comment above, the 'interests' field option appears never to have been implemented, so removing it from the list does not reduce functionality. Just stops this bug happening!

            Show
            Sam Marshall added a comment - Requesting peer review. To recap my comment above, the 'interests' field option appears never to have been implemented, so removing it from the list does not reduce functionality. Just stops this bug happening!
            Hide
            Frédéric Massart added a comment -

            Hi Sam,

            thanks for the patch. While I think you can freely push this for integration, how would you feel about writing a small unit test to assert that:

            • The list of items returned by get_condition_user_fields are actually valid fields, or custom fields?
            • Conditions are respected while using profile fields? (Out of the scope of this issue, but always useful)

            Many thanks!
            Fred

            Show
            Frédéric Massart added a comment - Hi Sam, thanks for the patch. While I think you can freely push this for integration, how would you feel about writing a small unit test to assert that: The list of items returned by get_condition_user_fields are actually valid fields, or custom fields? Conditions are respected while using profile fields? (Out of the scope of this issue, but always useful) Many thanks! Fred
            Hide
            Sam Marshall added a comment -

            Thanks Fred!

            You are right that it would be nice to have a test that detects this issue in case there are any other problem fields (I had already looked at the list and it looked ok but that's not the most reliable way).

            I have added a test which checks (a) that all fields in the list, it can actually check the value of (the test that would detect this problem) and (b) that custom fields are included in the list.

            Now submitting for integration review as the test should more than cover the change.

            I didn't do a check for the comparison operators, which as you say is a separate issue; I decided it was out of scope for this and I hope somebody else adds it! (Other than this one I've just written, there seem to be no unit tests at all for the user-field-condition feature which is rather poor. I can't remember if I reviewed this code or not, but whoever it was should have spotted that...)

            Show
            Sam Marshall added a comment - Thanks Fred! You are right that it would be nice to have a test that detects this issue in case there are any other problem fields (I had already looked at the list and it looked ok but that's not the most reliable way). I have added a test which checks (a) that all fields in the list, it can actually check the value of (the test that would detect this problem) and (b) that custom fields are included in the list. Now submitting for integration review as the test should more than cover the change. I didn't do a check for the comparison operators, which as you say is a separate issue; I decided it was out of scope for this and I hope somebody else adds it! (Other than this one I've just written, there seem to be no unit tests at all for the user-field-condition feature which is rather poor. I can't remember if I reviewed this code or not, but whoever it was should have spotted that...)
            Hide
            Sam Marshall added a comment -

            Removing the 'security issue' marker, this is not a security issue for the reasons I gave some way above (I forgot to do this earlier).

            Show
            Sam Marshall added a comment - Removing the 'security issue' marker, this is not a security issue for the reasons I gave some way above (I forgot to do this earlier).
            Hide
            Damyon Wiese 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.

            Thanks!

            Show
            Damyon Wiese 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. Thanks!
            Hide
            Damyon Wiese added a comment -

            Thanks Sam,

            The patch makes sense and the unit test is good.

            I think it would be worth fixing any existing courses that have been broken by this bug. Can you please add an upgrade step that deletes from section_avail_fields and course_modules_avail_fields any condition using this broken field? (I imagine there would be a lecturer somewhere who would be eternally grateful for this!)

            Thanks, Damyon

            Show
            Damyon Wiese added a comment - Thanks Sam, The patch makes sense and the unit test is good. I think it would be worth fixing any existing courses that have been broken by this bug. Can you please add an upgrade step that deletes from section_avail_fields and course_modules_avail_fields any condition using this broken field? (I imagine there would be a lecturer somewhere who would be eternally grateful for this!) Thanks, Damyon
            Hide
            Sam Marshall added a comment -

            OK - to confirm, I am coding the upgrade step now.

            Show
            Sam Marshall added a comment - OK - to confirm, I am coding the upgrade step now.
            Hide
            Sam Marshall added a comment -

            I have updated both branches with an upgrade step. As this is a change to the main db/upgrade.php and version.php the version numbers might be wrong - please correct these when integrating.

            Show
            Sam Marshall added a comment - I have updated both branches with an upgrade step. As this is a change to the main db/upgrade.php and version.php the version numbers might be wrong - please correct these when integrating.
            Hide
            Damyon Wiese added a comment -

            Thanks - taking another look now.

            Show
            Damyon Wiese added a comment - Thanks - taking another look now.
            Hide
            Damyon Wiese added a comment -

            $DB->delete_records('course_sections_avail_fields', array('userfield' => 'idnumber'));

            should be

            $DB->delete_records('course_sections_avail_fields', array('userfield' => 'interests'));

            Show
            Damyon Wiese added a comment - $DB->delete_records('course_sections_avail_fields', array('userfield' => 'idnumber')); should be $DB->delete_records('course_sections_avail_fields', array('userfield' => 'interests'));
            Hide
            Damyon Wiese added a comment -

            I'll add a patch to fix the upgrade commands - that's the only issue I spotted.

            Show
            Damyon Wiese added a comment - I'll add a patch to fix the upgrade commands - that's the only issue I spotted.
            Hide
            Damyon Wiese added a comment -

            Thanks Sam,

            I have integrated this for 24 and master with an additional patch to fix the upgrade code.

            Thanks for working on this important bug fix!

            Show
            Damyon Wiese added a comment - Thanks Sam, I have integrated this for 24 and master with an additional patch to fix the upgrade code. Thanks for working on this important bug fix!
            Hide
            David Monllaó added a comment -

            Tested in master and 24, it passes

            Show
            David Monllaó added a comment - Tested in master and 24, it passes
            Hide
            Sam Marshall added a comment -

            Can't believe I made that mistake! I tested the upgrade step didn't give an error, but I didn't test it actually worked... Thanks for fixing it.

            Show
            Sam Marshall added a comment - Can't believe I made that mistake! I tested the upgrade step didn't give an error, but I didn't test it actually worked... Thanks for fixing it.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Because

            A
            MARVELOUS
            A       U
            Z  YOU  P
            I  ARE  E
            N  PPL  R
            G       B
              TNKS! 
            

            Closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Because A MARVELOUS A U Z YOU P I ARE E N PPL R G B TNKS! Closing, ciao
            Hide
            Vidya Kumar added a comment -

            Hi,

            I'm using Moodle 2.4.1 (Build: 20130114).

            I created a label that I wanted to be visible only to users who have logged in. So I added a condition that the email ID field should have "@". (This may not have been the best or the smartest approach, but I couldn't think of another way to hide the label for those who haven't logged in.)

            Now the installation seems to have crashed. At least I'm not able to even access my instance. The only screenshot I have is here.

            I'm not sure if this is the best place to log this issue, but any help would be appreciated.

            Thanks...

            Show
            Vidya Kumar added a comment - Hi, I'm using Moodle 2.4.1 (Build: 20130114). I created a label that I wanted to be visible only to users who have logged in. So I added a condition that the email ID field should have "@". (This may not have been the best or the smartest approach, but I couldn't think of another way to hide the label for those who haven't logged in.) Now the installation seems to have crashed. At least I'm not able to even access my instance. The only screenshot I have is here. I'm not sure if this is the best place to log this issue, but any help would be appreciated. Thanks...
            Hide
            Vidya Kumar added a comment -

            Hi,

            I'm using Moodle 2.4.1 (Build: 20130114).

            I created a label that I wanted to be visible only to users who have logged in. So I added a condition that the email ID field should have "@". (This may not have been the best or the smartest approach, but I couldn't think of another way to hide the label for those who haven't logged in.)

            Now the installation seems to have crashed. At least I'm not able to even access my instance. The only screenshot I have is here.

            I'm not sure if this is the best place to log this issue, but any help would be appreciated.

            Thanks...

            Show
            Vidya Kumar added a comment - Hi, I'm using Moodle 2.4.1 (Build: 20130114). I created a label that I wanted to be visible only to users who have logged in. So I added a condition that the email ID field should have "@". (This may not have been the best or the smartest approach, but I couldn't think of another way to hide the label for those who haven't logged in.) Now the installation seems to have crashed. At least I'm not able to even access my instance. The only screenshot I have is here. I'm not sure if this is the best place to log this issue, but any help would be appreciated. Thanks...
            Hide
            Sam Marshall added a comment -

            Vidya: That looks like you are describing this same issue. If you upgrade to the latest Moodle 2.4 stable version (or wait for 2.4.2) then you should find that the problem goes away.

            Show
            Sam Marshall added a comment - Vidya: That looks like you are describing this same issue. If you upgrade to the latest Moodle 2.4 stable version (or wait for 2.4.2) then you should find that the problem goes away.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: