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

New registration notification is not marked as read #1408

Closed
melroy89 opened this issue Jan 31, 2025 · 11 comments · Fixed by #1437
Closed

New registration notification is not marked as read #1408

melroy89 opened this issue Jan 31, 2025 · 11 comments · Fixed by #1437
Assignees
Labels
bug Something isn't working
Milestone

Comments

@melroy89
Copy link
Member

melroy89 commented Jan 31, 2025

Describe the bug

When a new user is register, a new notification will now pop-up for admins to approve or reject.

On which Mbin instance did you find the bug?
kbin.melroy.org

Which Mbin version was running on the instance?
main

To Reproduce
Steps to reproduce the behavior:

  1. Go to Notifications
  2. See the registration notification

Expected behavior

A link to the approval registration page. (incl the correct page number), which is required when you the "New users have to be approved by an admin before they can log in." feature is checked.

The username link will never work when admin approval is required. Only show a link to the user if the admin approval feature is disabled.

Screenshots

Image

@melroy89 melroy89 added the bug Something isn't working label Jan 31, 2025
@melroy89 melroy89 added this to the v1.8.0 milestone Jan 31, 2025
@BentiGorlich
Copy link
Member

Again this is only true when approvals are enabled in the admin settings. When approvals are not needed the link is working fine :D

@melroy89
Copy link
Member Author

I edited my desc.

@melroy89
Copy link
Member Author

melroy89 commented Feb 3, 2025

Fixed in #1411? Right?

@melroy89 melroy89 closed this as completed Feb 3, 2025
@melroy89
Copy link
Member Author

melroy89 commented Feb 3, 2025

Well except for "Only show a link to the user if the admin approval feature is disabled."

@BentiGorlich
Copy link
Member

Hmm... Do you think we should remove the user_inline component? I mean maybe we could change the page it links to to the same signup request?

@melroy89
Copy link
Member Author

melroy89 commented Feb 3, 2025

Yes you can either disable the link (remove the link part) I guess. Or also point to the approval page.
Only when admin approval is enabled.

@melroy89
Copy link
Member Author

melroy89 commented Feb 5, 2025

@BentiGorlich Can we also "mark as read" the notification when clicking on the 'Open signup request' link?

@melroy89 melroy89 reopened this Feb 6, 2025
@melroy89
Copy link
Member Author

melroy89 commented Feb 6, 2025

Notification doesn't get marked as read after clicking on the Open signup request link. Hence I reopen the ticket.

@melroy89 melroy89 changed the title New registration notification link is missing/not working New registration notification is not marked as read Feb 6, 2025
@melroy89
Copy link
Member Author

melroy89 commented Feb 6, 2025

I suspect an issue here somewhere:

$this->notificationRepository->markUserSignupNotificationsAsRead($loggedInUser, $user);

I think it's not sufficient to have this check only in the UserFrontController. Also the $this->getUser(); was already called on line 46:

$requestedByUser = $this->getUser();

I would like to at least add markUserSignupNotificationsAsRead to the AdminSignupRequestsController.php file.

@BentiGorlich
Copy link
Member

Can we also "mark as read" the notification when clicking on the 'Open signup request' link?

I actually thought that I did that...

@melroy89
Copy link
Member Author

melroy89 commented Feb 6, 2025

Can we also "mark as read" the notification when clicking on the 'Open signup request' link?

I actually thought that I did that...

Well.. not really, there is only this mark as read at another location:

$this->notificationRepository->markUserSignupNotificationsAsRead($loggedInUser, $user);

However, this PR will fix all this: #1437

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants