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

Accessing user notification settings from the site throws a fatal error #8592

Closed
Vitaliy-1 opened this issue Jan 31, 2023 · 5 comments
Closed
Assignees
Labels
Bug:1:Low A bug that does not have a severe consequence or affects a small number of users.

Comments

@Vitaliy-1
Copy link
Collaborator

Describe the bug
When 2 or more journals are installed, site settings page becomes accessible to the admin user. That means that the admin user can edit own user profile from the site, without context being available. This leads to a fatal error when trying to view notification settings as they are bound to the context

PHP Fatal error:  Uncaught TypeError: APP\notification\form\NotificationSettingsForm::getNotificationSettingCategories(): Argument #1 ($context) must be of type PKP\context\Context, null given, called in /Users/vitalii/Documents/development/ojs-main/ojs/lib/pkp/classes/notification/form/PKPNotificationSettingsForm.php on line 131 and defined in /Users/vitalii/Documents/development/ojs-main/ojs/classes/notification/form/NotificationSettingsForm.php:27
Stack trace:
#0 /Users/vitalii/Documents/development/ojs-main/ojs/lib/pkp/classes/notification/form/PKPNotificationSettingsForm.php(131): APP\notification\form\NotificationSettingsForm->getNotificationSettingCategories(NULL)
#1 /Users/vitalii/Documents/development/ojs-main/ojs/lib/pkp/controllers/tab/user/ProfileTabHandler.php(342): PKP\notification\form\PKPNotificationSettingsForm->fetch(Object(APP\core\Request))
#2 [internal function]: PKP\controllers\tab\user\ProfileTabHandler->notificationSettings(Array, Object(APP\core\Request))
#3 /Users/vitalii/Documents/development/ojs-main/ojs/lib/pkp/classes/core/PKPRouter.php(465): call_user_func(Array, Array, Object(APP\core\Request))
#4 /Users/vitalii/Documents/development/ojs-main/ojs/lib/pkp/classes/core/PKPComponentRouter.php(291): PKP\core\PKPRouter->_authorizeInitializeAndCallRequest(Array, Object(APP\core\Request), Array)
#5 /Users/vitalii/Documents/development/ojs-main/ojs/lib/pkp/classes/core/Dispatcher.php(163): PKP\core\PKPComponentRouter->route(Object(APP\core\Request))
#6 /Users/vitalii/Documents/development/ojs-main/ojs/lib/pkp/classes/core/PKPApplication.php(379): PKP\core\Dispatcher->dispatch(Object(APP\core\Request))
#7 /Users/vitalii/Documents/development/ojs-main/ojs/index.php(21): PKP\core\PKPApplication->execute()
#8 {main}
  thrown in /Users/vitalii/Documents/development/ojs-main/ojs/classes/notification/form/NotificationSettingsForm.php on line 27

To Reproduce
Steps to reproduce the behavior:

  1. OJS instance should have at least 2 journals installed
  2. Log in as an admin user, go to the Administration -> Site settings
  3. In the user navigation, click on the Edit Profile
  4. Click on the Notifications tab
  5. See the error

What application are you using?
OJS main branch. Stable branch also needs testing

Additional information

notifications

As it affects only users with an admin role and we don't have site-level notification settings, I'd probably just avoid loading notifications settings in this case, when context isn't available. Another option could be to display the settings for each journal, the same way as it's done for Roles.

@Vitaliy-1 Vitaliy-1 added the Bug:1:Low A bug that does not have a severe consequence or affects a small number of users. label Jan 31, 2023
@Vitaliy-1 Vitaliy-1 added this to the 3.4 milestone Jan 31, 2023
@NateWr
Copy link
Member

NateWr commented Feb 1, 2023

Confirmed on main. I tested on stable-3_3_0 and could not reproduce it.

@NateWr
Copy link
Member

NateWr commented Feb 1, 2023

Looking at stable-3_3_0, it looks like it is possible to save notification settings at the site level using CONTEXT_ID_NONE (0). And it looks like it's possible to request notifications blocked at the site level with PKPNotificationOperationManager::getUserBlockedNotifications(). However, I don't see any notifications created without a context, so the site-level setting is never used.

We now have a foreign key on the notification_subscription_settings table that references the context id, so such values will be invalid in 3.4.

It is only possible to access the user profile at the site level for users who have roles in more than one journal. So if the user lands on this page, it is because they have roles in more than one journal. I think there are two things to do:

  1. When viewing the user profile at the site-level, replace the form in the Notifications tab with a list of links to the user's profile in each context they are registered in. Like this:

Manage notifications for each journal you are registered with.

  1. Write a pre-flight check to ensure there are no entries in notification_subscription_settings with invalid context_id.

@asmecher just want to run this plan by you. Does this sound right to you?

@asmecher
Copy link
Member

asmecher commented Feb 1, 2023

I would also be game to have the notification_subscription_settings.context_id column be nullable in the database -- that's the equivalent to CONTEXT_ID_NONE in SQL-land that doesn't break referential integrity with a made-up ID of 0. But we're talking about hypotheticals, since we don't have a current use case for CONTEXT_ID_NONE in this case (and the notification toolset will eventually get a rewrite), so I'm OK either way.

NateWr added a commit to NateWr/pkp-lib that referenced this issue Feb 2, 2023
NateWr added a commit to NateWr/ojs that referenced this issue Feb 2, 2023
NateWr added a commit to NateWr/omp that referenced this issue Feb 2, 2023
NateWr added a commit to NateWr/ops that referenced this issue Feb 2, 2023
@NateWr
Copy link
Member

NateWr commented Feb 2, 2023

That was much easier, so I've done that. The context column in notification_subscription_settings is now nullable, which fixes this bug and allows subscription opt-outs to be saved at the site-wide level.

PRs:
#8600
pkp/ojs#3738
pkp/omp#1317
pkp/ops#453

NateWr added a commit that referenced this issue Feb 2, 2023
#8592 Fix site-wide notification subscriptions form
NateWr added a commit to pkp/ojs that referenced this issue Feb 2, 2023
NateWr added a commit to pkp/omp that referenced this issue Feb 2, 2023
NateWr added a commit to pkp/ops that referenced this issue Feb 2, 2023
@NateWr
Copy link
Member

NateWr commented Feb 2, 2023

Thanks all, merged to main.

@NateWr NateWr closed this as completed Feb 2, 2023
defstat added a commit to defstat/ojs that referenced this issue Feb 15, 2023
defstat added a commit to defstat/pkp-lib that referenced this issue Feb 15, 2023
defstat added a commit to defstat/ui-library that referenced this issue Feb 15, 2023
defstat added a commit to defstat/omp that referenced this issue Feb 15, 2023
defstat added a commit to defstat/ops that referenced this issue Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug:1:Low A bug that does not have a severe consequence or affects a small number of users.
Projects
Status: Done
Development

No branches or pull requests

4 participants