Comments

• 

  Anthony Borrow

  Wed, 28 Aug 2013, 10:00 AM

  Many thanks for sharing this plugin with the Moodle community. In order to properly review this plugin we need to know for which version it is intended to be used. The current version available for download does not specify the version of Moodle.
  
  The second issue I noticed in givng the code a quick look is that it requires modification of core Moodle files which technically makes it a patch rather than a pure plugin. If it is to be shared with the Moodle community in its current form it would need to be moved out of the authentication area into the other category.
  
  It would seem to me that you might be able to use the loginpage_hook function to avoid having to patch the login_form.html file. I would suggest taking a look at the CAS authentication plugin to see how they implemented it but it looks similar to what you are trying to do. You could even verify which of the social platforms have been setup and just display the buttons for those and that would give site admins control over which of the social sites they want to grant access too. I could see where this could be helpful when there is a tighter integration with Google Apps.
  
  Also, because it deals with authentication, possible creation of accounts, etc. I would want to take some time to have the code more carefully reviewed. One minor detail I noticed as ^M at the end of some of the lines. Make sure the files are utf8 encoded to avoid those.
  
  I also encourage maintainers to provide the source control, issue tracker and documentation URLs for the Useful links section. Thanks for providing the screenshot as that helps folks see what it will look like.
  
  For now, I am going to mark this entry as needing more work until these issues are resolved. Please do not hesitate to let me know if there is anything I can do to be supportive of your efforts.
  
  Peace - Anthony
• 

  Anthony Borrow

  Thu, 14 Nov 2013, 3:18 PM

  On 10/28/2013 04:03 AM - elcentra admin has commented on a plugin submission that hasn't been approved :
  
  Hi,
  
  Based on your feedback, we have updated the application. Please check the
  comments below. I have included your comments and our replies to the same so
  that it would be easier for you to track.
  
  >>> In order to properly review this plugin we need to know for which version
  it is intended to be used. The current version available for download does not
  specify the version of Moodle.
  
  We have now specified the version of moodle that supports the plugin.
  
  >>> The second issue I noticed in givng the code a quick look is that it
  requires modification of core Moodle files which technically makes it a patch
  rather than a pure plugin. If it is to be shared with the Moodle community in
  its current form it would need to be moved out of the authentication area into
  the other category.It would seem to me that you might be able to use the
  loginpage_hook function to avoid having to patch the login_form.html file. I
  would suggest taking a look at the CAS authentication plugin to see how they
  implemented it but it looks similar to what you are trying to do. You could
  even verify which of the social platforms have been setup and just display the
  buttons for those and that would give site admins control over which of the
  social sites they want to grant access too. I could see where this could be
  helpful when there is a tighter integration with Google Apps.
  
  Basically this really did not require modification to the core files. We wanted
  the user to add a few buttons to the theme in the place of their choice. But
  since you wanted it to be more user friendly, we honour it and we have made
  better provisions.
  - The plugin will find the login form and will append the buttons on its
  own.
  - This behaviour can be disabled by appending the buttons inside a
  specific div
  - The entire appending can be disabled by which the user can add the
  buttons to locations of their own choice.
  
  
  
  >>> Also, because it deals with authentication, possible creation of accounts,
  etc. I would want to take some time to have the code more carefully reviewed.
  One minor detail I noticed as ^M at the end of some of the lines. Make sure the
  files are utf8 encoded to avoid those.
  
  We have now updated all the files in utf8. Please let us know the name of the
  file if you still find any problems with one of the files.
  
  >>> I also encourage maintainers to provide the source control, issue tracker
  and documentation URLs for the Useful links section. Thanks for providing the
  screenshot as that helps folks see what it will look like.
  
  We will comply this in sometime after the plugin is authorized and gets
  published. Till then, we will hear to the user's comments manually and will
  maintain codes clearly.
  
  We are eager to see this in Moodle plugins site soon.
• 

  Anthony Borrow

  Thu, 14 Nov 2013, 3:20 PM

  Due to the recent crash, could you please re-add the updated version of this plugin? Peace - Anthony
• 

  Anthony Borrow

  Thu, 14 Nov 2013, 3:21 PM

  On 10/31/2013 12:44 AM - Aparup Banerjee has commented on a plugin submission that hasn't been approved:
  
  Thanks for your update
  
  - regarding the supported moodle versions, you need to look at the 'download versions' tab and edit both versions (or at least one) there. you can then select moodle supported versions there.
  - btw, i saw some use of $_GET['state'] in some files which is sometimes a security concern, there is already use of optional_param() though so i leave this to you.
  - add a issue tracker or if you need , we can create on in
  http://tracker.moodle.org/browse/CONTRIB for this plugin. This is to have a proper place for users to report issues instead of here. in comments.
  - have you seen the other auth ones? there is auth_googleoauth2 and
  auth_linkedin , you could consider contributing to that too as there seems to be quiet an overlap
  - the readme still mentions code to be copied
• 

  elcentra admin

  Tue, 19 Nov 2013, 12:08 PM

  HI. Thanks for all your updates. We have added the source control and supported moodle versions. For full documentation URLs, we have linked the README.txt file contents Since we plan to do modifications to the readme.txt on future versions. THis way the users who download the updates will have the full documentation with them and the ones who want to download this as a fresh one will have access to the documentation prior to downloading. If still insisted by you, we will create a separate URL for this and add a documentation for this, but from what we see, we don't think there will be a need for separate extended documentation.
  
  Let us know your comments
• 

  elcentra admin

  Tue, 19 Nov 2013, 12:20 PM

  And regarding your concern of readme.txt asking to copy code, we have mentioned that it is not mandatory. after the users get into Administration panel. (Just in case users without a knowledge of HTML/PHP want to modify their theme and place these buttons in a different place, we have given instructions for doing so. Other than that we have made sure to our best possible extent that users dont get confused).
  
  But we have missed this update in the readme.txt file. Thanks for pointing this out . We will update this very shortly.
  
  And for users who copy this code, you can see that all it contains is 4 tags that have their src's in the same website of the user. So we aren't doing any kind of tracking stuffs
  
  Thanks again for all your assistances
• 

  elcentra admin

  Fri, 22 Nov 2013, 3:28 PM

  Also, request you to note that all the above comments are related to version 1.02 (2013112200)
• 

  Anthony Borrow

  Mon, 30 Dec 2013, 9:21 AM

  Thanks for your patience. I have installed and tested this on one of my production sites and it seems to work well. I think there are a few minor issues I would like to see tidied up before we get this approved. First, in the lang/en/auth_electra.php file the @package name is incorrect and the copyright is listed as Martin. Please correct those. Also, I like to see the boilerplate notice making the GPL license explicit. Ideally this is in all of the files but for starters it would be good if it were in the version.php file. I did receive a PHP warning when logging in for the first time with a Google account - Warning: Creating default object from empty value in /moodle/auth/elcentra/auth.php on line 230. Thanks for providing the Github source control URL. Please consider following the Moodle naming convention for repositories - namely, moodle-{type}_{name} so in this case moodle-auth_elcentra. Pointing to the README file in the repository is fine but you are also welcome to create a page in Moodle Docs. I am going to mark this as needing more work but the changes should be simple. If you have any questions just let me know. Peace - Anthony
• 

  Anthony Borrow

  Mon, 30 Dec 2013, 9:24 AM

  p.s. - I second Aparup's recommendation to avoid $GET for state - any reason not to use the optional_param there?
• 

  elcentra admin

  Tue, 21 Jan 2014, 6:33 PM

  Thanks for your reply. We are now done with all your above requirements. Rather than moodle docs, we prefer using the README file since it would be a single place to update all our changes. If you insist us to create a moodle docs page, please send us the procedure to do so.
  
  We believe we have fixed all your reports and answered your queries. I am interested & eager to see this plugin go live. Please feel to let us know in case we have missed any.
  
  Thanks
• 

  Aparup Banerjee

  Mon, 24 Feb 2014, 12:41 PM

  Hi,
  Thanks for addressing so many issues.
  
  ps: https://moodle.org/plugins/view.php?plugin=auth_googleoauth2 is very similar (twitter support seems to be the difference). It seems technically feasible that these are merged, so please have a look and consider.