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

Possible PHP notices on Site Health info page with Dashboard Sharing #6597

Closed
aaemnnosttv opened this issue Feb 16, 2023 · 2 comments
Closed
Labels
Exp: SP P1 Medium priority PHP Type: Bug Something isn't working Type: Support Support request

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Feb 16, 2023

Bug Description

Site Kit currently adds a number of fields to its dedicated Site Health section on the "Info" tab of the core Site Health page. For dashboard sharing, additional fields are added for information related to the feature.

Currently it's possible to raise a PHP Notice for array access on an undefined index when dashboard sharing is enabled. This happens when sharing settings fields are being constructed and the implementation assumes that every shareable module will have a respective entry in the sharing settings, which isn't the case.

Steps to reproduce

  1. Enable dashboardSharing
  2. Reset the dashboard sharing settings, if any are set
  3. Go to the Site Health > Info tab
  4. See notices (in console, debug log, or Query Monitor, etc)

Screenshots

image

Additional Context

  • No E2E coverage currently exists for the Site Health integration, and any PHP notices like this would cause it to fail if it were, so adding a basic test or two here would be a nice enhancement

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • No PHP notices should be raised from Site Kit when viewing its info in Site Health
  • All dashboard sharing specific fields should only be added to the Site Health data when the feature is enabled, including "Recoverable Modules" as well as information about shared roles and management

Implementation Brief

  • Finish PR started in Fix Site Health notices and add basic E2E tests #6598
    • So far this only adds E2E coverage and fixes a flaw/quirk in the debug log assertions so that the notices raised here will be caught
    • Add guards for dashboard sharing fields to return early if dashboardSharing isn't enabled
    • Update Debug_Data::get_module_sharing_settings_fields to access keys of the sharing settings safely

Test Coverage

  • Add basic E2E coverage for Site Kit's Site Health integration (basically load the screen, ensure the SK section is there, and to ensure no notices are raised which is part of every E2E test automatically)

QA Brief

  • See instructions above for how to reproduce, which is simple – essentially any shareable module (module showing up in the dashboard sharing settings) but without any settings saved for it will result in notices raised when viewing the Site Health info tab
  • The fix should also be covered by E2E (added here) and going forward

Changelog entry

  • Prevent PHP errors on the Site Health info page when Dashboard Sharing is enabled.
@aaemnnosttv aaemnnosttv added Type: Bug Something isn't working Type: Support Support request P1 Medium priority labels Feb 16, 2023
@aaemnnosttv aaemnnosttv self-assigned this Feb 16, 2023
@aaemnnosttv aaemnnosttv removed their assignment Feb 16, 2023
@eugene-manuilov eugene-manuilov self-assigned this Feb 16, 2023
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Feb 16, 2023
@techanvil techanvil assigned techanvil and aaemnnosttv and unassigned techanvil Feb 20, 2023
@aaemnnosttv aaemnnosttv assigned techanvil and unassigned aaemnnosttv Feb 20, 2023
@techanvil techanvil removed their assignment Feb 21, 2023
@mohitwp mohitwp self-assigned this Feb 22, 2023
@mohitwp
Copy link
Collaborator

mohitwp commented Feb 22, 2023

QA Update ✅

  • Tested on main environment.
  • Notices are appearing under debug log and Query monitor on latest environment with Dashboard sharing.
  • On main branch, PHP notices on Site health info page with Dashboard sharing are not raising now.
  • All dashboard sharing specific fields added to the Site Health data when the feature is enabled, including "Recoverable Modules" as well as information about shared roles and management.
  • Also, tested Dashboard sharing functionality.

image

Recording.240.mp4

@mohitwp mohitwp removed their assignment Feb 22, 2023
@felixarntz felixarntz assigned felixarntz and unassigned felixarntz Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP P1 Medium priority PHP Type: Bug Something isn't working Type: Support Support request
Projects
None yet
Development

No branches or pull requests

5 participants