Regression analysis 5.3.0 (fixed 5.3.2) - Duplicate Contact Checking with ajax off
What happened?
An improvement to the automatic Duplicate Checking feature of the “New Contact” form gave better results when entering data, but caused the “Check for Matching Contacts” button to stop working when manually clicked.
How & When was it addressed
The bug was released in 5.3. It was fixed in 5.3.2 and 5.4.0, by the original author, about 24 hours after being reported on stack exchange by Robin Fenwick. Robin and Seamus Lee both diligently followed up to test and merge the fix. 5.3.2 was issued within 72 hours of the bug being found
How did the regression fit with our processes?
The original improvement was tested by a number of people. However, they did not test all workflows. Our focus was on getting the new feature working correctly and there were a number of hurdles, notably that the default “Supervised” deduping rule is not well-suited for checking matches during data entry. This is also an old area of the codebase which does not contain any unit tests.
The additional (less used & less tested) workflow involved a setting in non-default mode. Developers are often keen to introduce settings as a way to get something into core that not everyone will want but they reduce the robustness of our product maintenance.
The review time taken from raising the review to getting it merged into core was very long on the original PR - this generally increases risk.
Recommendations going forwards
- Identify when a PR touches code that does not have test coverage, and require extra vigilance in QA, either in the form of new tests, or in complete manual testing.
- In this case, writing a unit test was not practical because there is no testing framework in place for the CiviCRM front-end. This deficit still needs to be addressed at some point but was out-of-scope for such a small change.
- Try to prevent more setting coming into core as they create more complexity and often are hard to test.
- Keep the PR queue manageable (including closing non-reviewable PRs) to reduce drawn out PR work and adopting a 'finishing oriented' approach - ie. try to finish more work & start less