Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.7, 2.1, 2.2
    • Fix Version/s: 2.2
    • Component/s: Unknown
    • Labels:
      None
    • Testing Instructions:
      Hide

      If you don't already have one, go to http://en.gravatar.com/ and create one or more Gravatar accounts with one or more email addresses you control, for testing.

      Create (or edit) two Moodle users to use the Gravatar-assigned email addresses, one with an uploaded user picture and one without. You will also want at least two other Moodle users with non-Gravatar email addresses - one with an uploaded picture, and another without. (Alternatively, you could simply alter one user as you go, to fulfil the various testing criteria as needed).

      Run a site upgrade at /admin/ to pick up the new Gravatar setting (leave at the default: NO).

      1. Gravatar OFF
      Examine user pictures (in user profile view and edit pages, in forums, course participants, etc.) to see that the existing behaviour has persisted (normal user picture, no Gravatar image).

      2. Gravatar ON by default
      Change the Gravatar admin setting to "YES, show by default".
      Examine user pictures again - the user picture should now be the Gravatar image, for those users with a Gravatar-enabled email address. Moodle users without a Gravatar-enabled email address should show their picture as normal (if any).

      3. Gravatar ON, but user-decided
      Change the Gravatar admin setting to "YES, but don't them show by default (let the user decide)".
      Examine user pictures again - there should be no Gravatar images, unless the user has specified to use the Gravatar image in their user profile. The user picture should be their uploaded picture as normal (or the system default if none uploaded).
      Change a Gravatar-enabled Moodle user to use Gravatar in their profile. This user's picture should now appear as their Gravatar image.

      Show
      If you don't already have one, go to http://en.gravatar.com/ and create one or more Gravatar accounts with one or more email addresses you control, for testing. Create (or edit) two Moodle users to use the Gravatar-assigned email addresses, one with an uploaded user picture and one without. You will also want at least two other Moodle users with non-Gravatar email addresses - one with an uploaded picture, and another without. (Alternatively, you could simply alter one user as you go, to fulfil the various testing criteria as needed). Run a site upgrade at /admin/ to pick up the new Gravatar setting (leave at the default: NO). 1. Gravatar OFF Examine user pictures (in user profile view and edit pages, in forums, course participants, etc.) to see that the existing behaviour has persisted (normal user picture, no Gravatar image). 2. Gravatar ON by default Change the Gravatar admin setting to "YES, show by default". Examine user pictures again - the user picture should now be the Gravatar image, for those users with a Gravatar-enabled email address. Moodle users without a Gravatar-enabled email address should show their picture as normal (if any). 3. Gravatar ON, but user-decided Change the Gravatar admin setting to "YES, but don't them show by default (let the user decide)". Examine user pictures again - there should be no Gravatar images, unless the user has specified to use the Gravatar image in their user profile. The user picture should be their uploaded picture as normal (or the system default if none uploaded). Change a Gravatar-enabled Moodle user to use Gravatar in their profile. This user's picture should now appear as their Gravatar image.
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull Master Branch:
      wip-MDL-21676-master-revised
    • Rank:
      5841

      Description

      I don't know if you guys have heard of Gravatar, but I think it's a very cool idea. If you haven't heard of it, check out http://www.gravatar.com/.

      I've attached a very small patch that will add a "Enable Gravatar for profile pictures" setting to the "Site Administration > Miscellaneous > Experimental" page. When the check box is checked, if a user has not uploaded a profile picture yet, Moodle will try to display a Gravatar image for that user. If no Gravatar image is found, the default profile picture is displayed.

      I decided NOT to add this to the "Modules and plugins" on moodle.org, because if it's added as a setting that's disabled by default, there's no reason not to simply add it to Moodle.

      Some of the credit for this should go to Piotr Majewski. He originally suggested it in http://moodle.org/mod/forum/discuss.php?d=135968

      1. gravatar_head.txt
        4 kB
        Dan Marsden
      2. gravatar_head2.patch
        7 kB
        Jonathan Robson
      3. gravatar_head3.patch
        8 kB
        Jonathan Robson
      4. gravatar_head4.patch
        10 kB
        Jonathan Robson
      5. gravatar_integration.patch
        3 kB
        Jonathan Robson
      6. gravatar_integration2.patch
        3 kB
        Jonathan Robson
      7. gravatar_integration3.patch
        4 kB
        Jonathan Robson
      8. MDL-21676-simple.diff
        6 kB
        Jonathan Harker

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment -

          If I read it correctly user->email is not always available in that function which means this would require changes in many places, right?

          Show
          Petr Škoda added a comment - If I read it correctly user->email is not always available in that function which means this would require changes in many places, right?
          Hide
          Jonathan Robson added a comment -

          Hmm, I guess you're correct. The email is not always there. Although, couldn't we just add a check to see if $user->email is set, and if it's not, get that field from the database? I've attached gravatar_integration2.patch to show what I'm talking about.

          Show
          Jonathan Robson added a comment - Hmm, I guess you're correct. The email is not always there. Although, couldn't we just add a check to see if $user->email is set, and if it's not, get that field from the database? I've attached gravatar_integration2.patch to show what I'm talking about.
          Hide
          Petr Škoda added a comment -

          Fetching from database could have serious performance side effects. I was thinking about gravatar integration already a few years ago when I was configuring my own for the ohloh site. I have already started the refactoring of our codebase so that all places are using the same list of fields that need to be present in the user record.

          Thanks for the patch, I am sure some admins might be interested.

          Show
          Petr Škoda added a comment - Fetching from database could have serious performance side effects. I was thinking about gravatar integration already a few years ago when I was configuring my own for the ohloh site. I have already started the refactoring of our codebase so that all places are using the same list of fields that need to be present in the user record. Thanks for the patch, I am sure some admins might be interested.
          Hide
          Jonathan Robson added a comment -

          Yeah, it could have some performance side effects, but really only on pages where a lot of profile pictures are being displayed together. Another good reason to put it on the "Experimental" settings page (and to leave it disabled by default). Could even add the performance costs as a disclaimer.

          I still think there's no reason not to add it. It simply being there would be a good reminder to improve the implementation. I know a lot of people would really like to have this as an option without having to hack the Moodle code.

          Show
          Jonathan Robson added a comment - Yeah, it could have some performance side effects, but really only on pages where a lot of profile pictures are being displayed together. Another good reason to put it on the "Experimental" settings page (and to leave it disabled by default). Could even add the performance costs as a disclaimer. I still think there's no reason not to add it. It simply being there would be a good reminder to improve the implementation. I know a lot of people would really like to have this as an option without having to hack the Moodle code.
          Hide
          Jonathan Robson added a comment -

          I came up with a slightly better solution. Looks like that function will sometimes query the database anyway so I just added another check to see if it needs to. See gravatar_integration3.patch

          Show
          Jonathan Robson added a comment - I came up with a slightly better solution. Looks like that function will sometimes query the database anyway so I just added another check to see if it needs to. See gravatar_integration3.patch
          Hide
          Jonathan Robson added a comment -

          Hopefully you don't interpret this as me being persistently annoying. It's just that I know it's hard to answer every user's request. So this is my attempt to help out and hopefully make it easier for you.

          I ran a little test:

          Basically, I added a few lines of code to a few sites that would keep track of all calls to the print_user_picture() function. It records the file and line number from which it was called (using PHP's debug_backtrace() function) and whether or not $user->email was set. The test was ran on ten different Moodle sites simultaneously. All of which were being used normally during the test. All of the information was recorded to one database. Over a period of several hours, the print_user_picture() function was called 89,018 times. At the bottom of this comment is a table showing the distribution of where the function calls were coming from, how often, and whether or not $user->email was set.

          Based on this data, $user->email is only set about 10.799% of the time. However, if one was to fix ONLY these calls:

          • lines 2862, 2866, 3155, and 3185 of mod/forum/lib.php
          • line 146 of blocks/online_users/block_online_users.php
          • lines 340, 1054, and 1056 of message/lib.php
          • line 1182 of mod/assignment/lib.php
          • line 46 of blocks/messages/block_messages.php
          • line 732 of grade/report/grader/lib.php

          ...then $user->email would be set 98.37% of the time instead. Which I think would be sufficient enough to prevent a Gravatar integration from causing a performance hit.

          File Line # Email? % Calls
          mod/forum/lib.php 2866 no 28.74
          blocks/online_users/block_online_users.php 146 no 13.723
          mod/forum/lib.php 2862 no 12.363
          message/lib.php 1054 no 10.005
          mod/forum/lib.php 3155 no 7.783
          mod/forum/lib.php 3185 no 6.414
          mod/forum/lib.php 2664 yes 3.536
          user/index.php 728 yes 2.876
          message/lib.php 340 no 2.26
          mod/assignment/lib.php 1182 no 1.824
          message/lib.php 1056 no 1.813
          mod/forum/lib.php 2660 yes 1.49
          blocks/messages/block_messages.php 46 no 1.347
          grade/report/grader/lib.php 732 no 1.299
          mod/glossary/formats/encyclopedia/encyclopedia_format.php 16 yes 1.109
          user/view.php 217 yes 0.543
          user/index.php 729 yes 0.504
          mod/forum/lib.php 2896 no 0.436
          grade/report/grader/lib.php 1017 no 0.397
          message/user.php 55 yes 0.294
          mod/chat/lib.php 625 no 0.286
          grade/report/grader/lib.php 696 no 0.264
          mod/assignment/lib.php 249 yes 0.125
          mod/glossary/view.php 448 yes 0.064
          mod/quiz/report/overview/report.php 464 no 0.055
          mod/forum/lib.php 5334 yes 0.039
          mod/forum/lib.php 2694 yes 0.035
          mod/assignment/lib.php 1183 no 0.033
          message/history.php 66 yes 0.03
          mod/assignment/lib.php 2611 no 0.03
          message/history.php 58 yes 0.029
          mod/forum/lib.php 3151 no 0.029
          user/edit_form.php 75 yes 0.028
          blocks/online_users/block_online_users.php 141 no 0.028
          mod/assignment/lib.php 880 yes 0.027
          mod/chat/gui_header_js/users.php 128 no 0.026
          mod/assignment/lib.php 967 yes 0.024
          mod/wiki/ewiki/ewiki.php 1279 no 0.022
          mod/forum/lib.php 5346 yes 0.015
          blog/lib.php 178 yes 0.01
          mod/scorm/report.php 175 no 0.01
          mod/chat/view.php 187 no 0.007
          message/lib.php 499 yes 0.006
          message/lib.php 501 yes 0.006
          mod/assignment/lib.php 968 yes 0.003
          mod/chat/report.php 175 yes 0.002
          mod/workshop/lib.php 1033 no 0.002
          mod/glossary/report.php 71 no 0.002
          mod/assignment/type/upload/assignment.class.php 118 yes 0.001
          mod/quiz/review.php 195 yes 0.001
          mod/quiz/reviewquestion.php 129 yes 0.001
          message/discussion.php 134 yes 0.001
          mod/forum/report.php 74 no 0.001
          mod/chat/gui_basic/index.php 105 no 0.001
          Show
          Jonathan Robson added a comment - Hopefully you don't interpret this as me being persistently annoying. It's just that I know it's hard to answer every user's request. So this is my attempt to help out and hopefully make it easier for you. I ran a little test: Basically, I added a few lines of code to a few sites that would keep track of all calls to the print_user_picture() function. It records the file and line number from which it was called (using PHP's debug_backtrace() function) and whether or not $user->email was set. The test was ran on ten different Moodle sites simultaneously. All of which were being used normally during the test. All of the information was recorded to one database. Over a period of several hours, the print_user_picture() function was called 89,018 times. At the bottom of this comment is a table showing the distribution of where the function calls were coming from, how often, and whether or not $user->email was set. Based on this data, $user->email is only set about 10.799% of the time. However, if one was to fix ONLY these calls: lines 2862, 2866, 3155, and 3185 of mod/forum/lib.php line 146 of blocks/online_users/block_online_users.php lines 340, 1054, and 1056 of message/lib.php line 1182 of mod/assignment/lib.php line 46 of blocks/messages/block_messages.php line 732 of grade/report/grader/lib.php ...then $user->email would be set 98.37% of the time instead. Which I think would be sufficient enough to prevent a Gravatar integration from causing a performance hit. File Line # Email? % Calls mod/forum/lib.php 2866 no 28.74 blocks/online_users/block_online_users.php 146 no 13.723 mod/forum/lib.php 2862 no 12.363 message/lib.php 1054 no 10.005 mod/forum/lib.php 3155 no 7.783 mod/forum/lib.php 3185 no 6.414 mod/forum/lib.php 2664 yes 3.536 user/index.php 728 yes 2.876 message/lib.php 340 no 2.26 mod/assignment/lib.php 1182 no 1.824 message/lib.php 1056 no 1.813 mod/forum/lib.php 2660 yes 1.49 blocks/messages/block_messages.php 46 no 1.347 grade/report/grader/lib.php 732 no 1.299 mod/glossary/formats/encyclopedia/encyclopedia_format.php 16 yes 1.109 user/view.php 217 yes 0.543 user/index.php 729 yes 0.504 mod/forum/lib.php 2896 no 0.436 grade/report/grader/lib.php 1017 no 0.397 message/user.php 55 yes 0.294 mod/chat/lib.php 625 no 0.286 grade/report/grader/lib.php 696 no 0.264 mod/assignment/lib.php 249 yes 0.125 mod/glossary/view.php 448 yes 0.064 mod/quiz/report/overview/report.php 464 no 0.055 mod/forum/lib.php 5334 yes 0.039 mod/forum/lib.php 2694 yes 0.035 mod/assignment/lib.php 1183 no 0.033 message/history.php 66 yes 0.03 mod/assignment/lib.php 2611 no 0.03 message/history.php 58 yes 0.029 mod/forum/lib.php 3151 no 0.029 user/edit_form.php 75 yes 0.028 blocks/online_users/block_online_users.php 141 no 0.028 mod/assignment/lib.php 880 yes 0.027 mod/chat/gui_header_js/users.php 128 no 0.026 mod/assignment/lib.php 967 yes 0.024 mod/wiki/ewiki/ewiki.php 1279 no 0.022 mod/forum/lib.php 5346 yes 0.015 blog/lib.php 178 yes 0.01 mod/scorm/report.php 175 no 0.01 mod/chat/view.php 187 no 0.007 message/lib.php 499 yes 0.006 message/lib.php 501 yes 0.006 mod/assignment/lib.php 968 yes 0.003 mod/chat/report.php 175 yes 0.002 mod/workshop/lib.php 1033 no 0.002 mod/glossary/report.php 71 no 0.002 mod/assignment/type/upload/assignment.class.php 118 yes 0.001 mod/quiz/review.php 195 yes 0.001 mod/quiz/reviewquestion.php 129 yes 0.001 message/discussion.php 134 yes 0.001 mod/forum/report.php 74 no 0.001 mod/chat/gui_basic/index.php 105 no 0.001
          Hide
          Jonathan Robson added a comment -

          I just realized some of the line numbers aren't exactly accurate, because the sites weren't all running the same version of Moodle. Oops! The versions ranged from 1.9.4+ to 1.9.7+. Oh well. The information is still very useful. It's not too hard to figure out where the function is being called.

          Show
          Jonathan Robson added a comment - I just realized some of the line numbers aren't exactly accurate, because the sites weren't all running the same version of Moodle. Oops! The versions ranged from 1.9.4+ to 1.9.7+. Oh well. The information is still very useful. It's not too hard to figure out where the function is being called.
          Hide
          Dan Marsden added a comment -

          Hi Petr,

          would be really good if we could get this into HEAD - I've attached a patch for Head (based on Jonathan's) that adds this to the render_user_picture function - I see someone is using some magic numbers there user->picture == 2 ?

          I presume that was so that in future a user would be able to select not to use Gravatar - but all they need to do is add their own profile image and it will override a gravatar image. We already provide the ability for the users to delete their userprofile image.

          I suppose this "could" be useful long term if there were other Gravatar style sites around and someone wanted to add those to Moodle as well, but it's unlikely someone will have time to write a nice interface to allow this so can't we just add the patch as it currently stands?

          (we don't need any of the e-mail checks that were needed for 1.9 as e-mail is already a required field for user_picture)

          Show
          Dan Marsden added a comment - Hi Petr, would be really good if we could get this into HEAD - I've attached a patch for Head (based on Jonathan's) that adds this to the render_user_picture function - I see someone is using some magic numbers there user->picture == 2 ? I presume that was so that in future a user would be able to select not to use Gravatar - but all they need to do is add their own profile image and it will override a gravatar image. We already provide the ability for the users to delete their userprofile image. I suppose this "could" be useful long term if there were other Gravatar style sites around and someone wanted to add those to Moodle as well, but it's unlikely someone will have time to write a nice interface to allow this so can't we just add the patch as it currently stands? (we don't need any of the e-mail checks that were needed for 1.9 as e-mail is already a required field for user_picture)
          Hide
          Petr Škoda added a comment -

          Hmm, I would prefer if we used the "2" constant and would allow each user to specify if they want to use gravater or standard moodle icon or nothing. The reason is that you would not be able to prevent gravatar image easily == potential privacy issue. In the UI it would be a 3 state selection - Gravatar/image/none. And then we would enable the file picker only if Image option selected.

          Show
          Petr Škoda added a comment - Hmm, I would prefer if we used the "2" constant and would allow each user to specify if they want to use gravater or standard moodle icon or nothing. The reason is that you would not be able to prevent gravatar image easily == potential privacy issue. In the UI it would be a 3 state selection - Gravatar/image/none. And then we would enable the file picker only if Image option selected.
          Hide
          Dan Marsden added a comment -

          Hi Petr,

          I disagree. - If users have registered for Gravatar - they want to use the service. If they want to override their image (and not use gravatar (if enabled on the site) they can upload their own image that overrides the gravatar one.

          The advantage of using Gravatar is lost a bit if we rely on individual user decisions to use their "gravatar" image.

          The patch above is something we could also squeeze into 2.0 without too much effort - would be good to get this capability in Moodle! - adding the ability for users to individually "choose" whether to use it or not could come in future?

          Show
          Dan Marsden added a comment - Hi Petr, I disagree. - If users have registered for Gravatar - they want to use the service. If they want to override their image (and not use gravatar (if enabled on the site) they can upload their own image that overrides the gravatar one. The advantage of using Gravatar is lost a bit if we rely on individual user decisions to use their "gravatar" image. The patch above is something we could also squeeze into 2.0 without too much effort - would be good to get this capability in Moodle! - adding the ability for users to individually "choose" whether to use it or not could come in future?
          Hide
          Dan Marsden added a comment -

          ..obviously the config setting to use gravatar would be turned off by default (maximum privacy setting)

          Show
          Dan Marsden added a comment - ..obviously the config setting to use gravatar would be turned off by default (maximum privacy setting)
          Hide
          Petr Škoda added a comment -

          Let's ask MD, I personally do not care about my own gravatar image, but I would not like to like to deal with the user complains.

          Show
          Petr Škoda added a comment - Let's ask MD, I personally do not care about my own gravatar image, but I would not like to like to deal with the user complains.
          Hide
          Petr Škoda added a comment -

          In any case thank you for the patch!!

          Show
          Petr Škoda added a comment - In any case thank you for the patch!!
          Hide
          Jonathan Robson added a comment -

          I have to agree with Dan. I think the spirit of Gravatar is that it just works without the user having to do anything extra. Requiring them to opt in doesn't really fit with the spirit of Gravatar. There's also the fact that the majority of organizations that use Moodle are schools and corporations. At the schools, most of the users use their school email, and at the corporations, most of the users are using a work email. In those situations, they can associate a specific image with that email address that's appropriate for that organization.

          Although, I know how you feel about dealing with user complaints, which is why I think that the best solution is one you guys may not have considered yet: a second site-wide admin setting that lets the site administrator choose whether not users should have to opt in.

          Show
          Jonathan Robson added a comment - I have to agree with Dan. I think the spirit of Gravatar is that it just works without the user having to do anything extra. Requiring them to opt in doesn't really fit with the spirit of Gravatar. There's also the fact that the majority of organizations that use Moodle are schools and corporations. At the schools, most of the users use their school email, and at the corporations, most of the users are using a work email. In those situations, they can associate a specific image with that email address that's appropriate for that organization. Although, I know how you feel about dealing with user complaints, which is why I think that the best solution is one you guys may not have considered yet: a second site-wide admin setting that lets the site administrator choose whether not users should have to opt in.
          Hide
          Jonathan Robson added a comment -

          ...forgot to finish my thought...
          That way it's on the site administrators and not you guys.

          Show
          Jonathan Robson added a comment - ...forgot to finish my thought... That way it's on the site administrators and not you guys.
          Hide
          Dan Marsden added a comment -

          Hi Jonathan, unless you have time to develop something like that for 2.0 (before release) - I'd suggest that was shelved as something to do in future - I'd like to see gravatar support in 2.0 and wouldn't want to wait for that extra functionality to be added. (which I don't really think is necessary) - people already have the ability to manage this by uploading their own image or customising their gravatar settings as you mention. It seems like a feature that isn't really needed to me.

          Show
          Dan Marsden added a comment - Hi Jonathan, unless you have time to develop something like that for 2.0 (before release) - I'd suggest that was shelved as something to do in future - I'd like to see gravatar support in 2.0 and wouldn't want to wait for that extra functionality to be added. (which I don't really think is necessary) - people already have the ability to manage this by uploading their own image or customising their gravatar settings as you mention. It seems like a feature that isn't really needed to me.
          Hide
          Jonathan Robson added a comment -

          I've attached a patch of what I'm suggesting. It turns the admin setting checkbox into a select (drop-down) menu with 3 options:

          • NO, don't show any Gravatar pictures
          • YES, show them by default
          • YES, but don't them by default (let the user decide)

          When one of the "YES, ..." options is selected, it displays a checkbox on the edit profile page for the users.

          Show
          Jonathan Robson added a comment - I've attached a patch of what I'm suggesting. It turns the admin setting checkbox into a select (drop-down) menu with 3 options: NO, don't show any Gravatar pictures YES, show them by default YES, but don't them by default (let the user decide) When one of the "YES, ..." options is selected, it displays a checkbox on the edit profile page for the users.
          Hide
          Jonathan Harker added a comment -

          The point of Gravatar is that users who have avatar pictures with Gravatar do not have to enable or tweak anything on each site they use, it just works. The overall admin setting in Moodle is a good idea, but an individual user profile setting is redundant. Gravatar pictures are administered at the Gravatar site by the user, not in each individual site (Moodle in this case).

          Show
          Jonathan Harker added a comment - The point of Gravatar is that users who have avatar pictures with Gravatar do not have to enable or tweak anything on each site they use, it just works. The overall admin setting in Moodle is a good idea, but an individual user profile setting is redundant. Gravatar pictures are administered at the Gravatar site by the user, not in each individual site (Moodle in this case).
          Hide
          Jonathan Robson added a comment -

          Please ignore "gravatar_head2.patch" as it is broken. I've uploaded a fixed one: "gravatar_head3.patch"

          Show
          Jonathan Robson added a comment - Please ignore "gravatar_head2.patch" as it is broken. I've uploaded a fixed one: "gravatar_head3.patch"
          Hide
          Jonathan Robson added a comment -

          Jonathan, I agree. Especially after having written that code that added the user option. It just makes it too complicated, and that's not what Gravatar is supposed to be. You're supposed to just manage your images on the Gravatar site. You can have different images associated with different email addresses. If the user doesn't like the image being displayed, they can upload a different one, change it on the Gravatar site, or use a different email address. I think the option in the user profile is necessary. Every app that I can think of that uses Gravatar simply has one global setting to enable or disable it and that's it. It should never have to be more complicated than that, and that's the point.

          Show
          Jonathan Robson added a comment - Jonathan, I agree. Especially after having written that code that added the user option. It just makes it too complicated, and that's not what Gravatar is supposed to be. You're supposed to just manage your images on the Gravatar site. You can have different images associated with different email addresses. If the user doesn't like the image being displayed, they can upload a different one, change it on the Gravatar site, or use a different email address. I think the option in the user profile is necessary. Every app that I can think of that uses Gravatar simply has one global setting to enable or disable it and that's it. It should never have to be more complicated than that, and that's the point.
          Hide
          Dan Marsden added a comment -

          yeah - I'm against having to add the user selectable option too - I'll try to ping MD next time I see him for a final decision.

          Show
          Dan Marsden added a comment - yeah - I'm against having to add the user selectable option too - I'll try to ping MD next time I see him for a final decision.
          Hide
          Dan Marsden added a comment -

          Chatlog from the Boss in Dev Chat.

          (15:56:04) ***moodler pretends not to see new features
          (15:57:34) moodler: * NO, don't show any Gravatar pictures

          • YES, show them by default
          • YES, but don't them by default (let the user decide)

          (15:57:36) moodler: ....seems ok?
          (15:58:16) moodler: i guess some people may not want to mix up their public internet identity with their picture at a schol

          so we're adding the user selectable option but slightly differently to JR's patch - Jonathan do you have time to modify your patch? - if not I'll try to take a better look later this week.
          if option 2 above is set then the user would just upload their own profile image if they wanted to override the gravatar one. - if option 3 is set we give the user the ability to "enable" their gravatar image, disabled by default - if enabled it should disable the upload element and completely override any uploaded image.

          Show
          Dan Marsden added a comment - Chatlog from the Boss in Dev Chat. (15:56:04) ***moodler pretends not to see new features (15:57:34) moodler: * NO, don't show any Gravatar pictures YES, show them by default YES, but don't them by default (let the user decide) (15:57:36) moodler: ....seems ok? (15:58:16) moodler: i guess some people may not want to mix up their public internet identity with their picture at a schol so we're adding the user selectable option but slightly differently to JR's patch - Jonathan do you have time to modify your patch? - if not I'll try to take a better look later this week. if option 2 above is set then the user would just upload their own profile image if they wanted to override the gravatar one. - if option 3 is set we give the user the ability to "enable" their gravatar image, disabled by default - if enabled it should disable the upload element and completely override any uploaded image.
          Hide
          Petr Škoda added a comment -

          great, let's get it in asap...

          Show
          Petr Škoda added a comment - great, let's get it in asap...
          Hide
          Petr Škoda added a comment -

          reassigning, please let me review it before commit, thanks everybody

          Show
          Petr Škoda added a comment - reassigning, please let me review it before commit, thanks everybody
          Hide
          Jonathan Robson added a comment -

          Dan, you said...
          "so we're adding the user selectable option but slightly differently to JR's patch - Jonathan do you have time to modify your patch? - if not I'll try to take a better look later this week.
          if option 2 above is set then the user would just upload their own profile image if they wanted to override the gravatar one. - if option 3 is set we give the user the ability to "enable" their gravatar image, disabled by default - if enabled it should disable the upload element and completely override any uploaded image."

          I believe I've made the changes you requested (if I'm understanding them correctly):

          • If option 2 ("YES, show them by default") is set, no checkbox appears on the edit profile page, but the user can override the Gravatar image by uploading their own. Also, with this option, an explanation is displayed telling the user "Your Gravatar picture is currently being used (if available). You can override this by uploading a different picture below."
          • If option 3 ("YES, but don't them by default (let the user decide)") is set, the Gravatar checkbox shows up on the edit profile page, and the file uploader will not display when they've selected to use Gravatar.

          Is this what you wanted? The patch is "gravatar_head4.patch".

          Show
          Jonathan Robson added a comment - Dan, you said... "so we're adding the user selectable option but slightly differently to JR's patch - Jonathan do you have time to modify your patch? - if not I'll try to take a better look later this week. if option 2 above is set then the user would just upload their own profile image if they wanted to override the gravatar one. - if option 3 is set we give the user the ability to "enable" their gravatar image, disabled by default - if enabled it should disable the upload element and completely override any uploaded image." I believe I've made the changes you requested (if I'm understanding them correctly): If option 2 ("YES, show them by default") is set, no checkbox appears on the edit profile page, but the user can override the Gravatar image by uploading their own. Also, with this option, an explanation is displayed telling the user "Your Gravatar picture is currently being used (if available). You can override this by uploading a different picture below." If option 3 ("YES, but don't them by default (let the user decide)") is set, the Gravatar checkbox shows up on the edit profile page, and the file uploader will not display when they've selected to use Gravatar. Is this what you wanted? The patch is "gravatar_head4.patch".
          Hide
          Jonathan Robson added a comment -

          Also, just a small request... when you commit this, do you mind putting a comment in the code somewhere (wherever is appropriate) giving me credit for the work I've done on this? Thanks.

          Show
          Jonathan Robson added a comment - Also, just a small request... when you commit this, do you mind putting a comment in the code somewhere (wherever is appropriate) giving me credit for the work I've done on this? Thanks.
          Hide
          Petr Škoda added a comment -

          credit: we should try to push Martin more to adopt Git as the main version control, but sssh, it was not my idea....

          Show
          Petr Škoda added a comment - credit: we should try to push Martin more to adopt Git as the main version control, but sssh, it was not my idea....
          Hide
          Dan Marsden added a comment -

          looks closer! - I like the hiding of the delete checkbox if no image has been uploaded but we should probably put that on a seperate bug instead of bundle it with this one.

          also - hiding the filepicker like that isn't quite right - we should use mform disabledif style statements to disable it based on the usegravatar element - if ticked then the filepicker should be disabled - if unticked then the filepicker should become enabled - means that a user doesn't have to reload the page to upload an image if they decide to use their own uploaded one.

          thanks Jonathan!

          Show
          Dan Marsden added a comment - looks closer! - I like the hiding of the delete checkbox if no image has been uploaded but we should probably put that on a seperate bug instead of bundle it with this one. also - hiding the filepicker like that isn't quite right - we should use mform disabledif style statements to disable it based on the usegravatar element - if ticked then the filepicker should be disabled - if unticked then the filepicker should become enabled - means that a user doesn't have to reload the page to upload an image if they decide to use their own uploaded one. thanks Jonathan!
          Hide
          Jonathan Robson added a comment -

          That would be great if disabledIf() worked with filepicker elements, but it does NOT. Technically, it does disable the field named "imagefile", but that's a hidden field since it's a filepicker. I'm not sure how this should be fixed since the filepicker element is not just a standard HTML form element.

          There's also the fact that disabledIf() does NOT reset the value of the disabled element when it gets disabled. For instance, if you added the following code (just for testing it):

          $mform->disabledIf('imagealt', 'usegravatar', 'checked');

          ... then enter some text into the "Picture description" (imagealt) field, and then check the "Use Gravatar..." checkbox, the element does indeed get disabled but the value of the disabled element does NOT change. This is a problem because, in my opinion, the value of a disabled element probably should NOT get submitted with the form. There are multiple places that would cause problems.

          Show
          Jonathan Robson added a comment - That would be great if disabledIf() worked with filepicker elements, but it does NOT. Technically, it does disable the field named "imagefile", but that's a hidden field since it's a filepicker. I'm not sure how this should be fixed since the filepicker element is not just a standard HTML form element. There's also the fact that disabledIf() does NOT reset the value of the disabled element when it gets disabled. For instance, if you added the following code (just for testing it): $mform->disabledIf('imagealt', 'usegravatar', 'checked'); ... then enter some text into the "Picture description" (imagealt) field, and then check the "Use Gravatar..." checkbox, the element does indeed get disabled but the value of the disabled element does NOT change. This is a problem because, in my opinion, the value of a disabled element probably should NOT get submitted with the form. There are multiple places that would cause problems.
          Hide
          Dan Marsden added a comment -

          Aaron has a patch on MDL-26167 that adds support for disabledif to the filepicker. When this goes in, hopefully we can get Gravatar accepted in core as well.

          Show
          Dan Marsden added a comment - Aaron has a patch on MDL-26167 that adds support for disabledif to the filepicker. When this goes in, hopefully we can get Gravatar accepted in core as well.
          Hide
          Jonathan Harker added a comment -

          Hi all,
          I've just pushed a branch here for review which includes Jonathan Robson's head4.patch more or less as written. The fix for disabledIf on filepicker controls is now in upstream, so this can/should be amended.

          Show
          Jonathan Harker added a comment - Hi all, I've just pushed a branch here for review which includes Jonathan Robson's head4.patch more or less as written. The fix for disabledIf on filepicker controls is now in upstream, so this can/should be amended.
          Hide
          Jonathan Harker added a comment -

          Can I also suggest:

          define('GRAVATAR_OFF', 0);
          define('GRAVATAR_ON_GLOBAL', 1);
          define('GRAVATAR_ON_OPTIONAL', 2);

          JR - do you have an account on gitorious or github?

          Show
          Jonathan Harker added a comment - Can I also suggest: define('GRAVATAR_OFF', 0); define('GRAVATAR_ON_GLOBAL', 1); define('GRAVATAR_ON_OPTIONAL', 2); JR - do you have an account on gitorious or github?
          Hide
          Jonathan Harker added a comment -

          I've pushed a commit to tidy up isset() and use constants.

          Show
          Jonathan Harker added a comment - I've pushed a commit to tidy up isset() and use constants.
          Hide
          Dan Marsden added a comment -

          Hi JH, now that the patch has gone in that allows the filepicker to be disabled in mform - could you use it to disable/enable the filepicker depending on the settings made?

          Show
          Dan Marsden added a comment - Hi JH, now that the patch has gone in that allows the filepicker to be disabled in mform - could you use it to disable/enable the filepicker depending on the settings made?
          Hide
          Jonathan Robson added a comment -

          Yes, I do have a GitHub account. Dan, would you prefer that submit my patch as a pull request?

          JH, why did you take out the isset()'s? If the "enablegravatar" property doesn't exist yet, PHP will trigger a notice.

          Show
          Jonathan Robson added a comment - Yes, I do have a GitHub account. Dan, would you prefer that submit my patch as a pull request? JH, why did you take out the isset()'s? If the "enablegravatar" property doesn't exist yet, PHP will trigger a notice.
          Hide
          Jonathan Harker added a comment -

          Hi Jonathan - because the code in admin/settings/users.php will take care of ensuring the property always exists (once the site is upgraded).

          Show
          Jonathan Harker added a comment - Hi Jonathan - because the code in admin/settings/users.php will take care of ensuring the property always exists (once the site is upgraded).
          Hide
          Jonathan Robson added a comment -

          If we're going to use constants for $CFG->enablegravatar, shouldn't we use constants for $USER->picture as well? Or are the constants even necessary?

          Show
          Jonathan Robson added a comment - If we're going to use constants for $CFG->enablegravatar , shouldn't we use constants for $USER->picture as well? Or are the constants even necessary?
          Hide
          Jonathan Robson added a comment -

          Here's my updated code: https://github.com/jnrbsn/moodle/compare/master...gravatar

          I added the constants as separate commits to make it easier if you don't like them.

          Show
          Jonathan Robson added a comment - Here's my updated code: https://github.com/jnrbsn/moodle/compare/master...gravatar I added the constants as separate commits to make it easier if you don't like them.
          Hide
          Jonathan Harker added a comment - - edited

          There should not be magic numbers in code. USER->picture should use constants as well (what does "2" mean???), but one bug at a time... ...update: Oh, I see you've added USER->picture constants as well. Maybe we should do those as a separate bug as there's USER->picture checks in a lot of places? Just a thought.

          Show
          Jonathan Harker added a comment - - edited There should not be magic numbers in code. USER->picture should use constants as well (what does "2" mean???), but one bug at a time... ...update: Oh, I see you've added USER->picture constants as well. Maybe we should do those as a separate bug as there's USER->picture checks in a lot of places? Just a thought.
          Hide
          Jonathan Robson added a comment -

          I'm fine with doing that separately if that's what everyone wants. Dan and Petr, what do you think?

          I can take that commit out of that branch if that makes things easier for you guys.

          Show
          Jonathan Robson added a comment - I'm fine with doing that separately if that's what everyone wants. Dan and Petr, what do you think? I can take that commit out of that branch if that makes things easier for you guys.
          Hide
          Dan Marsden added a comment -

          the magic numbers don't mean a lot without the Gravatar code so we "should" be able to do that as a single pull request - as long as we fix all instances of i$USER->picture - JR can you find the other instances and replace them with the new constants? Once that's done I'll "request peer review" - JH do you have time to do this? - then we can submit this for integration with master!

          Show
          Dan Marsden added a comment - the magic numbers don't mean a lot without the Gravatar code so we "should" be able to do that as a single pull request - as long as we fix all instances of i$USER->picture - JR can you find the other instances and replace them with the new constants? Once that's done I'll "request peer review" - JH do you have time to do this? - then we can submit this for integration with master!
          Hide
          Jonathan Harker added a comment -

          Yes, sounds like a cunning plan.

          Show
          Jonathan Harker added a comment - Yes, sounds like a cunning plan.
          Hide
          Jonathan Robson added a comment -

          Ok. I have fixed all other instances of $USER->picture to use the new USER_PICTURE_* constants. The commit is in that same branch to which I linked previously. Here's the link again just in case: https://github.com/jnrbsn/moodle/compare/master...gravatar

          Also, in case you're wondering, here's the regular expression I used to search for the instances:

          /\$user\-\>picture\b/gi
          Show
          Jonathan Robson added a comment - Ok. I have fixed all other instances of $USER->picture to use the new USER_PICTURE_* constants. The commit is in that same branch to which I linked previously. Here's the link again just in case: https://github.com/jnrbsn/moodle/compare/master...gravatar Also, in case you're wondering, here's the regular expression I used to search for the instances: /\$user\-\>picture\b/gi
          Hide
          Jonathan Robson added a comment -

          Do you want me to submit a pull request on GitHub?

          Show
          Jonathan Robson added a comment - Do you want me to submit a pull request on GitHub?
          Hide
          Dan Marsden added a comment -

          no - Moodle uses the tracker to manage integration requests - not the pull request stuff on github

          Show
          Dan Marsden added a comment - no - Moodle uses the tracker to manage integration requests - not the pull request stuff on github
          Hide
          Dan Marsden added a comment -

          assigning to JH for peer-review - JH do you have time to install and test this? - we also need to add instructions for testing this above if poss - these instructions are for the QA and integration review to understand how to test without having to read all the comments on the bug.

          Show
          Dan Marsden added a comment - assigning to JH for peer-review - JH do you have time to install and test this? - we also need to add instructions for testing this above if poss - these instructions are for the QA and integration review to understand how to test without having to read all the comments on the bug.
          Hide
          Dan Marsden added a comment -

          Jr - I'm not sure if you're able to add test instructions - try hitting the "edit" link on this tracker issue and see if you have the field to add testing instructions?

          Show
          Dan Marsden added a comment - Jr - I'm not sure if you're able to add test instructions - try hitting the "edit" link on this tracker issue and see if you have the field to add testing instructions?
          Hide
          Dan Marsden added a comment -

          just saw the hovercard things on gravatar - could be cool to do a Moodle version that pulled the info from a users Moodle profile....something for another bug...

          Show
          Dan Marsden added a comment - just saw the hovercard things on gravatar - could be cool to do a Moodle version that pulled the info from a users Moodle profile....something for another bug...
          Hide
          Jonathan Harker added a comment -

          The only thing is perhaps in lib/outputrenderers.php where the $src variable is potentially assigned twice when using Gravatar user pictures, with an unnecessary call to the pix_url() function, which would be better inside an if-else. However I don't see it as a show-stopper.

          Show
          Jonathan Harker added a comment - The only thing is perhaps in lib/outputrenderers.php where the $src variable is potentially assigned twice when using Gravatar user pictures, with an unnecessary call to the pix_url() function, which would be better inside an if-else. However I don't see it as a show-stopper.
          Hide
          Jonathan Harker added a comment -

          Hi Jonathan and Dan - I've put up some testing instructions, please do feel free to alter/update add as necessary. Cheers!

          Show
          Jonathan Harker added a comment - Hi Jonathan and Dan - I've put up some testing instructions, please do feel free to alter/update add as necessary. Cheers!
          Hide
          Jonathan Harker added a comment -

          Any progress here? What needs to happen next?

          Show
          Jonathan Harker added a comment - Any progress here? What needs to happen next?
          Hide
          Dan Marsden added a comment -

          As long as you're happy with it - I can submit for integration review (or you can pressing the button up the top) although we've missed this weeks review so you'll need to rebase it again based after this weeks review has finished.

          Also - I think most of HQ are at the AU moot next week so it might not make it in next weeks review either.

          Show
          Dan Marsden added a comment - As long as you're happy with it - I can submit for integration review (or you can pressing the button up the top) although we've missed this weeks review so you'll need to rebase it again based after this weeks review has finished. Also - I think most of HQ are at the AU moot next week so it might not make it in next weeks review either.
          Hide
          Jonathan Harker added a comment -

          I've followed my own testing instructions and it works for me. Who do I have to prod?

          Show
          Jonathan Harker added a comment - I've followed my own testing instructions and it works for me. Who do I have to prod?
          Hide
          Dan Marsden added a comment -

          I'll submit it for integration today if you rebase on master - although you might have missed this weeks integration review so might have to rebase again next week!

          Show
          Dan Marsden added a comment - I'll submit it for integration today if you rebase on master - although you might have missed this weeks integration review so might have to rebase again next week!
          Show
          Jonathan Harker added a comment - Updated to master here: http://git.catalyst.net.nz/gw?p=moodle-r2.git;a=shortlog;h=refs/heads/MDL-21676_1
          Hide
          Dan Marsden added a comment -

          looks like more work is needed here sorry! - Sam has a patch on MDL-28043 that improves user pic a bit but this patch will need a (hopefully small) amount of work to merge with it.

          Show
          Dan Marsden added a comment - looks like more work is needed here sorry! - Sam has a patch on MDL-28043 that improves user pic a bit but this patch will need a (hopefully small) amount of work to merge with it.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          After commenting with Dan, yes, this needs proper integration of the URL generation into the new stuff to land (hopefully this week).

          So reopening, thanks for the effort!

          Show
          Eloy Lafuente (stronk7) added a comment - After commenting with Dan, yes, this needs proper integration of the URL generation into the new stuff to land (hopefully this week). So reopening, thanks for the effort!
          Hide
          Jonathan Harker added a comment -

          I've got a bit of time this avo to look at that - I'll try a rebase on Sam's patch.

          Show
          Jonathan Harker added a comment - I've got a bit of time this avo to look at that - I'll try a rebase on Sam's patch.
          Hide
          Jonathan Harker added a comment - - edited

          Here's the Gravatar stuff rebased on Sam's MDL-28043 work: MDL-21676-rebased-on-MDL-28043
          Note that I needed to shift the $src assignment in lib/outputrenderers.php to above the if...$class assignment, as the Gravatar image URL uses the Moodle image as a fallback redirect.

          Show
          Jonathan Harker added a comment - - edited Here's the Gravatar stuff rebased on Sam's MDL-28043 work: MDL-21676-rebased-on-MDL-28043 Note that I needed to shift the $src assignment in lib/outputrenderers.php to above the if...$class assignment, as the Gravatar image URL uses the Moodle image as a fallback redirect.
          Hide
          Dan Marsden added a comment -

          NOTE TO INTEGRATOR - This patch includes Sam's patch from MDL-28043 - thanks!

          Show
          Dan Marsden added a comment - NOTE TO INTEGRATOR - This patch includes Sam's patch from MDL-28043 - thanks!
          Hide
          Jonathan Harker added a comment -

          Hang on - I see I need to re-do that patch and put it in $userpicture->get_url in lib/outputcomponents.php - Dan, can you hold up for a bit. Cheers

          Show
          Jonathan Harker added a comment - Hang on - I see I need to re-do that patch and put it in $userpicture->get_url in lib/outputcomponents.php - Dan, can you hold up for a bit. Cheers
          Hide
          Jonathan Harker added a comment -

          Sorry - let's try that again: MDL-21676-rebased-on-MDL-28043

          • Rebased on Sam's wip-MDL-28043-master branch
          • Gravatar URL code moved to the correct place in the new MDL-28043 get_url method in lib/outputcomponents.php
          • Not tested
          Show
          Jonathan Harker added a comment - Sorry - let's try that again: MDL-21676-rebased-on-MDL-28043 Rebased on Sam's wip- MDL-28043 -master branch Gravatar URL code moved to the correct place in the new MDL-28043 get_url method in lib/outputcomponents.php Not tested
          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
          Jonathan Harker added a comment -

          This is a little complicated by the fact that this feature is now based on the MDL-28043 fix, but here are the relevant branches:

          Show
          Jonathan Harker added a comment - This is a little complicated by the fact that this feature is now based on the MDL-28043 fix, but here are the relevant branches: wip-MDL-28043-master_1 - Sam's MDL-28043 fix rebased onto latest 2.2dev weekly MDL-21676-rebased-on-MDL-28043_2 - This MDL-21676 Gravatar feature rebased onto the above MDL-28043 branch.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Lol, this is becoming complex, eh!

          I'm awaiting for some comments about MDL-28043 before finally integrating it. So I hope this will land too (surely will need one more rebranching here, sorry).

          Note also +1 is required from our benevolent dictator for this to be integrated (I've just requested his opinion here).

          Crossing my toes, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Lol, this is becoming complex, eh! I'm awaiting for some comments about MDL-28043 before finally integrating it. So I hope this will land too (surely will need one more rebranching here, sorry). Note also +1 is required from our benevolent dictator for this to be integrated (I've just requested his opinion here). Crossing my toes, ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi guys, I'm reopening this because, after chat with Sam we have agreed to reopen MDL-28043 once more to discuss and push the final solution.

          So I'd recommend you to wait until it lands upstream definitively to avoid more headaches. Also Sam will be able to help here once the blocker has been fixed.

          Many thanks for the support and patience, it has been a cumulus of circumstances preventing us to integrate such issue.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi guys, I'm reopening this because, after chat with Sam we have agreed to reopen MDL-28043 once more to discuss and push the final solution. So I'd recommend you to wait until it lands upstream definitively to avoid more headaches. Also Sam will be able to help here once the blocker has been fixed. Many thanks for the support and patience, it has been a cumulus of circumstances preventing us to integrate such issue. Ciao
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          Sorry about all of the back and forwards that MDL-28043 is causing.
          We very much wanted to tidy that up and get it right this time!
          It's up for integration review now and I've very confident it is going in this week.

          Jonathan if you like I will rebase this for you when it goes upstream and put it up for integration.
          Otherwise yell out if you need a hand or have any questions. I'm very keen to see these changes go in, it'll be great to finally support gravatar profile pics.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, Sorry about all of the back and forwards that MDL-28043 is causing. We very much wanted to tidy that up and get it right this time! It's up for integration review now and I've very confident it is going in this week. Jonathan if you like I will rebase this for you when it goes upstream and put it up for integration. Otherwise yell out if you need a hand or have any questions. I'm very keen to see these changes go in, it'll be great to finally support gravatar profile pics. Cheers Sam
          Hide
          Dan Marsden added a comment -

          assigning to Sam - would be great if you could rebase and resubmit for integration - would be nice to finally get this into core!

          Thanks to all who have worked on this!

          Show
          Dan Marsden added a comment - assigning to Sam - would be great if you could rebase and resubmit for integration - would be nice to finally get this into core! Thanks to all who have worked on this!
          Hide
          Jonathan Harker added a comment -

          I've rebased this onto Sam's newest MDL-28043 branch (Sep 6): MDL-21676-rebased-on-MDL-28043_3

          Show
          Jonathan Harker added a comment - I've rebased this onto Sam's newest MDL-28043 branch (Sep 6): MDL-21676-rebased-on-MDL-28043_3
          Hide
          Sam Hemelryk added a comment -

          Hi Jonathan,

          I've just been looking at this now and there are a few problems with the code presently:

          1. useredit_update_picture $USER is the current user and not necessarily the user being editing, when checking $USER->picture == 2 really you need to be checking $usernew->picture == 2. At a glance I'm not sure that will be set however so you may need to go to the database for it.
          2. useredit_shared_defintion Same as above unfortunately $USER is the current user and not necessarily the user being edited. This code works well for the current user but doesn't work as expected when editing other users. Unfortunately at this point in the code the user being edited isn't available (or at least I don't think they are) in which case there are really two options a) Pass the user being editing through the edit form so you can access it there b) remove the smarts that are not including fields based upon the user.
          3. user_picture::get_url there is a typo in the line determining if gravatar is being used $user->picture needs to be $this->user->picture
          4. When creating the gravatar URL we should be using their secure URL if the Moodle site is set up to for ssl. http://secure.gravatar.com (I think we can strpos $FULLME)
          5. Within the lang files the new strings with anchors the target attribute needs to be removed as its not XHTML strict.
          6. There is a typo in the gravatarletuserdecide string

          If you have time to fix those up that would be great, otherwise I'll see if I can get it done over the weekend so that this can be put up for integration this week.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Jonathan, I've just been looking at this now and there are a few problems with the code presently: useredit_update_picture $USER is the current user and not necessarily the user being editing, when checking $USER->picture == 2 really you need to be checking $usernew->picture == 2. At a glance I'm not sure that will be set however so you may need to go to the database for it. useredit_shared_defintion Same as above unfortunately $USER is the current user and not necessarily the user being edited. This code works well for the current user but doesn't work as expected when editing other users. Unfortunately at this point in the code the user being edited isn't available (or at least I don't think they are) in which case there are really two options a) Pass the user being editing through the edit form so you can access it there b) remove the smarts that are not including fields based upon the user. user_picture::get_url there is a typo in the line determining if gravatar is being used $user->picture needs to be $this->user->picture When creating the gravatar URL we should be using their secure URL if the Moodle site is set up to for ssl. http://secure.gravatar.com (I think we can strpos $FULLME) Within the lang files the new strings with anchors the target attribute needs to be removed as its not XHTML strict. There is a typo in the gravatarletuserdecide string If you have time to fix those up that would be great, otherwise I'll see if I can get it done over the weekend so that this can be put up for integration this week. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've made the required changes and also fixed up a couple more issues that have appeared when editing another users profile and when serving users pictures who have selected to use gravatar after gravatar has been disabled.

          It's up for peer-review now.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've made the required changes and also fixed up a couple more issues that have appeared when editing another users profile and when serving users pictures who have selected to use gravatar after gravatar has been disabled. It's up for peer-review now. Cheers Sam
          Hide
          Jonathan Harker added a comment -

          Hi Sam - cheers for the fix-ups, commit diff looks good. Only a minor thing but on line 322 of user/editlib.php you don't need to check !empty($CFG->enablegravatar) as the property will always exist(added in admin/settings)

          Show
          Jonathan Harker added a comment - Hi Sam - cheers for the fix-ups, commit diff looks good. Only a minor thing but on line 322 of user/editlib.php you don't need to check !empty($CFG->enablegravatar) as the property will always exist(added in admin/settings)
          Hide
          Sam Hemelryk added a comment -

          Thanks for looking at the patch Jonathan,

          Indeed that check is not needed, I had read over that section too fast and not spotted the check to skip user pic during installation.

          I've pushed up an branch that removes the unnecessary check and am putting this up for integration now

          Once the next weekly has been released I'll revise this just once more to bump the version number to ensure during testing (before the weekly version bump) users are forced to upgrade and set the new setting.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for looking at the patch Jonathan, Indeed that check is not needed, I had read over that section too fast and not spotted the check to skip user pic during installation. I've pushed up an branch that removes the unnecessary check and am putting this up for integration now Once the next weekly has been released I'll revise this just once more to bump the version number to ensure during testing (before the weekly version bump) users are forced to upgrade and set the new setting. Cheers Sam
          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
          Jonathan Harker added a comment -

          No need to rebase, it merges cleanly.

          Show
          Jonathan Harker added a comment - No need to rebase, it merges cleanly.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yeah, thanks for keeping your github clone in such a beautiful state, so I clicked the diff URL to overview the patch and my Safari ba-boom!

          Are there any gravatar for finger@integration.moodle.org ? LOL

          Show
          Eloy Lafuente (stronk7) added a comment - Yeah, thanks for keeping your github clone in such a beautiful state, so I clicked the diff URL to overview the patch and my Safari ba-boom! Are there any gravatar for finger@integration.moodle.org ? LOL
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Oki, I think that from a coding perspective the patch is pretty ready.

          But there is something in the implemented behavior that I don't get:

          • When gravatar is enabled globally, the the fallback is: uploaded => gravatar => default.
          • When gravatar is enabled user by user, then the fallback is: gravatar => uploaded => default

          Why do we have these different fallbacks depending of the gravatars being enabled globally or user by user?

          More yet, why do we need the user by user setting? IMO the uploaded => gravatar => default fallback covers it perfectly, so, if the user has uploaded an icon, it is used, else gravatar (if exists and enabled globally) and finally the default one. So, the user can decide, both by uploading one icon or by configuring his gravatar what will be shown. Hence I cannot see why the "one by one" alternative is useful for anything. User also decides in the global alternative.

          Surely I'm missing something... would be great if you can illustrate me.

          Also, when playing with the hack, I found the "default" wording and the help strings a bit unintuitive, because really it is not "use gravatar by default", but use it as fallback if no uploaded file is available... but that's a minor detail. The previous paragraph seems far more important, the need to have the "one by one" option.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Oki, I think that from a coding perspective the patch is pretty ready. But there is something in the implemented behavior that I don't get: When gravatar is enabled globally, the the fallback is: uploaded => gravatar => default. When gravatar is enabled user by user, then the fallback is: gravatar => uploaded => default Why do we have these different fallbacks depending of the gravatars being enabled globally or user by user? More yet, why do we need the user by user setting? IMO the uploaded => gravatar => default fallback covers it perfectly, so, if the user has uploaded an icon, it is used, else gravatar (if exists and enabled globally) and finally the default one. So, the user can decide, both by uploading one icon or by configuring his gravatar what will be shown. Hence I cannot see why the "one by one" alternative is useful for anything. User also decides in the global alternative. Surely I'm missing something... would be great if you can illustrate me. Also, when playing with the hack, I found the "default" wording and the help strings a bit unintuitive, because really it is not "use gravatar by default", but use it as fallback if no uploaded file is available... but that's a minor detail. The previous paragraph seems far more important, the need to have the "one by one" option. Ciao
          Hide
          Dan Marsden added a comment -

          user by user setting was added as per Petr's request/direction - didn't make sense to me either but our chances of getting this reviewed for inclusion were higher if we did it... in the end MD made a call about the user settings too (see comments above for details) - if you can convince MD to not use the user settings then go for it - it will make it a lot simpler! - IMO if a user has loaded a gravatar they want to use it - if they use the same e-mail in moodle - they want to use their gravatar. if they don't they either upload a new image in moodle or disable their gravatar account with that e-mail. I can't see any privacy issues as it's all user controllable anyway.

          not sure about the fallbacks though.

          Show
          Dan Marsden added a comment - user by user setting was added as per Petr's request/direction - didn't make sense to me either but our chances of getting this reviewed for inclusion were higher if we did it... in the end MD made a call about the user settings too (see comments above for details) - if you can convince MD to not use the user settings then go for it - it will make it a lot simpler! - IMO if a user has loaded a gravatar they want to use it - if they use the same e-mail in moodle - they want to use their gravatar. if they don't they either upload a new image in moodle or disable their gravatar account with that e-mail. I can't see any privacy issues as it's all user controllable anyway. not sure about the fallbacks though.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yeah, I read the whole issue before commenting, hence I asked for illustration, because cannot get the reason for the "user by user" one when the global achieves exactly that (it's in user hands to upload image / configure gravatar).

          Show
          Eloy Lafuente (stronk7) added a comment - Yeah, I read the whole issue before commenting, hence I asked for illustration, because cannot get the reason for the "user by user" one when the global achieves exactly that (it's in user hands to upload image / configure gravatar).
          Hide
          Dan Marsden added a comment -

          yep - completely agree - perhaps Petr can comment?

          Show
          Dan Marsden added a comment - yep - completely agree - perhaps Petr can comment?
          Hide
          Petr Škoda added a comment -

          hmm, I have to agree now I think it was a privacy overkill too, uploaded->gravatar->default should be hopefully fine as long as we explain to users how to get rid of their private gravatar on school moodle sites - simply upload random abstract image in moodle. Admins should understand that they can not prevent avatar image change via moodle capabilities too.

          (hmmm, I should upload my favorite frankenstyle avatar to gravatar.com)

          Show
          Petr Škoda added a comment - hmm, I have to agree now I think it was a privacy overkill too, uploaded->gravatar->default should be hopefully fine as long as we explain to users how to get rid of their private gravatar on school moodle sites - simply upload random abstract image in moodle. Admins should understand that they can not prevent avatar image change via moodle capabilities too. (hmmm, I should upload my favorite frankenstyle avatar to gravatar.com)
          Hide
          Dan Marsden added a comment -

          great - so I guess this should be rejected so that someone can simplify the patch?

          Show
          Dan Marsden added a comment - great - so I guess this should be rejected so that someone can simplify the patch?
          Hide
          David Mudrak added a comment -

          My +1 for the single uploaded->gravatar->default fallback path. That is how everybody out there is using gravatar services. Thanks everybody who have maintained the patch for such a long time!

          Show
          David Mudrak added a comment - My +1 for the single uploaded->gravatar->default fallback path. That is how everybody out there is using gravatar services. Thanks everybody who have maintained the patch for such a long time!
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          It made me chuckle reading through all of the comments here.
          Too be truthful while working with the brilliant patch created by Jonathan, Jonathan and Dan I did get the feeling the new setting was a little overkill (and unneeded confusion) as well however noting the discussion Dan had with Martin I was quite happy to continue on.

          I've now created a patch that implements only the admin setting, does away with the defines and complexity and pretty much just implements vanilla gravatar support.
          With this patch enabled the path is uploaded > gravatar > default.

          My +! goes to this new patch that doesn't use the user setting. We can always add more complexity later on if users are finding it too easy

          Git repo: git://github.com/samhemelryk/moodle.git
          Branch: wip-MDL-21676-master-revised-again
          Diff URL: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-21676-master-revised-again
          Fetch cmd: git fetch git://github.com/samhemelryk/moodle.git wip-MDL-master-revised-again

          Eloy I have now updated my github master branch... sorry must've missed updating this week in all the rush! I will endeavour to remember that this week so that you can give your finger a rest

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, It made me chuckle reading through all of the comments here. Too be truthful while working with the brilliant patch created by Jonathan, Jonathan and Dan I did get the feeling the new setting was a little overkill (and unneeded confusion) as well however noting the discussion Dan had with Martin I was quite happy to continue on. I've now created a patch that implements only the admin setting, does away with the defines and complexity and pretty much just implements vanilla gravatar support. With this patch enabled the path is uploaded > gravatar > default . My +! goes to this new patch that doesn't use the user setting. We can always add more complexity later on if users are finding it too easy Git repo: git://github.com/samhemelryk/moodle.git Branch: wip- MDL-21676 -master-revised-again Diff URL: https://github.com/samhemelryk/moodle/compare/master...wip-MDL-21676-master-revised-again Fetch cmd: git fetch git://github.com/samhemelryk/moodle.git wip-MDL-master-revised-again Eloy I have now updated my github master branch... sorry must've missed updating this week in all the rush! I will endeavour to remember that this week so that you can give your finger a rest Cheers Sam
          Hide
          Jonathan Harker added a comment -

          I'm just in the process of re-writing this patch. Will put up a git URL later this afternoon

          Show
          Jonathan Harker added a comment - I'm just in the process of re-writing this patch. Will put up a git URL later this afternoon
          Hide
          Jonathan Harker added a comment -

          Could it be as simple as this?!

          Show
          Jonathan Harker added a comment - Could it be as simple as this?!
          Hide
          Jonathan Harker added a comment -

          Sorry - cross-post. Sam's master-revised-again looks good

          Show
          Jonathan Harker added a comment - Sorry - cross-post. Sam's master-revised-again looks good
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Two little comments, about both edit_form and editadvanced_form

          1) the 'gravatar' element is shown as 'static' on the left (spreading in an horrible multiline). Surely it would be better to show it like sort of "heading" in the fieldset.
          2) The $hasuploadedpicture... is it calculated ok? Seems to be doing exactly the opposite than expected. I'd expect it being set to true if ANY of the files exist.

          In other words, how does https://github.com/stronk7/moodle/commit/5568ce92a8cdefa6d701539a42ed388d03a4fd9a look?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Two little comments, about both edit_form and editadvanced_form 1) the 'gravatar' element is shown as 'static' on the left (spreading in an horrible multiline). Surely it would be better to show it like sort of "heading" in the fieldset. 2) The $hasuploadedpicture... is it calculated ok? Seems to be doing exactly the opposite than expected. I'd expect it being set to true if ANY of the files exist. In other words, how does https://github.com/stronk7/moodle/commit/5568ce92a8cdefa6d701539a42ed388d03a4fd9a look? Ciao
          Hide
          Sam Hemelryk added a comment -

          Hey dude,
          Good catch! Your changes look spot on thanks Eloy.
          +1 for the combo.

          Cheers Sam

          Show
          Sam Hemelryk added a comment - Hey dude, Good catch! Your changes look spot on thanks Eloy. +1 for the combo. Cheers Sam
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Finally integrated, yay! Let's test it!

          Show
          Eloy Lafuente (stronk7) added a comment - Finally integrated, yay! Let's test it!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Note: Please amend the testing instructions.

          Show
          Eloy Lafuente (stronk7) added a comment - Note: Please amend the testing instructions.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Forget my last comment suggesting revamped instructions, I've been using this, uploading files, changing email and enabling/disabling the gravatar setting and everything seems to be working reasonably ok.

          So passing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Forget my last comment suggesting revamped instructions, I've been using this, uploading files, changing email and enabling/disabling the gravatar setting and everything seems to be working reasonably ok. So passing, thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this code has been spread to all Moodle git and cvs repositories. Many thanks! Closing. Ciao
          Hide
          Martin Dougiamas added a comment -

          Congrats all for sticking with this one - what an odyssey!

          Show
          Martin Dougiamas added a comment - Congrats all for sticking with this one - what an odyssey!

            People

            • Votes:
              9 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: