Skip to content

Add ability to set email confirmation state to false - #2997

Closed
davwheat wants to merge 7 commits into
masterfrom
dw/add-deactivate-user-logic
Closed

davwheat wants to merge 7 commits into
masterfrom
dw/add-deactivate-user-logic

Conversation

@davwheat

Copy link
Copy Markdown
Contributor

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
7 of 9 tasks
@davwheat
davwheat requested review from SychO9 and luceos August 1, 2021 12:32
@luceos

luceos commented Aug 6, 2021

Copy link
Copy Markdown
Contributor

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

davwheat commented Aug 9, 2021

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor

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

luceos commented Sep 3, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor 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
Copy Markdown
Contributor

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.

@askvortsov1 askvortsov1 left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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.

4 participants