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

Add system status check for missing dedupe rules #22369

Merged
merged 1 commit into from
Jan 5, 2022

Conversation

braders
Copy link
Contributor

@braders braders commented Jan 4, 2022

Overview

As discussed in core#2918, CiviCRM assumes that Unsupervised and Supervised dedupe rules will always exist, but allows the system to get into a state where only General rules are actually configured. This adds a system check to warn when either Unsupervised or Supervised dedupe rules are missing.

Before

It was possible to configure CiviCRM without either Unsupervised or Supervised dedupe rules. This can lead to exceptions being thrown, blocking certain functionality from working correctly. For example:

  • Adding users in WordPress (this is the bug reported in core#2918)
  • Registering users for events on the public facing area of the site (this is an issue I have noticed more recently when dedupe rules were missing)

After

If dedupe rules have not been configured correctly, system messages will be shown warning the user that they should review their configuration. A link to the dedupe contacts screen is included, and from here the configuration can be changed:

Screenshot 2022-01-04 at 18 57 59

Note that it's still too easy (in my opinion) to get into a bad state, and I'd still like to implement the UI improvements suggested on core#2918 if I can find time.

Technical Details

The code felt tidiest when split into two check functions, both using a shared getContactTypesForRule method. This does mean that two very similar database queries are required, rather than a single query. However, I belive this should be a fairly efficient database call and so I don't think this should be a massive problem.

I've not included tests because:

  • I wasn't quite sure how you'd go about writing tests for something like this,
  • None of the other checks in CRM/Utils/Check/Component/ had tests which I could use as a reference.

Comments

I wasn't 100% sure which severity and icon to go for. I've settled on \Psr\Log\LogLevel::WARNING and fa-server, but did very much wonder if \Psr\Log\LogLevel::ERROR would be more appropriate.

@civibot
Copy link

civibot bot commented Jan 4, 2022

(Standard links)

@civibot civibot bot added the master label Jan 4, 2022
@colemanw
Copy link
Member

colemanw commented Jan 5, 2022

I've given this r-run on the demo site and all works well! Good to merge, aside from one outstanding code nitpick.

@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Jan 5, 2022
@colemanw colemanw added merge on pass and removed merge ready PR will be merged after a few days if there are no objections labels Jan 5, 2022
@colemanw
Copy link
Member

colemanw commented Jan 5, 2022

@braders here is the other followup I mentioned: #22383. As with #22382 it would be great if you have a minute to test & leave a review comment on the PR. That way someone with merge rights will see that it's been reviewed.

@colemanw colemanw merged commit bf2fc66 into civicrm:master Jan 5, 2022
@colemanw
Copy link
Member

colemanw commented Jan 6, 2022

@braders I did one more follow-up: #22389

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants