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

Update widget registration to declare associated modules #4849

Closed
aaemnnosttv opened this issue Feb 15, 2022 · 8 comments
Closed

Update widget registration to declare associated modules #4849

aaemnnosttv opened this issue Feb 15, 2022 · 8 comments
Labels
P0 High priority Type: Enhancement Improvement of an existing feature

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Feb 15, 2022

Feature Description

In #4807 the infrastructure for widget registration and selection was extended to allow for widgets to have one or more associated modules and to select them optionally filtered by a list of modules.

This issue is about updating the widget registration for (nearly) all widgets to declare their respective modules – i.e. the modules they are for or depend on.


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

Acceptance criteria

  • All widgets should have their registration updated to include all modules they depend on, passing the module slug(s)
    • Widgets registered in a module's registerWidgets function should all declare the current module as a minimum (in most cases, this will be all that's needed)
    • Any widget that depends on another module (e.g. AdSense's DashboardTopEarningPagesWidget – note the multiple whenActive use) should be registered with both/all modules
    • Note: At this point, the (legacy) URL search widget should be the only widget not associated with any module.

Implementation Brief

  • Using assets/js/modules/adsense/index.js,
    • Update all registerWidget calls to include modules: [ 'adsense' ] in their second parameter except the following widgets where additional modules should be added:
      • adsenseTopEarningPages: analytics should be added modules
      • adsenseModuleTopEarningPages: modules should contain analytics only (no adsense).
  • Using assets/js/modules/analytics/index.js,
    • Update all registerWidget calls to include modules: [ 'analytics' ] in their second parameter.
  • Using assets/js/modules/idea-hub/index.js,
    • Update all registerWidget calls to include modules: [ 'idea-hub' ] in their second parameter.
  • Using assets/js/modules/pagespeed-insights/index.js,
    • Update all registerWidget calls to include modules: [ 'pagespeed-insights' ] in their second parameter.
  • Using assets/js/modules/search-console/index.js,
    • Update all registerWidget calls to include modules: [ 'search-console' ] in their second parameter except the following widgets where additional modules should be added:
      • searchFunnel: analytics should be added modules

Test Coverage

  • No new tests to be added.

QA Brief

Changelog entry

  • Update widget registration to declare associated modules.
@aaemnnosttv aaemnnosttv added P0 High priority Type: Enhancement Improvement of an existing feature labels Feb 15, 2022
@aaemnnosttv aaemnnosttv self-assigned this Feb 15, 2022
@aaemnnosttv
Copy link
Collaborator Author

@felixarntz is there anything you can think of more concretely that should determine when a module (other than the main module for a widget) should be associated with a widget?

@felixarntz
Copy link
Member

@aaemnnosttv There may be widgets that don't depend on any module, I think that would be good to point out in the ACs as well. For example the (legacy UI) URL search widget, but potentially also the AdSense CTA widget? For the latter, I'm actually wondering, what does "associated module" really mean for us? The AdSense CTA widget does not get any real data from AdSense, so the user doesn't need to have shared data access to see it. At the same time, that widget still shouldn't be shown to non-signed in users since they can't configure the module anyway.

@aaemnnosttv
Copy link
Collaborator Author

For example the (legacy UI) URL search widget, but potentially also the AdSense CTA widget? For the latter, I'm actually wondering, what does "associated module" really mean for us?

Yes, I think the search widget is the only one I found that would be without any module.

As for the AdSense CTA widget (and your question about what the associated module really means for us), I think the association is primarily about the data source(s) used within it, but even for a widget that did not display any data, if it was somehow specific to the AdSense module, it should be associated with it.

At the same time, that widget still shouldn't be shown to non-signed in users since they can't configure the module anyway.

Right, which could be handled either as conditional widget registration, logic within the widget (returning WidgetNull if not capable) or maybe we could further extend widget registration with required capabilities? This one would then require our MANAGE_OPTIONS to be returned. With that in mind, maybe using capabilities would be a cleaner separation to use that would still allow for the a list of widgets by module using the new meta capabilities (although not sure exactly how that would work for authenticated admins)? This is a bit closer to how WP registers some things as well (e.g. admin pages). I'm not necessarily suggesting we make this change, but what do you think?

@felixarntz
Copy link
Member

As for the AdSense CTA widget (and your question about what the associated module really means for us), I think the association is primarily about the data source(s) used within it, but even for a widget that did not display any data, if it was somehow specific to the AdSense module, it should be associated with it.

That makes sense. I agree we can then include a "manual" capability check in the CTA widget to return WidgetNull if the user doesn't have the required capability.

maybe using capabilities would be a cleaner separation to use that would still allow for the a list of widgets by module using the new meta capabilities (although not sure exactly how that would work for authenticated admins)? This is a bit closer to how WP registers some things as well (e.g. admin pages). I'm not necessarily suggesting we make this change, but what do you think?

That's a great idea. However I don't think it's necessarily a replacement for what we're doing here, it could be another future enhancement, allowing to control widget access based on capability. I think for now we can rely solely on the module approach as originally defined and check for capabilities in the few places needed manually, but it would be great to eventually have a (not dashboard sharing-related) issue to explore capability-based access.

We can move this forward to IB.

@asvinb asvinb assigned asvinb and unassigned asvinb Mar 23, 2022
@eugene-manuilov eugene-manuilov self-assigned this Mar 25, 2022
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Mar 25, 2022
@kuasha420 kuasha420 self-assigned this Apr 14, 2022
@kuasha420 kuasha420 removed their assignment Apr 15, 2022
@asvinb asvinb assigned asvinb and unassigned asvinb Apr 15, 2022
@wpdarren wpdarren self-assigned this Apr 18, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Apr 18, 2022

QA Update: ⚠️

make sure all the widgets are rendering for the authenticated user as usual.

@kuasha420 please could you confirm if this should be tested with dashboardSharing enabled, or not? Or, is it worth running through both enabled and disabled at this point? Please clarify.

@aaemnnosttv
Copy link
Collaborator Author

@wpdarren it just means make sure the widgets are all showing as before and that no regressions have been introduced in changing the widget registrations. There is no intended change to what should be seen on the dashboards yet at this point. The changes here are related to dashboard sharing but not based on the feature flag.

@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • Checked that all widgets appear on main and entity dashboard.
  • Checked with zeroDataStates enabled and disabled. Check
  • Checked on a site with data and also one with zero/gathering data.

@wpdarren wpdarren removed their assignment Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants