matomo POC new component for copying reports

Description:

This is a POC for a new component which can be used throughout the system for implementing the ability to copy reports and other entities, like goals, funnels, and so on. It uses some events to alert the parent of certain actions, such as validation. Some changes were made to SiteSelector to allow somewhat more configurability.

For right now, we're simply allowing one site and not allowing roll-up sites. We plan to add the ability to select more than one site at a time and come up with a better mechanism for determining whether roll-up sites should be allowed based on what is being copied.

Here is a test implementation:

The word 'report' is the default value for what is being copied and can be overridden using a prop. The div with the test form inputs is the slot defined by the parent view and is scrollable when it exceeds the available height. The copy button is disabled until validation passes.

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

May 21 '25 05:05 snake14

:tada: Snyk checks have passed. No issues have been found so far.

:white_check_mark: security/snyk check is complete. No issues have been found. (View Details)

May 21 '25 05:05 snyk-io[bot]

@caddoo Could some of the team look over this POC draft PR and give some initial feedback on the approach? They can use the Jira ticket PG-4019 as reference.

May 21 '25 05:05 snake14

@snake14 LGTM, with the caveat that I'm new to the codebase. I've only looked at it from a PoC and structural point of view, and haven't reviewed any code in depth, or tested.

May 21 '25 23:05 james-hill-matomo

@caddoo Could some of the team look over this POC draft PR and give some initial feedback on the approach? They can use the Jira ticket PG-4019 as reference.

Appreciate the POC draft approach @snake14 .

As this is a POC, i will comment just on the code, the interface for other developers and also a bit on the UI.

Some feedback items ( I don't know how much are already decisions, so will just mention all ).

On the interface for the component.

I had to try and use it myself to try and understand how it works and came up with:

  <MatomoCopyModal
    v-model="showCopyModal"
    :copyEntityType="'dashboard'"
    :formData="copyFormData"
    @copySuccessful="handleCopySuccess"
    @resetFormData="resetForm"
  >
    <input v-model="copyFormData.title" placeholder="Enter new title" />
  </MatomoCopyModal>

Maybe naming a component other than MatomoCopyModal from reading it, I'm not sure if it's copying text or anything else intention isn't clear. But I also understand because it supports multiple entity types that might be difficult, but just raising as that may be an issue.

The property called copyEntityType looks like it should be a translated string to be displayed in the dialog. Maybe just name it 'modalTitle' and people can pass in something like 'translate('CoreHome_CopyX', 'dashboard')'. Clearer intention.

formData again could probably use a strict type or something, so it's clear what to expect as input (again might just be because PoC).

In terms of UI, it feels like an anti-pattern to have disabled submit buttons, the user doesn't know what to do in order to make it active. Usually they would be able to click it to see what is required, but again just opinion.

May 22 '25 00:05 caddoo

Thank you @caddoo . Your example usage looks pretty close to what I was using to test. :+1: Please see my inline responses below.

Maybe naming a component other than MatomoCopyModal from reading it, I'm not sure if it's copying text or anything else intention isn't clear. But I also understand because it supports multiple entity types that might be difficult, but just raising as that may be an issue.

I'm definitely open to different name ideas. I simply went with MatomoCopyModal because it's generic enough that it could be used for copying a variety of things and it's similar to the existing MatomoDialog, MatomoUrl, ... components.

The property called copyEntityType looks like it should be a translated string to be displayed in the dialog. Maybe just name it 'modalTitle' and people can pass in something like 'translate('CoreHome_CopyX', 'dashboard')'. Clearer intention.

Yes. It's supposed to be a translated item. I could also adjust it to allow a translation key. I kept it somewhat generic because it's used by 3 translations in the component, including the title. Any occurrences of 'report' in the example screenshot I provided in the description are using the copyEntityType prop.

formData again could probably use a strict type or something, so it's clear what to expect as input (again might just be because PoC).

I could use some feedback around this. I thought about using an interface, but I also want to allow each usage of the component to provide the data required to make a copy. For example, one report might only need the report ID and the destination site ID while others might want to select additional options. Leaving the formData object open allows validating and POSTing the the unique data required by Goals, Funnels, Segments, ... to make a copy.

In terms of UI, it feels like an anti-pattern to have disabled submit buttons, the user doesn't know what to do in order to make it active. Usually they would be able to click it to see what is required, but again just opinion.

That's a fair point. I was thinking about the save button of some of our update forms starting out disabled until the form is 'dirty', but this is a different use case. I can change the submit button to be enabled until validation fails.

May 23 '25 00:05 snake14

If you don't want this PR to be closed automatically in 28 days then you need to assign the label 'Do not close'.

Jun 25 '25 02:06 github-actions[bot]

The failing UI test case appears to be unrelated to this PR as I have another very different PR with the exact same test case failing.

Jul 04 '25 03:07 snake14

Thank you for your feedback @michalkleiner .

I will try to refactor using EntityDuplicator instead of MatomoCopyModal and similar names.
You make a good point about implementing only what's available. However, as there are already plans/tickets for implementing supporting multiple sites, I'm hesitant to remove the code I've included in preparation for that.
Tried using props and emitting before switching to the store, but that didn't work as cleanly as I would have liked and it resulted in what felt like duplication and increased complexity in using the components.
- I suppose that I could try defining shared properties in the modal and then passing the entire modal reference as a property to each action. I'll see if that simplifies or increases complexity.
I'll see if I can move the FieldSite and SitesManager API changes into a separate PR :+1:

Once all of your feedback has been addressed, is there anything else you see that needs to be done to transition this from POC to mergeable/releasable feature?

Jul 08 '25 23:07 snake14

I think it's ok to keep the store if it was too complicated/convoluted to pass around the events.

I'm wondering is there possibly a way how not to pass the store via props? I guess since we create a new store based on the duplicated entity type, it's hard to have a single instance of the store... just trying to think how this can be done differently.

Other than that I think in general this could transition from a POC to a feature.

Jul 09 '25 03:07 michalkleiner

I think it's ok to keep the store if it was too complicated/convoluted to pass around the events.

I'm wondering is there possibly a way how not to pass the store via props? I guess since we create a new store based on the duplicated entity type, it's hard to have a single instance of the store... just trying to think how this can be done differently.

Other than that I think in general this could transition from a POC to a feature.

Thank you @michalkleiner . I thought about having a singleton store which held a collection of modal states instead of local instances, but I felt that might lead to collisions when the same duplicateEntityType was used in more than one place.

Jul 09 '25 04:07 snake14

@michalkleiner I didn't have time to try refactoring the use of the store, but I believe that I addressed everything else that you mentioned, including create #23422 to split out the SiteSelector changes.

Jul 09 '25 06:07 snake14

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

Jul 26 '25 02:07 github-actions[bot]

Closing in favour of #23456

Jul 28 '25 22:07 snake14