Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Files API, Themes
    • Labels:
      None
    • Testing Instructions:
      Hide

      There should be no visible changes with this patch (better performance and more accuracy)

      1. Browse around the site and an admin and as a normal user and just check you can still see user profile pics as you did before.
      Show
      There should be no visible changes with this patch (better performance and more accuracy) Browse around the site and an admin and as a normal user and just check you can still see user profile pics as you did before.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      wip-MDL-28043-master

      Description

      Introduced by MDL-27866.
      From the upper level, moodle should check if user profile image exists. and pix_url should trigger page initialization.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Sam Hemelryk added a comment -

            This needs to be looked at immediately after the release of 2.1 - DO NOT LOWER ITS PRIORITY it is the tip of the ice berg to a bigger problem (the initialisation of page, theme, and output).
            It also needs to be fixed in Moodle 2.0+ and 2.1+.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - This needs to be looked at immediately after the release of 2.1 - DO NOT LOWER ITS PRIORITY it is the tip of the ice berg to a bigger problem (the initialisation of page, theme, and output). It also needs to be fixed in Moodle 2.0+ and 2.1+. Cheers Sam
            Hide
            Sam Hemelryk added a comment -

            Ok - I've got a patch up for this.

            The changes remove the need to initialise the page, theme and output when serving a user profile pic if a users profile picture is not available at the time of the request.
            It also cleans up the context hack at the same time, and shifts the calculation of the URL for a profile pic from the output renderer and pluginfile to the user_picture component.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Ok - I've got a patch up for this. The changes remove the need to initialise the page, theme and output when serving a user profile pic if a users profile picture is not available at the time of the request. It also cleans up the context hack at the same time, and shifts the calculation of the URL for a profile pic from the output renderer and pluginfile to the user_picture component. Cheers Sam
            Hide
            Rajesh Taneja added a comment -

            Looks Great to me Sam...
            Thanks for improving Moodle

            Show
            Rajesh Taneja added a comment - Looks Great to me Sam... Thanks for improving Moodle
            Hide
            Sam Hemelryk added a comment -

            Rebased now

            Show
            Sam Hemelryk added a comment - Rebased now
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Hi, 3 questions before integrating this:

            • This line seems copy/paste unused one:

              $usercontext = get_context_instance(CONTEXT_USER, $this->user->id);
              

            • Minor: Not important at all but wouldn't be better to make the url ..../theme/fX instead of ..../fX/theme ?
            • This must be backported to 20_STABLE and 21_STABLE, correct? If so, please state it in the testing instructions and comment about it.

            I'll keep this open...ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Hi, 3 questions before integrating this: This line seems copy/paste unused one: $usercontext = get_context_instance(CONTEXT_USER, $this->user->id); Minor: Not important at all but wouldn't be better to make the url ..../theme/fX instead of ..../fX/theme ? This must be backported to 20_STABLE and 21_STABLE, correct? If so, please state it in the testing instructions and comment about it. I'll keep this open...ciao
            Hide
            Jonathan Harker added a comment -

            I've rebased the Gravatar stuff on Sam's wip-MDL-28043-master branch - see MDL-21676

            Show
            Jonathan Harker added a comment - I've rebased the Gravatar stuff on Sam's wip- MDL-28043 -master branch - see MDL-21676
            Hide
            Eloy Lafuente (stronk7) 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.

            TIA and ciao

            Show
            Eloy Lafuente (stronk7) 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. TIA and ciao
            Hide
            Sam Hemelryk added a comment -

            Hi Eloy,

            Thanks for looking at it, sorry I didn't get a chance to review your comments while I was away.
            In regards to the three points you have raised:

            1: Definitely an unused typo carried across while I was rearranging that method.

            2: I'm open on discussion to this, the theme was added to the end to make it backwards compatible (albeit I'm sure it could be rearranged and still made bc).
            Perhaps the challenge will be coming up with a system that allows for the optional theme in the URL while still supporting all forms of the user pic, both default and what ever they have chosen. This is really only a problem if there is a path component to the URL, unfortunately I havn't had a look into, or think about this in all circumstances yet.
            If this is going to be backported to 20 and 21 then we had better decide now, if its not going to be backported or there is urgency to get this in (Id don't think there is is there?) then lets create a new issue and properly discuss and look at it. For the time being my +1 to delay this and look at it properly.

            3: I think this is suitable for backport, it should still be backwards compatible without any problems so my +1 to backport.

            Let me know what you think.

            Cheers
            Sam

            p.s. I am the dev so probably shouldn't integrate this, it could either wait till you're back online Eloy or I can assist Apu in reviewing it

            Show
            Sam Hemelryk added a comment - Hi Eloy, Thanks for looking at it, sorry I didn't get a chance to review your comments while I was away. In regards to the three points you have raised: 1: Definitely an unused typo carried across while I was rearranging that method. 2: I'm open on discussion to this, the theme was added to the end to make it backwards compatible (albeit I'm sure it could be rearranged and still made bc). Perhaps the challenge will be coming up with a system that allows for the optional theme in the URL while still supporting all forms of the user pic, both default and what ever they have chosen. This is really only a problem if there is a path component to the URL, unfortunately I havn't had a look into, or think about this in all circumstances yet. If this is going to be backported to 20 and 21 then we had better decide now, if its not going to be backported or there is urgency to get this in (Id don't think there is is there?) then lets create a new issue and properly discuss and look at it. For the time being my +1 to delay this and look at it properly. 3: I think this is suitable for backport, it should still be backwards compatible without any problems so my +1 to backport. Let me know what you think. Cheers Sam p.s. I am the dev so probably shouldn't integrate this, it could either wait till you're back online Eloy or I can assist Apu in reviewing it
            Hide
            Sam Hemelryk added a comment -

            Forgot to mention I am still setting things up - I will make the coding changes once everything is up and running at my end (or if you'd like to integrate feel free to remove the context during integration).

            Show
            Sam Hemelryk added a comment - Forgot to mention I am still setting things up - I will make the coding changes once everything is up and running at my end (or if you'd like to integrate feel free to remove the context during integration).
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Reopening heading to final solution this week. Apologizes for all these iterations before landing, but better once and correct.

            Show
            Eloy Lafuente (stronk7) added a comment - Reopening heading to final solution this week. Apologizes for all these iterations before landing, but better once and correct.
            Hide
            Michael de Raadt added a comment -

            Shifting this reopened issue to the current sprint.

            Show
            Michael de Raadt added a comment - Shifting this reopened issue to the current sprint.
            Hide
            Sam Hemelryk added a comment -

            Hi guys

            Putting this up for peer-review.
            I'm removed the unused context variable, switched the order of the themename in URL, and backported this to 20 and 21.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi guys Putting this up for peer-review. I'm removed the unused context variable, switched the order of the themename in URL, and backported this to 20 and 21. Cheers Sam
            Hide
            Aparup Banerjee added a comment -

            Hi Sam,
            Thanks for fixing up the @return so quick!

            • minor: extra semicolon on pluginfile.php:284
            • swapping between '&&' and 'and' in pluginfile.php:295 has the potential to be read/rewritten incorrectly. maybe a note to remind of the precedence needed there (if there is any).
              or perhaps multiple 'if'? note sure on the guidelines here.

            Otherwise the code looks alright to me , cheers!

            Show
            Aparup Banerjee added a comment - Hi Sam, Thanks for fixing up the @return so quick! minor: extra semicolon on pluginfile.php:284 swapping between '&&' and 'and' in pluginfile.php:295 has the potential to be read/rewritten incorrectly. maybe a note to remind of the precedence needed there (if there is any). or perhaps multiple 'if'? note sure on the guidelines here. Otherwise the code looks alright to me , cheers!
            Hide
            Sam Hemelryk added a comment -

            Thanks for checking that out, this is up for integration now

            Show
            Sam Hemelryk added a comment - Thanks for checking that out, this is up for integration now
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Integrated (finally), yay! Thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - Integrated (finally), yay! Thanks!
            Hide
            Michael de Raadt added a comment -

            Test result: User picture was shown successfully at various sizes, before and after purging caches. Well done.

            Show
            Michael de Raadt added a comment - Test result: User picture was shown successfully at various sizes, before and after purging caches. Well done.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            git & cvs repositories updated with your gorgeous code. Many thanks!

            Closing and ciao

            Show
            Eloy Lafuente (stronk7) added a comment - git & cvs repositories updated with your gorgeous code. Many thanks! Closing and ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: