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

Core#2918 Implement new UI for configuring dedupe rule usage. #22804

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

braders
Copy link
Contributor

@braders braders commented Feb 20, 2022

Overview

This implements the new UI proposed and discussed in Core#2918.

Before

Currently the UI for configuring dedupe rules allows the usage to be selected from a list of radio options:

Screenshot 2022-02-20 at 14 55 17

This is problematic for multiple reasons:

  • It allows a situation where no supervised or unsupervised rule exists (A system check to warn against this was added in Add system status check for missing dedupe rules #22369).
  • It incorrectly suggests that multiple supervised or unsupervised rules can be configured.
  • The help text is somewhat squeezed into the helptext popup.

After

The UI is now driven through a popup allowing much more context into the available options:
Screenshot 2022-02-20 at 14 58 18

It is no longer possible to change the usage for supervised and unsupervised rules - the "change usage" button is disabled, and the title attribute set to read "To change the usage for this rule, please configure another rule as Supervised":

Screenshot 2022-02-20 at 14 59 08

Comments

Minor CSS adjustments have been made, which will affect non-default themes. However, without the CSS changes the functionality is still perfectly usable, so this shouldn't be a big issue.

@civibot
Copy link

civibot bot commented Feb 20, 2022

(Standard links)

@civibot civibot bot added the master label Feb 20, 2022
@braders braders force-pushed the core-2198-dedupe-rules-ui branch from d2a3d32 to faff5d8 Compare February 20, 2022 15:05
@braders
Copy link
Contributor Author

braders commented Feb 20, 2022

There seems to be style failures in civicrm.css that are not releated to my changes. Additionally the semi-colon failure on civicrm.css:1524 looks to be a failure of the stylechecker rather than a legitimate issue; I'm not the correct way to resolve that.

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@jaapjansma
Copy link
Contributor

test this please

@jaapjansma
Copy link
Contributor

@braders We are reviewing PR's and we came across this one. We checked the PR in the related test environment to this PR. However we could not click Change Usagge. Nothing happens when we click on that button. We tried it with an existing rule, a new rule in both cases nothing lets us change the Usage. Can you have another look?

@braders
Copy link
Contributor Author

braders commented Feb 28, 2022

@jaapjansma Thanks for spotting that. It appears I was using Drupals outdated jQuery rather than CRM.$. I'll try and push a fix tonight or later this week.

@braders braders force-pushed the core-2198-dedupe-rules-ui branch from faff5d8 to 232d712 Compare February 28, 2022 19:12
@braders
Copy link
Contributor Author

braders commented Feb 28, 2022

@jaapjansma This is now working correctly in the test environment.

@yashodha
Copy link
Contributor

yashodha commented Mar 2, 2022

@braders this will address https://lab.civicrm.org/dev/core/-/issues/1769 as well

@BettyDolfing
Copy link

test this please

@jaapjansma
Copy link
Contributor

  • General standards
    • Explain (r-explain)
      • PASS : The goal/problem/solution have been adequately explained in the PR.
    • User impact (r-user)
      • PASS: The change would be intuitive for a majority of users who work with this feature.
    • Documentation (r-doc)
      • PASS: We have checked the documentation and this still works as how it is explained in the documentation.
    • Run it (r-run)
      • PASS: We created and edited dedupe rules.
  • Developer standards
    • Technical impact (r-tech)
      • PASS: The change preserves compatibility with existing callers/code/downstream.
    • Code quality (r-code)
      • PASS: The functionality, purpose, and style of the code seems clear+sensible.
    • Maintainability (r-maint)
      • PASS: The change is trivial enough that it does not require tests.
    • Test results (r-test)
      • PASS: The test results are all-clear.

@eileenmcnaughton or @seamuslee001 can one of you merge this PR?

@jaapjansma jaapjansma added the merge ready PR will be merged after a few days if there are no objections label Mar 14, 2022
@colemanw
Copy link
Member

In general, CiviCRM has a problem with managing css. I certainly don't love adding lines to the global civicrm.css file just for the sake of one screen, but it's a pre-existing condition and at least what you added is well-commented.

@colemanw colemanw merged commit 629bcbf into civicrm:master Mar 14, 2022
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.

6 participants