Adding the abililty to exclude specific sites from the site selector

[snake14]

Description:

I needed the ability to exclude specific sites from the Vue SiteSelector component. I tried to avoid negatively impacting other views/components that use the same SiteStore. Issue: PG-233 (used to be DEV-3068) There is a related pull request for the RollUpReporting plugin that uses the changes in this branch.

Review

• [ ] Functional review done
• [ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
• [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
• [ ] Security review done
• [ ] Wording review done
• [ ] Code review done
• [ ] Tests were added if useful/possible
• [ ] Reviewed for breaking changes
• [ ] Developer changelog updated if needed
• [ ] Documentation added if needed
• [ ] Existing documentation updated if needed

[snake14]

Hi @diosmosis . Would you please take a quick look at this draft PR and let me know if this is a good approach?

[diosmosis]

Personally I think it's probably better to exclude the sites server side in the query (ie, NOT IN (...)), otherwise the display may not be the full result set (eg, someone excludes 10 sites, searching results in 20 sites, the first 10 are excluded, the other 10 are not, but nothing displays because the site selector limits the display to the first 10 sites).

It's worth asking developers on the core team as well (cc @matomo-org/core-team).

[snake14]

Thank you @diosmosis . I thought about doing the exclusion server side, but there are multiple views/components that use the sites cached in the SitesStore and I didn't want the sites excluded for one view to exclude the sites for other views. Another option I considered was not altering SitesStore at all and simply have all the exclusion logic in the SiteSelector component itself. That was just more difficult since the cached list of all sites provided by SitesStore is read only. I can ask for feedback in the developer channel.

[diosmosis]

@snake14 Ok. In case it helps to know, the reason multiple components share the same state is just because that's how it was done in AngularJS. So changing things so the SiteSelector owns the list of sites it displays rather than sharing it with other SiteSelector components is also an option for you.

EDIT: You could also cache based on excluded sites instead of having just one cached variable.

[snake14]

Thanks @diosmosis . That's a good point. I could change the SiteSelector to use the SitesStore by default. Then, if the sitesToExclude prop is set, make a separate server call that filters based on the excluded sites. Would it be better to have that logic in the SitesStore or simply make the AJAX call right in the SitesSelector code?

[diosmosis]

@snake14 I would keep it in SitesStore after a refactor for more code reuse (adding a search function that doesn't use the store state and re-using it in the existing search function that does use it). I also haven't thought about it much so my opinion should be taken with a grain of salt. But you might want to ask others on the core team as I ultimately won't be the one to review your code.

[snake14]

@diosmosis Sounds good. I appreciate your input. I was already leaning toward trying to keep it in the SitesStore, so I'm glad you had a similar thought. I'll start working on adjusting the search to allow excluding specific site IDs and see if the rest of the core team chimes in with any input. Thanks again.

[snake14]

It seems, the changes broke the site selector, if there are multiple sites, should be a dropdown. You can see from this screen comparison. Let me know if you need help with that. https://builds-artifacts.matomo.org/matomo-org/matomo/pg-233-allow-exclusions-for-site-selector-vue-component/59246/SiteSelector_loaded.png

@peterhashair Didn't that artifact already exist in 4.x-dev? For example: https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/59201/SiteSelector_loaded.png

[peterhashair]

It seems, the changes broke the site selector, if there are multiple sites, should be a dropdown. You can see from this screen comparison. Let me know if you need help with that. https://builds-artifacts.matomo.org/matomo-org/matomo/pg-233-allow-exclusions-for-site-selector-vue-component/59246/SiteSelector_loaded.png

@peterhashair Didn't that artifact already exist in 4.x-dev? For example: https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/59201/SiteSelector_loaded.png

@snake14 it was supposed to be a dropdown, but now the dropdown is gone.

[snake14]

It seems, the changes broke the site selector, if there are multiple sites, should be a dropdown. You can see from this screen comparison. Let me know if you need help with that. https://builds-artifacts.matomo.org/matomo-org/matomo/pg-233-allow-exclusions-for-site-selector-vue-component/59246/SiteSelector_loaded.png

@peterhashair Didn't that artifact already exist in 4.x-dev? For example: https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/59201/SiteSelector_loaded.png

@snake14 it was supposed to be a dropdown, but now the dropdown is gone.

@peterhashair Yes, but it appears to be an existing issue with the build. The example that I provided above was from the 4.x-dev branch with doesn't contain my changes, but still has the same issue as the build for this branch.

[peterhashair]

@snake14 I don't think the issue is about the build. If you look at here, initialSites will always take SitesStore.initialSitesFiltered.value is proxy will always true.

     hasMultipleSites() {
 -     return SitesStore.initialSites.value && SitesStore.initialSites.value.length > 1;
 +    const initialSites = SitesStore.initialSitesFiltered.value
        ? SitesStore.initialSitesFiltered.value : SitesStore.initialSites.value;
      return initialSites && initialSites.length > 1;
    },

@peterhashair Yes, but it appears to be an existing issue with the build. The example that I provided above was from the 4.x-dev branch with doesn't contain my changes, but still has the same issue as the build for this branch.

[snake14]

@peterhashair Thank you for pointing out that issue in the hasMultipleSites() function. I will address that.

[snake14]

@peterhashair Thank you again for catching that code issue. There are still quite a few test cases failing, but I'm not seeing any related to my changes. Of course, it's hard to tell when there are so many tests failing. Please let me know if you see anything else that needs to be corrected.

[peterhashair]

@snake1 good work, I think if you revert changes to tests/resources/OmniFixture-dump.sql will fix those UI tests, shouldn't need to update dump SQL.

[snake14]

@peterhashair Yeah. I guess I shouldn't have used the auto-generated version of that file. I switched it back to the 4.x-dev version and made just enough changes to make the fixtures build correctly on my local machine. It's just the changes I should have made when I added those new columns to TagManager a while back.

[snake14]

H @peterhashair . Thank you again for your help. Is there anything else that you see that needs to be fixed?

[peterhashair]

@snake14 looks good to me, just revert the fixture SQL to 4.x-dev, I think you checkout from 5.x-dev, once the tests are passed, should be good to merge.

[snake14]

@snake14 looks good to me, just revert the fixture SQL to 4.x-dev, I think you checkout from 5.x-dev, once the tests are passed, should be good to merge.

@peterhashair Thank you. I checked out from 4.x-dev, but I then updated it to include the new columns for the TagManager plugin. I don't think the remaining test failures are due to my branch, but I could be wrong.

[peterhashair]

@snake14 yes, nothing related to this PR

[justinvelluppillai]

You can go ahead and merge approved PRs for contributors @peterhashair unless there's a reason not to. Non-core team members won't always have permissions to merge.

[snake14]

Thanks @justinvelluppillai . You are correct. It appears that I don't permission to merge this PR.