Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4][com_actionlog] - log event reset password #39435

Merged
merged 11 commits into from
Mar 7, 2023
Merged

Conversation

alikon
Copy link
Contributor

@alikon alikon commented Dec 16, 2022

Pull Request for Issue #39062.

Summary of Changes

generate proper event

Testing Instructions

  • Set up sending mail from the site,
  • Create a test user with a valid email address,
  • Use the item "Forgot your passwor" in the authorization form in the frontend for a test user,
  • Enter your login / confirmation code or follow the link in the letter received by mail,
  • Change the user's password to a new one.

Actual result BEFORE applying this Pull Request

see #39435

Expected result AFTER applying this Pull Request

the events are correctly logged in Action Logged

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@N6REJ
Copy link
Contributor

N6REJ commented Dec 16, 2022

please fill out PR info properly.

@alikon alikon marked this pull request as ready for review December 16, 2022 15:55
@alikon alikon changed the title Update Joomla.php [4][com_actionlog] - log event reset password Dec 16, 2022
@sandewt
Copy link
Contributor

sandewt commented Dec 16, 2022

Pull Request for Issue #39435 .
see #39435

Should be #39062

@SharkyKZ
Copy link
Contributor

Surely, adding new events is not necessary to fix the reported issue?

@Kostelano
Copy link
Contributor

I have tested this item ✅ successfully on fa80696

Thanks, now it works as it should.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39435.

@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on fa80696


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39435.

@richard67
Copy link
Member

Hmm there is a comment with a valid concern: #39435 (comment) . We should not add new events when it‘s not needed.

@richard67 richard67 added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Dec 17, 2022
@alikon
Copy link
Contributor Author

alikon commented Dec 19, 2022

What's the scope of the Events then ?

@richard67
Copy link
Member

What's the scope of the Events then ?

@alikon Have you checked if there is an existing event which can be used? If you say yes and there was none, I am ok with it.

@alikon
Copy link
Contributor Author

alikon commented Dec 19, 2022

in the previous version 3 it was used onUserAfterSave but 1st is not logical 2nd no separation of concerns, and 3rd not readable code

@rdeutz
Copy link
Contributor

rdeutz commented Dec 19, 2022

I think it is fine to create new events for thinks like this, but it should be consisted overt the whole process

@Kostelano
Copy link
Contributor

I have tested this item ✅ successfully on ba1d350

I re-tested after the changes - everything works. Thank you.
I hope this doesn't get stuck and gets merged.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39435.

@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on ba1d350


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39435.

@richard67
Copy link
Member

I think it is fine to create new events for thinks like this, but it should be consisted overt the whole process

@alikon If I understand @rdeutz 's comment right, he means that if we add events onUserAfterResetRequest and onUserAfterResetComplete, there should also be events onUserBeforeResetRequest and onUserBeforeResetComplete added, even if currently not used. @rdeutz Is my understanding right, and should this be done with this PR here?

@rdeutz
Copy link
Contributor

rdeutz commented Feb 23, 2023

@richard67 yes and yes.

@richard67
Copy link
Member

@alikon Could you check the previous two comments and if possible implement the requested changes? It would be good to bring this useful PR forward. Let us know if you need some help or other that or further advice. Thanks in advance.

@alikon alikon removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Feb 25, 2023
@alikon
Copy link
Contributor Author

alikon commented Feb 25, 2023

added the Before events

@Kostelano
Copy link
Contributor

I tested again. At the moment, the PR is broken, something needs to be fixed (problem after the last changes). A request to remind the login comes to the mail, a request to change the password does not. I tried several times.

@Kostelano
Copy link
Contributor

Kostelano commented Feb 25, 2023

I have tested this item ✅ successfully on afc06a4

Sorry, it works. Sending a successful test.

Why did it happen that at first it did not work: I created a user and, in addition to the "Registered" group, added him to the Superusers group. The system does NOT send a password change request for a group of Superusers (which is strange because I have seen error lines for superusers who want to change the password). In this case, the system proceeds to the next stage - entering a login / confirmation code, but nothing comes to the mail. Well, that's a bug for a separate fix.

As soon as I removed the user from the superuser group, everything began to work. Everything is displayed correctly in the action log.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39435.

@viocassel
Copy link
Contributor

I have tested this item ✅ successfully on afc06a4


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39435.

@alikon
Copy link
Contributor Author

alikon commented Mar 2, 2023

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39435.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 2, 2023
@sandewt
Copy link
Contributor

sandewt commented Mar 7, 2023

Why don't you use [ ] (brackets) instead of array() ?
See also line 1179.

@alikon Did I miss something, because as far as I know brackets are preferred in J4? You probably have a reason for it.

@roland-d roland-d merged commit 081afed into joomla:4.2-dev Mar 7, 2023
@roland-d
Copy link
Contributor

roland-d commented Mar 7, 2023

Thank you

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 7, 2023
@roland-d roland-d added this to the Joomla! 4.2.9 milestone Mar 7, 2023
@alikon alikon deleted the patch-4 branch March 7, 2023 16:53
@alikon
Copy link
Contributor Author

alikon commented Mar 7, 2023

@sandewt i'll change it to [] on a next pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.