Product-maintenance issueshttps://lab.civicrm.org/eileen/Product-maintenance/-/issues2018-08-02T00:02:37Zhttps://lab.civicrm.org/eileen/Product-maintenance/-/issues/15Regression analysis 5.3.1 (and 4.6.38?) (fixed 5.3.2) Wordpress merge screen...2018-08-02T00:02:37ZeileenRegression analysis 5.3.1 (and 4.6.38?) (fixed 5.3.2) Wordpress merge screen urls broken**What happened?**
[A security fix](https://github.com/civicrm/civicrm-core/commit/095b12d6#diff-68d9e076998c7fbe8c7e6a89c64322e5R52) to prevent Cross browser scripting attacks was overzealous and url-encoded already encoded urls. This ...**What happened?**
[A security fix](https://github.com/civicrm/civicrm-core/commit/095b12d6#diff-68d9e076998c7fbe8c7e6a89c64322e5R52) to prevent Cross browser scripting attacks was overzealous and url-encoded already encoded urls. This still worked on Drupal but not on Joomla! & Wordpress
**How & When was it addressed **
The bug was released in 5.3.1. It was [identified a week later](https://lab.civicrm.org/dev/core/issues/279) and a release with the fix was issued within 24 hours.
**How did the regression fit with our processes?**
Security fixes are notoriously risky as they affect all sorts of flows that are hard to test and as of this month they are no longer going through the rc process. It's hard to see how we can avoid errors that affect some flows without markedly more resources. A quick fix to any issues is probably the correct and sensible process.
**Recommendations going forwards**
Our goal is to move to greater & greater precautionary escaping & preferably move to opt-out escaping in Smarty (although possibly not until the 4.6 LTS has expired at the end of the year in order to keep this work manageable). Ensuring ALL urls are handled the same at the php layer will help substantially here. We should deprecate passing $queryParams as a string to the CRM_System_Url function. We should also come up with a tpl marker like escape|none to indicate where we know we have done this5.3.2https://lab.civicrm.org/eileen/Product-maintenance/-/issues/14Regression analysis 5.3.0 (fixed 5.3.2) - Duplicate Contact Checking with aja...2018-07-26T03:09:08ZeileenRegression analysis 5.3.0 (fixed 5.3.2) - Duplicate Contact Checking with ajax off**What happened?**
An [improvement to the automatic Duplicate Checking feature](https://github.com/civicrm/civicrm-core/pull/10341) of the “New Contact” form gave better results when entering data, but caused the “Check for Matching Con...**What happened?**
An [improvement to the automatic Duplicate Checking feature](https://github.com/civicrm/civicrm-core/pull/10341) 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](https://github.com/civicrm/civicrm-core/pull/12552), by the original author, about 24 hours after being reported on [stack exchange](https://civicrm.stackexchange.com/questions/25834/bug-with-check-for-matching-contacts-button) 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**
1. 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.
2. 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.
3. Try to prevent more setting coming into core as they create more complexity and often are hard to test.
4. 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 less5.3.2https://lab.civicrm.org/eileen/Product-maintenance/-/issues/13Regression analysis 5.2.0 (fixed 5.3.2) Affects front event event registratio...2018-07-27T23:33:30ZeileenRegression analysis 5.2.0 (fixed 5.3.2) Affects front event event registrations with personal campaign pages configured.**What happened**
A [bug fix submitted by reviewers](https://github.com/civicrm/civicrm-core/pull/12056) to replace [a patch submitted that did not meet our coding expectations](https://github.com/civicrm/civicrm-core/pull/12044) introd...**What happened**
A [bug fix submitted by reviewers](https://github.com/civicrm/civicrm-core/pull/12056) to replace [a patch submitted that did not meet our coding expectations](https://github.com/civicrm/civicrm-core/pull/12044) introduced a fatal error when submitting event pages configured with PCPs. The original submitter had chosen not to test the final patch and neither myself nor Monish were very familiar with the code area and it appears we both tested (did r-run) on the contribution page flow & missed the events flow, not picking it up as a separate flow.
**How and when was it addressed.**
The bug was released in 5.2.0. It was not detected until 5.3.1 was released 6 weeks later (by the submitter of the original patch). Fixes will be in 5.3.2 and in 5.4 within 4 days of [the bug report](https://lab.civicrm.org/dev/core/issues/272) (the patch was written by Monish and tested by Karin G and merged with 24-48 hours but as this was not detected until 6 weeks after it was released we did not rush the release out.)
**How did the regression fit with our processes?**
We pursued the bug because it fit a broad definition of critical - ie. any fatal error. However, after dealing with this particular bug I altered the definition of critical, [at least as it affects the product maintenance team](https://lab.civicrm.org/eileen/Product-maintenance), to only consider long-standing fatal bugs as critical if the reporter was also engaged in following through to completion. So, our process was already somewhat changed in that had the same thing occurred now we would not have prioritised fixing the original bug without the reporter agreeing to test.
**Context**
A [patch was submitted](https://github.com/civicrm/civicrm-core/pull/12044) to fix an error hit when the field data entered for the note on a pcp field exceeded the 255 characters. This was presented as a critical bug although it was long standing (at least 5 years) and we were not aware of other reports.
Monish and I provided feedback requesting the fix be changed to match our preferred approach (we have been adopting addField functionality when we have to update fields for things like this since around 4.5 and in where ever possible ask for metadata based approaches to fixes as a general rule). The submitter indicated they was not going to make the changes and advised “If this does not meet standards feel free to close;”
As the bug was critical by our definition at the time, and I had already sunk in work to try to make the fix easier for the submitter by doing a metadata change](https://github.com/civicrm/civicrm-core/pull/12046), I proceeded to [complete the bug fix and put up a PR](https://github.com/civicrm/civicrm-core/pull/12056). I then asked the submitter to test it. She advised she was not going to. I gave a lot of consideration at the time to closing out the fix unmerged despite the sunk time, but then Monish stepped up & tested and merged it. Neither of us were familiar with the flow and it appears we both focussed on the Contribution Page flow not event page.
**Recommendations going forwards**
1. I think the change already made to not treating long-standing bugs as critical unless the reporter is engaged in the full process would have prevented this. (we would have closed the PR & tracked the issue in gitlab until such time as someone raised a review-ready PR).
1. We should consider changing our terminology from ‘pull request’ to ‘review request’ to make it clearer to people they are are requesting someone spend time on reviewing their ideas code or proposals.
1. We should put together more documentation explaining the ways in which we are working to improve our code base so we have something to point people to. (This is already a lower priority item on my very long to-do list).
1. We should find out who is familiar enough with using PCPs (and willing) to test future PCP related patches (and to validate bug reports)5.3.2