Regression analysis 5.2.0 (fixed 5.3.2) Affects front event event registrations with personal campaign pages configured.
A bug fix submitted by reviewers to replace a patch submitted that did not meet our coding expectations 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 (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, 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 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. 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
- 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).
- 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.
- 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).
- We should find out who is familiar enough with using PCPs (and willing) to test future PCP related patches (and to validate bug reports)