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

Add ability to set email confirmation state to false #2997

Closed
wants to merge 7 commits into from

Conversation

davwheat
Copy link
Member

Changes proposed in this pull request:

Allows admins to set email confirmation state to false.

Reviewers should focus on:

Should we instead add a deactivate method to User.php and fire a deactivation event, similar to what we do for activation?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).

@davwheat davwheat force-pushed the dw/add-deactivate-user-logic branch from e814839 to e874010 Compare July 31, 2021 23:39
@davwheat davwheat marked this pull request as ready for review July 31, 2021 23:42
@davwheat davwheat mentioned this pull request Jul 31, 2021
9 tasks
@davwheat davwheat requested review from SychO9 and luceos August 1, 2021 12:32
@luceos
Copy link
Member

luceos commented Aug 6, 2021

I see that User::activate fires an event, would it make sense to have a similar event (and perhaps method on User) for deactivation?

Other than that this PR looks fine.

@davwheat
Copy link
Member Author

davwheat commented Aug 9, 2021

Implemented both. We now have User::deactivate and a Deactivated event.

@davwheat davwheat added this to the 1.1 milestone Aug 9, 2021
@davwheat davwheat self-assigned this Aug 9, 2021
@askvortsov1
Copy link
Member

I am not convinced of the use case for this. As mentioned over chat, I would be in favor of a general activation / deactivation mechanism where email confirmation is one factor, but until then, what's the use case for marking an email as not confirmed?

@luceos
Copy link
Member

luceos commented Sep 3, 2021

I was reading through the changes and got confused by the fact that activation and deactivation relies on a column about email verification. To that extend I agree, for now we might keep this logic and extend or modify it later? Imo this logic almost warrants it's own database column activated_at?

@davwheat
Copy link
Member Author

I think we should add this for now, and then change the activation logic later.

@askvortsov1 askvortsov1 modified the milestones: 1.1, 1.2 Sep 28, 2021
@askvortsov1
Copy link
Member

Very well, I think we can do this, but could we rename the methods to confirmEmail and unconfirmEmail (and deprecate activate)? Then the API would be consistent with the behavior we want to work towards in the future.

Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking over this again, I would be happy to approve this if we changed the method names as described above.

@davwheat davwheat force-pushed the dw/add-deactivate-user-logic branch from af66aec to c9d399b Compare December 20, 2021 15:49
@askvortsov1
Copy link
Member

Hello, thank you for your pull request! In order to speed up future development, keep all work in one place, and take advantage of CI tools that can help us avoid breaking changes, we have moved all Flarum code to a single monorepo at flarum/framework. You can read more about this process in our dev diary. Unfortunately, pull requests can't be carried over to the monorepo, so we have to close all open pull requests. If this pull request is still relevant, please feel free to reopen it over on the monorepo. We apologize for the inconvenience, and hope you will consider contributing to Flarum again in the future.

@SychO9 SychO9 deleted the dw/add-deactivate-user-logic branch January 5, 2023 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants