@snake14 opened this Pull Request on September 15th 2022 Contributor

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

@snake14 commented on September 15th 2022 Contributor

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

@diosmosis commented on September 15th 2022 Member

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 commented on September 15th 2022 Contributor

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 commented on September 15th 2022 Member

@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 commented on September 16th 2022 Contributor

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 commented on September 16th 2022 Member

@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 commented on September 16th 2022 Contributor

@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 commented on September 22nd 2022 Contributor

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 commented on September 22nd 2022 Contributor

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 commented on September 22nd 2022 Contributor

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 commented on September 22nd 2022 Contributor

@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 commented on September 22nd 2022 Contributor

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

@snake14 commented on September 22nd 2022 Contributor

@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 commented on September 22nd 2022 Contributor

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

@snake14 commented on September 22nd 2022 Contributor

@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 commented on September 26th 2022 Contributor

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

@peterhashair commented on September 26th 2022 Contributor

@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 commented on September 26th 2022 Contributor

@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 commented on September 26th 2022 Contributor

@snake14 yes, nothing related to this PR

@justinvelluppillai commented on September 26th 2022 Member

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 commented on September 26th 2022 Contributor

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

This Pull Request was closed on September 27th 2022
Powered by GitHub Issue Mirror