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

[Feature] Admin approval #1232

Open
wants to merge 4 commits into
base: dev/new_features
Choose a base branch
from

Conversation

BentiGorlich
Copy link
Member

@BentiGorlich BentiGorlich commented Nov 19, 2024

  • admins can now decide to turn a mandatory approval on in the admin settings
  • a new tab will appear labelled "Applications" where new users that are not yet approved will appear
  • when the setting is enabled a new input will be displayed on the register page: "Application Text"
  • This behavior will be apparent on registering with an added flash message informing the user about it
  • Users will receive an email when they are accepted or rejected
  • when a user is not approved they cannot log in and an appropriate message will be displayed
  • the register site now redirects to the login page instead of the front page

Tasks

  • API implemented
  • Updated Docs
  • Included Images
  • Implemented mails
  • Refine email texts (I am definitely not qualified to do that
  • Refine Admin application page

Pictures

Register Page

Register page
After registration

Admin application view

Admin application view

Mails

Approved email
Rejected email

Closing #722

@BentiGorlich BentiGorlich added enhancement New feature or request backend Backend related issues and pull requests labels Nov 19, 2024
@BentiGorlich BentiGorlich added this to the v1.8.0 milestone Nov 19, 2024
@BentiGorlich BentiGorlich self-assigned this Nov 19, 2024
- admins can now decide to turn a mandatory approval on in the admin settings
- a new tab will appear labelled "Applications" where new users that are not yet approved will appear
- when the setting is enabled a new input will be displayed on the register page: "Application Text"
- This behaviour will be apparent on registering with an added flash message informing the user about it
- Users will receive an email when they are accepted or rejected
- when a user is not approved they cannot log in and an appropriate message will be displayed
- the register site now redirects to the login page instead of the front page
@melroy89
Copy link
Member

Nice!

@BentiGorlich
Copy link
Member Author

Do we have someone that is good in phrasing user facing stuff like the emails? @jwr1 or @TheVillageGuy maybe?

email_application_rejected_title: Your application was rejected
email_application_rejected_body: Your signup application was rejected by the admins
email_application_pending: Your admin needs to approve your account before you can log in.
email_verification_pending: You have to verify your email address before you can log in.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
email_verification_pending: You have to verify your email address before you can log in.
email_verification_pending: Please verify your email address before you can log in.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this change makes sense. Its just a reminder in the approval email that the user still has to verify their email before they are allowed to log in. This is only appended to the mail if they did not already verify their email

public bool $isApproved;

#[Column(type: 'boolean', nullable: false, options: ['default' => false])]
public bool $isRejected = false;
Copy link
Member

Choose a reason for hiding this comment

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

In theory you could potentially end up with isApproved and isRejected set to true, since there is no guard against this incorrect state.

Not sure how to prevent that.

Copy link
Member Author

Choose a reason for hiding this comment

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

In theory yes. It is always a pain to change migrations after they have already been applied. But I could change this to an enum and then it would not be a possible state. However I think as there are not a lot of interaction points with this value and all of them set both values at the same time, I don't think that it will happen

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes of course, this needs to be an enum type. This will also allows you to introduce other states like "waiting" (waiting for approval) safely.

This is the way.

@melroy89
Copy link
Member

melroy89 commented Nov 20, 2024

image

The "One last step" in the message now contradict each other 😆 . Yes you still need to use the activation link, but if admin approval is required, this isn't the last step.. you than still need to wait.

@BentiGorlich
Copy link
Member Author

The "One last step" in the message now contradict each other 😆 . Yes you still need to use the activation link, but if admin approval is required, this isn't the last step.. you than still need to wait.

Yeah I know 😅, but I didn't know how to solve this. I didn't really want to mess up the existing translations... But if you think that it is necessary I will split the existing one up, so that both messages can be merged

@BentiGorlich BentiGorlich linked an issue Nov 22, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Backend related issues and pull requests enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add option for new accounts to require admin approval
3 participants