POC new component for copying reports

[snake14]

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. No fields need to be defined for the slot either, like this example:

For those with access, there's a draft PR for the heatmaps plugin to demo and test the current implementation.

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

[snyk-io]

🎉 Snyk checks have passed. No issues have been found so far.

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

[snake14]

@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.

[james-hill-matomo]

@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.

[caddoo]

@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.

[snake14]

Thank you @caddoo . Your example usage looks pretty close to what I was using to test. 👍
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.

[github-actions]

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

[snake14]

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.

[michalkleiner]

A high level review

Tested this with the Heatmaps plugin PR, and in general it seemed to work. I haven't tested it with other entity types.

I've left some inline comments that are mostly informative, small mentions or suggestions that don't need to be actioned as this is a POC.

---

I also agree with Matt that we could perhaps think about the naming a bit more. I think we don't need to prefix it with Matomo as it's just another piece of functionality in the CoreHome and is not super specific to any aspect of Matomo. Similar feature could be used in other products, so I'd be a bit more generic.

We mention 'entity' in a few places and I think that's a good term for what this copies — an entity of some type. It's not a record, it's not an object, row or action, so a generic 'entity' sounds good to me.

Copy is probably a bit ambiguous, too, I would probably suggest the verb 'duplicate' or 'replicate'. For the back-end code, having the word 'modal' in the name could also be confusing as it holds utility classes but not really anything UI modal specific.

We could consider something like EntityDuplicator, EntityCopier, EntityCloner or even more generic CloneTool or CloneWizard, despite having just a single step at the moment.

---

As we currently only support cloning to a single site, for now I would remove the handling of multiple sites in copyEntity method, and only add it when it's needed to make the final review easier.

---

I'm not 100% sure on sharing the store between the action and the modal as the action basically just updates the store to show the modal and gets the title for the tooltip. I think this might be better passed as a prop for the title and in the parent component, handle an event emitted by the action to open the modal. That will simplify the dependencies.

Would we then need the store to be a separate component and not part of the modal? Would it still need to be reactive?

---

A suggestion — it seems that the additions/changes to the FieldSite and SitesManager API could probably be separated out into a separate PR as well and basically reviewed and merged independently to make the final review of the addition easier/smaller.

[michalkleiner]

Could perhaps have a list defined in a constant so that it can be extended if needed instead of adding more lines here? Alternatively, does it cause any problems leaving the params there?

[snake14]

@michalkleiner Yes. I could have a list, but I don't think that's necessary. They likely aren't hurting anything, but why send irrelevant data to the server?

[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 👍

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?

[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.

[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.

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.

[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.