Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent adding duplicate dashlet if present with same name and label #20375

Merged
merged 1 commit into from
Jun 3, 2021

Conversation

monishdeb
Copy link
Member

Overview

Steps to replicate:

  1. Go to any existing report instance (say 'Activity Details') >> enable for 'Available for Dashboard' >> Save
  2. Then change the 'Limit Dashboard Results' count and save again.
  3. Go to homepage, and click on 'Available Dashlets'

Before

There are duplicate dashlets under 'Available Dashlets'

After

Updates the existing dashlets.

@civibot
Copy link

civibot bot commented May 21, 2021

(Standard links)

@civibot civibot bot added the master label May 21, 2021
$dashlet->name = $params['name'] ?? NULL;
$dashlet->url = $params['url'] ?? NULL;
$dashlet->url = $params['label'] ?? NULL;
$dashlet->find(TRUE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the overall issue, but am wondering if this is a typo: setting $dashlet->url to equal $params['label'] ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this, was a typo on my end :/ Updated the patch now.

Copy link
Member

@colemanw colemanw Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we search for existing dashlets by label? Labels are not guaranteed to be unique.

The existing code has this legacy quirk where even if you don't pass an id it will still attempt to update based on name or url. Removing the matching on url feels like a good step forward. It removes half of the legacy behavior (which IMO would be nice to get rid of completely). But then this PR adds label in, which feels like a step backwards toward more legacy behavior. Can we just do name matching only?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm agree, updated the patch. Can you please check now?

@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label Jun 1, 2021
@demeritcowboy
Copy link
Contributor

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS
      • I didn't test fancy things, like multiple domains, multiple users...
  • Developer standards
    • [r-tech] PASS
    • [r-code] PASS
    • [r-maint] ?
    • [r-test] PASS

@colemanw colemanw removed the merge ready PR will be merged after a few days if there are no objections label Jun 2, 2021
@monishdeb
Copy link
Member Author

Jenkins test this please

@demeritcowboy
Copy link
Contributor

This runs ok for me. And you can still have, e.g. two versions of the same civireport like chart and table as separate dashlets if you save a copy (separate report instance).

@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label Jun 2, 2021
@colemanw colemanw merged commit 8d4d542 into civicrm:master Jun 3, 2021
@seamuslee001 seamuslee001 deleted the core-65 branch June 3, 2021 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants