@peterhashair opened this Pull Request on November 8th 2021 Contributor

Description:

Fixes: #18134
add disable archiving segment configuration for plugins

  • Under [General]-> disable_archiving_segment_for_plugins accept string, sting with comma, array.
  • [General_{siteId}] can be enable by Site ID.

I guess this will be 4.7

Review

@peterhashair commented on November 16th 2021 Contributor

The last test runs fully passed https://app.travis-ci.com/github/matomo-org/matomo/builds/241828886. the Last update just one wording in global.config.php, I think no need to run the test again.

@justinvelluppillai commented on November 16th 2021 Contributor

@peterhashair changing the config changes one of the screenshots

@peterhashair commented on November 16th 2021 Contributor

@justinvelluppillai right, let me run it again.

@sgiehl commented on November 19th 2021 Member

[General_{siteId}] can be enable by Site ID.

If I see that right, the site specific config would then only work for that specific configuration value, right? Imho we should do it the same way like in tracker config. So it would be available for all values in General section.

See https://github.com/matomo-org/matomo/blob/e0417845f8fd7031e00e97c3576415c998abb2ce/core/Tracker/TrackerConfig.php#L28-L47

I'm not sure if it's easy to adjust the config class to support that. Maybe we need to introduce a new class GeneralConfig (like TrackerConfig) and replace all usages of the general config over time then...

@peterhashair commented on November 19th 2021 Contributor

@sgiehl right, that makes sense let me do the adjustment.

@peterhashair commented on November 22nd 2021 Contributor

@sgiehl yes, don't need load General. sorry, my brain was dead there 😅

@peterhashair commented on November 23rd 2021 Contributor
@peterhashair commented on November 24th 2021 Contributor

@sgiehl update a little bit,

  • add site Id configure test overwrite general test.
  • case insensitive,
  • special case ignored.
@peterhashair commented on November 24th 2021 Contributor
@peterhashair commented on November 29th 2021 Contributor

@sgiehl that make more sense. 👍

@sgiehl commented on November 29th 2021 Member

@peterhashair Just had another look at the original issue and it seems there is one requirement, that isn't fulfilled currently:

In the report itself, when a segment is viewed and there is no data, then show a footer message in the report that viewing segments for this report is disabled.

If you need some hints to find the correct places to implement this. Maybe start having a look here:
https://github.com/matomo-org/matomo/blob/5d9cf649aeefa7a1390574c2b147ce972b1b5c39/core/Plugin/Visualization.php#L228-L235
https://github.com/matomo-org/matomo/blob/1023fb7d691ebf47c27aa8a1cab4702c55c2630d/plugins/CoreHome/templates/_dataTable.twig#L58-L68

@peterhashair commented on November 29th 2021 Contributor

@sgiehl I did a mock-up for the requirement. Not sure that's the right approach for this, do I need to implement a UI test for that one?

In the report itself, when a segment is viewed and there is no data, then show a footer message in the report that viewing segments for this report are disabled.

Powered by GitHub Issue Mirror