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

Use saved ViewDataTable parameters mechanism for datatables displayed inside a widget container. #13088

Merged
merged 2 commits into from Jun 26, 2018

Conversation

diosmosis
Copy link
Member

Changes:

  • Introduce new containerId parameter sent by widget containers when getting child widgets. If found in a request, ViewDataTable will look for persisted ViewDataTable parameters.
  • containerId is appended to the option name for persisted parameters when present. This separates reports used within widget containers from the same reports used outside of widget containers. So if a user adds a metric to the Visits Overview (with graph), the change won't affect the Visitors > Overview page.
  • Call CoreHome.saveViewDataTableParameters when a report is either not in a widget (as before), or is in a widget but is in a widget container (new).

Fixes #11499

@diosmosis diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Jun 21, 2018
@diosmosis diosmosis added this to the 3.6.0 milestone Jun 21, 2018
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work as expected. Left two minor comments...

@@ -116,7 +117,7 @@ public static function build($defaultType = null, $apiAction = false, $controlle
if (!empty($report) && $controllerAction === $apiAction) {
$paramsKey = $report->getId();
}
$params = Manager::getViewDataTableParameters($login, $paramsKey);
$params = Manager::getViewDataTableParameters($login, $paramsKey, $containerId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$containerId might be undefined here, as it is set in an if condition

var reportId = $(domWidget).closest('[data-report]').attr('data-report');

var ajaxRequest = new ajaxHelper();
ajaxRequest.addParams({
module: 'CoreHome',
action: 'saveViewDataTableParameters',
report_id: reportId
report_id: reportId,
containerId: containerId,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guess the , in the end of the object list doesn't hurt, as we only support modern browsers...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove, force of habit.

@diosmosis
Copy link
Member Author

Updated the PR

@sgiehl sgiehl merged commit d780a73 into 3.x-dev Jun 26, 2018
@sgiehl sgiehl deleted the 11499-multiple-metrics-slow branch June 26, 2018 07:43
InfinityVoid pushed a commit to InfinityVoid/matomo that referenced this pull request Oct 11, 2018
… inside a widget container. (matomo-org#13088)

* Use saved ViewDataTable parameters mechanism for datatables displayed inside a widget container.

* apply PR feedback
@eramirezprotec
Copy link
Contributor

eramirezprotec commented Jan 15, 2020

Hi! @sgiehl @diosmosis

When entering to, for example, Visitors -> Devices, and changing the visualization of the Device type (any report, actually) report to a vertical bar graph, and then selecting any metric, one request is being made:

https://XXX/index.php?date=today&module=CoreHome&action=saveViewDataTableParameters&report_id=DevicesDetection.getType&containerId=&idSite=YY&period=day

With the following response:

<div class='alert alert-danger'><strong>Error:</strong> Setting parameters rows_to_display is not allowed. Please report this bug to the Matomo team.</div>

I tried the same in the Matomo Demo and the response received is this one:

<div class="alert alert-danger"><p><strong>Error:</strong> Your session has expired due to inactivity. Please log in to continue.</p><p><a href="index.php?module=Login">Sign in</a></p></div>

I'm guessing the metric(s) selected can't be persisted, and it's ok, but why creating an error and leaving it in the Apache log? If it's supposed to be expected behavior.

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

in Graphs, selected metrics may not be saved and lost after a page reload
3 participants