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

        1. MDL-41115_emaillogin.patch
          5 kB
          Petr Skoda
        2. patch.diff
          1 kB
          Dmitry Matora

          Issue Links

            Activity

            Hide
            Dmitry Matora added a comment -

            here is a patch

            Show
            Dmitry Matora added a comment - here is a patch
            Hide
            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
            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
            Petr Skoda added a comment -

            Interesting idea, thanks for the report!

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

            I cannot work on this, sorry.

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

            Submitting for peer review...

            Show
            Petr Skoda added a comment - Submitting for peer review...
            Hide
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            Dan Poltawski added a comment -

            hehe, of course

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

            Hi Petr,

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

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

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

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

            Lol.

            Show
            Mark Nelson added a comment - Lol.
            Hide
            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
            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
            Petr Skoda added a comment -

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

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

            No comment usually means good comment

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

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

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

            resubmitting with unit tests, thanks...

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

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

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            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 Glancy added a comment - Thanks Petr, integrated in master. There might be duplicates in tracker, I found at least this one MDL-20143
            Hide
            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
            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 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 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
            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
            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
            Mary Cooch added a comment -

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

            Show
            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: