Moodle
  1. Moodle
  2. MDL-34286

Caching issue in user profile field conditions

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4
    • Component/s: Conditional activities
    • Labels:
      None
    • Testing Instructions:
      Hide

      Test one:

      1. Log in as admin/teacher
      2. Create a menu profile field and set options as 1,2,3,4
      3. Edit an activity and set the condition so that it is visible if the menu field contains 3
      4. Login as student and make sure the activity is not available.
      5. Goto your profile and set the menu field to 3
      6. Return to course and activity is still not available.
      7. Logout and log back in, now you should be able to access the activity.

      Test two:

      1. Log in as an admin
      2. Create a new Menu profile field with the following details:
        • Shortname: favouritecolour
        • Name: What is your favourite colour?
        • Required: Yes
        • Display on signup page: Yes
        • Options: Undecided, Red, Green, Blue, Yellow, Orange, Pink, Purple, Magenta, Cyan, Turquoise, Brown
        • Default: Undecided
      3. Browse to a course
      4. Add a forum activity called what ever you want and add the following condition:
        • User field: What is your favourite colour?
        • doesn't contain
        • Undecided
      5. Save the forum and log out.
      6. Log in as a student of the course.
      7. Browse to the course.
      8. Confirm: The activity is shown as available.
      9. Go to the page to edit your profile
      10. Click the save button without making any changes
      11. Browse back to the course.
      12. Confirm: The activity is now shown as greyed out.
      Show
      Test one: Log in as admin/teacher Create a menu profile field and set options as 1,2,3,4 Edit an activity and set the condition so that it is visible if the menu field contains 3 Login as student and make sure the activity is not available. Goto your profile and set the menu field to 3 Return to course and activity is still not available. Logout and log back in, now you should be able to access the activity. Test two: Log in as an admin Create a new Menu profile field with the following details: Shortname: favouritecolour Name: What is your favourite colour? Required: Yes Display on signup page: Yes Options: Undecided, Red, Green, Blue, Yellow, Orange, Pink, Purple, Magenta, Cyan, Turquoise, Brown Default: Undecided Browse to a course Add a forum activity called what ever you want and add the following condition: User field: What is your favourite colour? doesn't contain Undecided Save the forum and log out. Log in as a student of the course. Browse to the course. Confirm: The activity is shown as available. Go to the page to edit your profile Click the save button without making any changes Browse back to the course. Confirm: The activity is now shown as greyed out.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-MDL-34286-m24
    • Rank:
      42651

      Description

      There is a caching issue when using user profile field conditions.

      • Log in as an admin
      • Add a condition to an activity that involves the user setting a checkbox (make sure they havn't set it already).
      • Log in as the student browse to the course and confirm the activity is not available.
      • Edit your profile and check the checkbox.
      • Browse back to the course.
      • Notice the activity is still unavailable... caching issue.

      Should be simple to fix, details on MDL-29538

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Linked parent issue MDL-29538

          Show
          Sam Hemelryk added a comment - Linked parent issue MDL-29538
          Hide
          Sam Hemelryk added a comment -

          Hi,

          This is up for peer-review now.

          I've made a commit that will fix both this issue and MDL-34287.
          It rewrites the way in which user profile data is loaded when processing conditions.
          I found when looking at it that in the case of the current user the data is already loaded 99% of the time.
          As part of the rewrite I made the following notable changes:

          1. If its the current user we try to use $USER if the information has been loaded there.
          2. If its $USER and the information is not there we use the user profile API to load the data which ensure defaults are applied (MDL-34287).
          3. It loads all of the custom profile fields on the first request for one and caches against the object.
          4. I removed the caching of the user data. 99% of the time it is already being cached in the $user object and it would mean having to load everything for the first request which would be unnecessary.
          5. Removed the $grabthelot arg, that is no longer required as we don't cache user profile data ourselves any more.

          Mark I'd appreciate it if you could have a look at this issue as well as the peer-reviewer.
          Certainly you will have a better understanding about how this code works and how your clients are using it.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi, This is up for peer-review now. I've made a commit that will fix both this issue and MDL-34287 . It rewrites the way in which user profile data is loaded when processing conditions. I found when looking at it that in the case of the current user the data is already loaded 99% of the time. As part of the rewrite I made the following notable changes: If its the current user we try to use $USER if the information has been loaded there. If its $USER and the information is not there we use the user profile API to load the data which ensure defaults are applied ( MDL-34287 ). It loads all of the custom profile fields on the first request for one and caches against the object. I removed the caching of the user data. 99% of the time it is already being cached in the $user object and it would mean having to load everything for the first request which would be unnecessary. Removed the $grabthelot arg, that is no longer required as we don't cache user profile data ourselves any more. Mark I'd appreciate it if you could have a look at this issue as well as the peer-reviewer. Certainly you will have a better understanding about how this code works and how your clients are using it. Cheers Sam
          Hide
          Frédéric Massart added a comment -

          Hi Sam, I have been through your code and this looks good to me. I'd just recommend to add some testing instructions. I followed the ones in the description, but could not reproduce it, unless I made sure to set a value on step 2.

          Feel free to push for integration whenever you're ready.

          Show
          Frédéric Massart added a comment - Hi Sam, I have been through your code and this looks good to me. I'd just recommend to add some testing instructions. I followed the ones in the description, but could not reproduce it, unless I made sure to set a value on step 2. Feel free to push for integration whenever you're ready.
          Hide
          Sam Hemelryk added a comment -

          Thanks for looking at this Fred, I'm not too sure why you couldn't replicate it without setting a value.
          I've vamped up the test instructions now and am putting this up for integration.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for looking at this Fred, I'm not too sure why you couldn't replicate it without setting a value. I've vamped up the test instructions now and am putting this up for integration. Cheers Sam
          Hide
          Dan Poltawski added a comment -

          Thanks Sam, i've integrated this now.

          One think I thought when reading the code was that I think it would've been more clear to use some parenthesis or an explicit (bool) to make the following statement more clear:

           $iscurrentuser = $USER->id == $userid;
          
          Show
          Dan Poltawski added a comment - Thanks Sam, i've integrated this now. One think I thought when reading the code was that I think it would've been more clear to use some parenthesis or an explicit (bool) to make the following statement more clear: $iscurrentuser = $USER->id == $userid;
          Hide
          Adrian Greeve added a comment -

          I went through the test instructions and noticed the following things:
          Test one

          • After logging in as a student, the activity wasn't available. I went and changed the field to 3 and went back to the activity and it was now available. There was no need to log out and then back in to get the activity to show up.

          Test two

          • When I logged in as a student the first time the activity was greyed out. I changed the favourite colour field to some colour and the activity became active.

          As far as I'm concerned both are working as expected, I'm pretty sure that I've tested the patch as Sam wanted. I just thought that I'd make a note here just in case there is something that I am missing.

          Test passed.

          Show
          Adrian Greeve added a comment - I went through the test instructions and noticed the following things: Test one After logging in as a student, the activity wasn't available. I went and changed the field to 3 and went back to the activity and it was now available. There was no need to log out and then back in to get the activity to show up. Test two When I logged in as a student the first time the activity was greyed out. I changed the favourite colour field to some colour and the activity became active. As far as I'm concerned both are working as expected, I'm pretty sure that I've tested the patch as Sam wanted. I just thought that I'd make a note here just in case there is something that I am missing. Test passed.
          Hide
          Aparup Banerjee added a comment -

          yay, it works!

          This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week.

          Thank you all for taking the time to get us here.

          cheers!

          Show
          Aparup Banerjee added a comment - yay, it works! This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week. Thank you all for taking the time to get us here. cheers!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: