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

Make sure we understand how to configure who receives notifications about new users to approve #2758

Closed
marc-farre opened this issue Sep 18, 2017 · 28 comments

Comments

@marc-farre
Copy link
Collaborator

marc-farre commented Sep 18, 2017

What steps will reproduce the problem?

Administration -> Users -> Settings ->

  • Require group admin approval after registration checked
  • Anonymous users can register checked

What is the expected result?

When an anonymous user register, admin should receive a notification (email and desktop) telling him he has to approve this new user.

What do you get instead?

No notification. Admin has got to manually reload the "pending approvals" (/admin/approval) page every day !!

Additional info

This subject is also discussed on the platform here and here

Q A
HumHub version 1.22
PHP version 7.2
Operating system Ubuntu server 16.04
@marc-farre
Copy link
Collaborator Author

Where, in the code, could we add theses notifications, until this problem is resolved ? Thanks !

@buddh4
Copy link
Contributor

buddh4 commented Sep 18, 2017

I think this would be the place to send an approval notification to all admins (or group manager).

@marc-farre
Copy link
Collaborator Author

Great ! Thanks a lot !

@jayjayel
Copy link

jayjayel commented Jan 4, 2019

Has this been implemented? I’m a new Humhub admin, and am wondering the same.

@luke- luke- changed the title No notification for pending approvals Notification for pending approvals (registration) Mar 25, 2020
@marc-farre
Copy link
Collaborator Author

Any plan to have this notification in a future version ?

@luke-
Copy link
Contributor

luke- commented Mar 27, 2020

This is planned for the long term, but is currently not on our roadmap.

@buddh4 buddh4 added this to Candidates in Planned developments via automation Apr 14, 2020
@gerhardbeck
Copy link

gerhardbeck commented Oct 4, 2020

I think this would be the place to send an approval notification to all admins (or group manager).

Sorry, I don't understand it: What to do with the registrationcontroller.php in order to get a notification via email?
Can you offer some details for a newbie, please?

@marc-farre marc-farre changed the title Notification for pending approvals (registration) Make sure we understand how to configure who receives notifications about new users to approve Mar 29, 2024
@marc-farre
Copy link
Collaborator Author

I've changed the title because now notifications are via Group::notifyAdminsForUserApproval().

But it's hardly impossible to understand that notifications are sent to the managers of the group where the user registered... See https://community.humhub.com/comment/perma?id=48457

There should be at least a hint on this field explaining what manager are for:
image

But maybe a better idea would be to send notifications to all users allowed to approve new registered users.

@ArchBlood
Copy link
Contributor

I've changed the title because now notifications are via Group::notifyAdminsForUserApproval().

But it's hardly impossible to understand that notifications are sent to the managers of the group where the user registered... See https://community.humhub.com/comment/perma?id=48457

There should be at least a hint on this field explaining what manager are for: image

But maybe a better idea would be to send notifications to all users allowed to approve new registered users.

I agree with this, we should modify this so that an admin or someone with the permissions get the notification(s), because currently this function doesn't work unless you're a group manager, so in my eyes it's just unused code;

Suggestion

  • Move notifier to own notification class in admin core and call from notifyAdminsForUserApproval().
  • Set default permissions for admins to always get notified.
  • Disable user editable notification and remove it.

@marc-farre
Copy link
Collaborator Author

@luke- @ArchBlood what do you think about this PR #6907 ?

Discussion: https://community.humhub.com/content/perma?id=276481

Before:

image

image

After:

image

image

@Gilbertdelyon
Copy link

May I add my 2 cents?
In Users settings form
image
In Group settings form
image
Doesn't matter if we use "group admin" or "Manager", but the same word should be used in both forms

@luke-
Copy link
Contributor

luke- commented Apr 1, 2024

@marc-farre Thanks for the PR. I am less in favor of renaming the "Manager" field as it is partly used for other features as well.

But we could just customize the attribute hint and add more information about the manager role in the approval context.

Would that be enough? If yes, we could ask @Semir1212 or @mbumpalumpa for help regarding the best wording.

@marc-farre
Copy link
Collaborator Author

@luke- as the "Manager" field is only displayed when "Require group admin approval after registration" is enabled, I fought there where no other usage of it.

@luke-
Copy link
Contributor

luke- commented Apr 1, 2024

@marc-farre Good point, but I would rather stop hiding the field. Some custom modules use it and the user administration for group managers also works without approval.

@Gilbertdelyon
Copy link

@marc-farre Good point, but I would rather stop hiding the field. Some custom modules use it and the user administration for group managers also works without approval.

As site admin I agree with this, because as long as the field is hiddeen in group setting form you may not understand what "Require group admin approval after registration" is refering to. This is what appened to me.

@marc-farre
Copy link
Collaborator Author

marc-farre commented Apr 1, 2024

@luke- commit c348067

Always displayed and no attribute hint (I still think something is missing here to understand what this field is for):
image

Changed attribute hint:
image

@luke-
Copy link
Contributor

luke- commented Apr 1, 2024

@marc-farre That's right. We can still add an attribute hint to "Managers". However, I would like to keep the term "Managers".

@Semir1212 @mbumpalumpa Can you please take a look into this?

@marc-farre
Copy link
Collaborator Author

Doesn't matter if we use "group admin" or "Manager", but the same word should be used in both forms

@Gilbertdelyon Sorry, I missed your suggestion. Done in commit 940877e

@mbumpalumpa
Copy link

mbumpalumpa commented Apr 1, 2024

@luke- @marc-farre hopefully, I understood this well, so here is my thought: I would simplify it and make it a bit clearer.

I would also opt for "Manager" as HH is multilingual, and longer sentences will result in longer translations, which may be okay for the desktop version, but in the mobile version, it will result in multiple lines. For example EN (29 chars) "Registration approval manager" is in HR (36 chars) "Menadžer za odobravanje registracija".


$base_url/admin/authentication
Current: Require group admin approval after registration
Suggestion 1: Require group admin or group manager approval after registration.
Suggestion 2: If enabled, the group admin or group manager will need to approve registration.

While I understand that mentioning "group" twice in one sentence might seem redundant, I do believe we are better off with it, just to be extra clear.


$base_url/admin/group/edit
Current: Manager
Suggestion 1: Manager(s)
Suggestion 2: Group Manager(s)
I would add "(s)" just to be consistent with the field above, "Default Space(s)", and to indicate to the user that they can add multiple managers. Also, the placeholder in "Default Space(s)" is "Add Space", and currently in "Manager" it is "Select user...", so I would also unify that to "Add User".

Snimka zaslona 2024-04-01 132149

Help Suggestion 1: Select a group manager who can approve pending registrations.
Help Suggestion 2: The group manager can approve pending registrations.

I wouldn't mention that they will get a notification because if you think about it, it's kind of logical and expected to receive that type of notification. So that part seems, at least to me, like unnecessary clutter.

Anyway, let me know if I'm completely off track.

@Semir1212
Copy link
Contributor

@mbumpalumpa Let's just go with group manger, leave "group admin" out.

As for the suggestions, let's go with option 2 in all three cases.

Just make the last suggestion clearer. The Group Manager can approve pending registrations of his group, not all pending registrations.

@mbumpalumpa
Copy link

@Semir1212 hopefully I understood it correctly, here is the table of the "final" copy.

URL TYPE CURRENT NEW
/admin/authentication Description Require group admin approval after registration If enabled, the Group Manager will need to approve registration.
/admin/group/edit Title Manager Group Manager(s)
/admin/group/edit Help n/a The Group Manager can approve pending registrations of this group.
/admin/group/edit Placeholder Select user... Add user

@Semir1212
Copy link
Contributor

Fine with me @marc-farre @luke- @mbumpalumpa

@Gilbertdelyon
Copy link

Gilbertdelyon commented Apr 3, 2024

Sorry, something is not clear for me.
In present situation (v1.15.4) I see 2 different issues:

  1. Wording of fieds labels, etc is confusing. I understand that this will improved so that it will now be as clear as possible. Good point, thank you!
  2. If site admin wants to be notified by email when there is a new "pending approval" it is not obvious for him (it took me a while to find this out, even with help of mentors) that:
  • he has to be one of the default groupe managers
  • "receive notification for new group member" checkbox must be selected.

It is not clear for me if this second point will be improved?

@marc-farre
Copy link
Collaborator Author

@luke- @Semir1212 @mbumpalumpa Done in commit f3862d7

@mbumpalumpa the first line of your table was unclear (description type), but what do you think about this?
image

For group, it's OK:
image

@luke- I've changed the Select user... to Add user in the User Picker (so it will apply to all HumHub):
f3862d7#diff-a3062c7bf604a4bb7718159c46f3719c14663c1cb47d08abdde8170eddbc2b08R92
Is it fine?

@mbumpalumpa
Copy link

mbumpalumpa commented Apr 4, 2024

@marc-farre Sorry, yes, I can see how that can be confusing. Personally, I would name the checkbox "If enabled*, the Group Manager will need to approve registration" rather than adding it as a description. But it is up to @Semir1212 and @luke- to decide.

*Or in this case, as it's a checkbox, maybe "If checked, the Group..."

@luke-
Copy link
Contributor

luke- commented Apr 4, 2024

For me it's fine.

@Semir1212 ? :-)

@Semir1212
Copy link
Contributor

Fine with me @luke- @marc-farre

github-merge-queue bot pushed a commit that referenced this issue Apr 4, 2024
#6907)

* Enh #2758: Make sure we understand how to configure who receives notifications about new users to approve

* Change attribute label and hints and remove hidding the Managers field

* Replace "Require group admin" with "Require group manager"

* #2758 (comment)

---------

Co-authored-by: Lucas Bartholemy <luke-@users.noreply.github.com>
@luke- luke- closed this as completed Apr 4, 2024
Planned developments automation moved this from Candidates to Moved to development Apr 4, 2024
@marc-farre
Copy link
Collaborator Author

@Gilbertdelyon thanks for your latest suggestions.
I think this PR already allows a better understanding, even if it's maybe not perfect (but we also need to keep texts shorts and simple).
Let's wait for HumHub 1.16 release and if you think it's still confusing, you can create a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Planned developments
Moved to development
Development

No branches or pull requests

9 participants