Details

    • Testing Instructions:
      Hide

      1/ run phpunit tests
      2/ try to login using both username and email, make sure email fails
      3/ enable new setting authloginviaemail
      4/ try to login using both username and email, make sure both methods work and the label for username on login screen is changed.

      Show
      1/ run phpunit tests 2/ try to login using both username and email, make sure email fails 3/ enable new setting authloginviaemail 4/ try to login using both username and email, make sure both methods work and the label for username on login screen is changed.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w12_MDL-41115_m27_emaillogin

      Description

      I really miss the feature an i have it implemented. Trying to submit a patch

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              dmatora Dmitry Matora added a comment -

              here is a patch

              Show
              dmatora Dmitry Matora added a comment - here is a patch
              Hide
              dmatora Dmitry Matora added a comment -

              I don't see how this is more convenient than using github.
              And i don't see how else i was supposed to submit the fix (i've spent an hour exploring site just to see that github suggests to use tracker.moodle.org and docs suggest github for non core modifications)
              I hope you can find a way to use it

              Show
              dmatora Dmitry Matora added a comment - I don't see how this is more convenient than using github. And i don't see how else i was supposed to submit the fix (i've spent an hour exploring site just to see that github suggests to use tracker.moodle.org and docs suggest github for non core modifications) I hope you can find a way to use it
              Hide
              skodak Petr Skoda added a comment -

              Interesting idea, thanks for the report!

              Show
              skodak Petr Skoda added a comment - Interesting idea, thanks for the report!
              Hide
              skodak Petr Skoda added a comment -

              I cannot work on this, sorry.

              Show
              skodak Petr Skoda added a comment - I cannot work on this, sorry.
              Hide
              skodak Petr Skoda added a comment -

              Submitting for peer review...

              Show
              skodak Petr Skoda added a comment - Submitting for peer review...
              Hide
              markn Mark Nelson added a comment -

              Hi Petr,

              Works as expected.

              Only thing is you are missing spaces around '=>'.

              Feel free to submit to integration when done.

              Show
              markn Mark Nelson added a comment - Hi Petr, Works as expected. Only thing is you are missing spaces around '=>'. Feel free to submit to integration when done.
              Hide
              skodak Petr Skoda added a comment -

              Thanks, submitting. I am not doing the spaces around "=>" unless it is inside foreach - our coding style does not cover this...

              Show
              skodak Petr Skoda added a comment - Thanks, submitting. I am not doing the spaces around "=>" unless it is inside foreach - our coding style does not cover this...
              Hide
              markn Mark Nelson added a comment -

              Also, the change from "$DB->set_field('user', 'auth', $auth, array('username' => $username));" to "$DB->set_field('user', 'auth', $auth, array('id' => $user->id));" seems unnecessary.

              Show
              markn Mark Nelson added a comment - Also, the change from "$DB->set_field('user', 'auth', $auth, array('username' => $username));" to "$DB->set_field('user', 'auth', $auth, array('id' => $user->id));" seems unnecessary.
              Hide
              skodak Petr Skoda added a comment - - edited

              no, it was actually a bug because it might affect mnet accounts, the id guarantees we modify the correct account, there are also potential problems with mixed-case usernames

              Show
              skodak Petr Skoda added a comment - - edited no, it was actually a bug because it might affect mnet accounts, the id guarantees we modify the correct account, there are also potential problems with mixed-case usernames
              Hide
              poltawski Dan Poltawski added a comment -

              Hmm - i'm a bit hesitant that this should be a global setting, wouldn't it be better implemented in each auth plugin?

              Show
              poltawski Dan Poltawski added a comment - Hmm - i'm a bit hesitant that this should be a global setting, wouldn't it be better implemented in each auth plugin?
              Hide
              skodak Petr Skoda added a comment -

              no, you need to do it in core to find out which auth plugin to use for existing account...

              Show
              skodak Petr Skoda added a comment - no, you need to do it in core to find out which auth plugin to use for existing account...
              Hide
              poltawski Dan Poltawski added a comment -

              hehe, of course

              Show
              poltawski Dan Poltawski added a comment - hehe, of course
              Hide
              poltawski Dan Poltawski added a comment -

              Hi Petr,

              I think that we could have unit tests for this change, what do you think?

              Show
              poltawski Dan Poltawski added a comment - Hi Petr, I think that we could have unit tests for this change, what do you think?
              Hide
              skodak Petr Skoda added a comment -

              I could or I also might have worked on something else.

              Show
              skodak Petr Skoda added a comment - I could or I also might have worked on something else.
              Hide
              markn Mark Nelson added a comment -

              Lol.

              Show
              markn Mark Nelson added a comment - Lol.
              Hide
              poltawski Dan Poltawski added a comment -

              Well, I think its worth the effort.

              Since you've not expressed any reason why this could not be done easily, i'm reopening.

              Show
              poltawski Dan Poltawski added a comment - Well, I think its worth the effort. Since you've not expressed any reason why this could not be done easily, i'm reopening.
              Hide
              skodak Petr Skoda added a comment -

              Well, because you did not tell me what you think about the actual patch I give up, ciao.

              Show
              skodak Petr Skoda added a comment - Well, because you did not tell me what you think about the actual patch I give up, ciao.
              Hide
              poltawski Dan Poltawski added a comment -

              No comment usually means good comment

              Show
              poltawski Dan Poltawski added a comment - No comment usually means good comment
              Hide
              cibot CiBoT added a comment -

              Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              skodak Petr Skoda added a comment -

              resubmitting with unit tests, thanks...

              Show
              skodak Petr Skoda added a comment - resubmitting with unit tests, thanks...
              Hide
              stronk7 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
              stronk7 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
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              marina Marina Glancy added a comment -

              Thanks Petr, integrated in master.
              There might be duplicates in tracker, I found at least this one MDL-20143

              Show
              marina Marina Glancy added a comment - Thanks Petr, integrated in master. There might be duplicates in tracker, I found at least this one MDL-20143
              Hide
              lameze Simey Lameze added a comment -

              This new feature is working properly, I logged using my username and email successfully.

              In PHPUnit test only a small error:
              1) core_filelib_testcase::test_curl_redirects
              Failed asserting that two strings are identical.

              But didn't affected the test.

              Show
              lameze Simey Lameze added a comment - This new feature is working properly, I logged using my username and email successfully. In PHPUnit test only a small error: 1) core_filelib_testcase::test_curl_redirects Failed asserting that two strings are identical. But didn't affected the test.
              Hide
              marina Marina Glancy added a comment -

              Thanks for your awesome code, it is now part of Moodle. Don't forget to submit another issue next week (and peer review two).

              Show
              marina Marina Glancy added a comment - Thanks for your awesome code, it is now part of Moodle. Don't forget to submit another issue next week (and peer review two).
              Hide
              marycooch Mary Cooch added a comment -

              Removing qa_test required label as there is a test for this MDLQA-6682 on the master copy ready for 2.7 QA testing.

              Show
              marycooch Mary Cooch added a comment - Removing qa_test required label as there is a test for this MDLQA-6682 on the master copy ready for 2.7 QA testing.
              Hide
              marycooch Mary Cooch added a comment -

              Removing docs_required label as this is documented here http://docs.moodle.org/27/en/Managing_authentication

              Show
              marycooch Mary Cooch added a comment - Removing docs_required label as this is documented here http://docs.moodle.org/27/en/Managing_authentication

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    12/May/14