Product-maintenance issueshttps://lab.civicrm.org/eileen/Product-maintenance/-/issues2018-12-29T22:17:18Zhttps://lab.civicrm.org/eileen/Product-maintenance/-/issues/21Regression Analysis - 5.8 WYSIWYG editor settings broke following refactorin...2018-12-29T22:17:18ZseamusleeRegression Analysis - 5.8 WYSIWYG editor settings broke following refactoring (Fixed in 5.9)**What happened**
During the 5.8 cycle, a number of preferences screens were refactored to work with the standard settings approach which is a very lightweight form with pretty much all elements being driven from settings metadata. One o...**What happened**
During the 5.8 cycle, a number of preferences screens were refactored to work with the standard settings approach which is a very lightweight form with pretty much all elements being driven from settings metadata. One of these settings that were changed was the WYSIWYG editor setting. There was a change in the way that the options for the select list was generated. This meant that the key was changed from the machine name of the option to the value. This had consequences in regards to javascript that runs and having the system detecting what WYSIWYG to load.
**How & When was it addressed**
This was identified and logged in the Lab as an issue which was shown up in Joomla most prominently. It was addressed within two weeks of first being reported, which given the holiday season is probably ok. It was addressed by modifying the settings' usage of option group pseduoconstant to allow for an optional param keyColumn in the same way the DAOs do which then allowed for the switching back to being keyed based on the machine name and not the value.
**How did the regression fit with our processes?**
The PR had been reviewed but there was a number of settings that had been migrated within the cycle and potentially not every single one was checked thoroughly.
**Recommendations going forwards**
In this case a more thorougher code review may have picked up the differences, however given the number of settings changes and the likelihood the reviewer was mostly looking for front end changes rather than doing a full r-run it is likely this was just missed.https://lab.civicrm.org/eileen/Product-maintenance/-/issues/19Regression analysis - 5.4 (fixed 5.5.2) CiviCase bug on new installs2018-09-22T23:21:28ZeileenRegression analysis - 5.4 (fixed 5.5.2) CiviCase bug on new installs**What happened **
An enhancement was made to CiviCase that required a new option value. The option value was added in the upgrade script but was missed for new installs.
**How & When was it addressed**
It was identified in stack excha...**What happened **
An enhancement was made to CiviCase that required a new option value. The option value was added in the upgrade script but was missed for new installs.
**How & When was it addressed**
It was identified in stack exchange but as it only affected new installs it was not immediately understood as an issue. It was fixed quickly once confirmed as an issue.
**How did the regression fit with our processes?**
The PR had been reviewed but this was missed
**Recommendations going forwards**
I think in this case we need to learn from the specific nature of the issue.https://lab.civicrm.org/eileen/Product-maintenance/-/issues/18Regression analysis - 5.5 (fixed 5.5.1) broken smart group on permissioned re...2018-09-22T23:17:49ZeileenRegression analysis - 5.5 (fixed 5.5.1) broken smart group on permissioned relationship**What happened**
An enhancement was made to CiviCRM to allow relationship permissions to be no/read only/edit instead of just yes/no. This was fixed in the search form but resulted in a breakage in pre-existing smart groups.
**How & Wh...**What happened**
An enhancement was made to CiviCRM to allow relationship permissions to be no/read only/edit instead of just yes/no. This was fixed in the search form but resulted in a breakage in pre-existing smart groups.
**How & When was it addressed**
It was found immediately after 5.5.0 was released (by Noah Miller) and fixed within 24 hours.
**How did the regression fit with our processes?**
The PR had been reviewed by multiple reviewers but this was not something someone thought to test
**Recommendations going forwards**
I think in this case we need to learn from the specific nature of the issue - ie. changes to search forms can affect smart groups and ensure developers are aware of this instance and bear in in mind
https://lab.civicrm.org/eileen/Product-maintenance/-/issues/16Regression Analysis from 5.2, Sending SMSes from the contact actions failed t...2018-08-28T23:41:29ZseamusleeRegression Analysis from 5.2, Sending SMSes from the contact actions failed to send**What happened?**
A refacot of the SMS Sending code [PR](https://github.com/civicrm/civicrm-core/pull/10946) [CRM-21037](https://issues.civicrm.org/jira/browse/CRM-21037) cased SMSes to not be able to send in single one off when the se...**What happened?**
A refacot of the SMS Sending code [PR](https://github.com/civicrm/civicrm-core/pull/10946) [CRM-21037](https://issues.civicrm.org/jira/browse/CRM-21037) cased SMSes to not be able to send in single one off when the send an SMS action was selected when viewing a contact record.
**How & When was it addressed**
The bug was introduced when a variable in this case `$doNotSms` was changed from an empty string to be TRUE. The variable was used in an if statement and previously an empty string was not resolving to be TRUE. However other uses of that variable before the if suggested it was meant to be a boolean variable. It was [identified](https://lab.civicrm.org/dev/core/issues/273) approximately around the time 5.3.1 was released, even tho the bug was introduced into 5.2.0. It will be addressed in either 5.5 or 5.4.1
**How did the regression fit with our processes?**
It fitted into our workflows because this was some code cleanup done in an effort to add unit tests to better guard against such regressions occurring in our system.
**Recommendations going forwards**
We need to be more careful when handing refactor, Refactor is the one time when its easy to miss something or not fully understand all the implications of a change. In this case it was making the variable into what it seemed to be a boolean that then caused issue. It would also be noted that when doing refactors if the code becomes testable, tests should be added to cover a majority of scenarios. In this case there were two missing scenarios which included when a phone_type_id is passed in as part of the contact details which is what happens when you do a single SMShttps://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.2https://lab.civicrm.org/eileen/Product-maintenance/-/issues/12Regression analysis 5.2.0 (fixed 5.2.2)2018-07-26T00:09:34ZeileenRegression analysis 5.2.0 (fixed 5.2.2)**What happened?**
A [pro-bono bug fix](https://lab.civicrm.org/dev/core/issues/60) to help Webform users add payments to events resulted in part of the back-office event registration screen not loading for Wordpress users.
**How & Wh...**What happened?**
A [pro-bono bug fix](https://lab.civicrm.org/dev/core/issues/60) to help Webform users add payments to events resulted in part of the back-office event registration screen not loading for Wordpress users.
**How & When was it addressed**
Patch release 5.2.2 released within 48 hours on the bug being logged in gitlab. Pradeep gets credit for analysing the bug and submitting the fix after doing some pretty careful analysis of a confusing stack exchange report. Kevin tested the fix for us (very promptly)
**How did the regression fit with our processes?**
The original fix was tested by multiple people and it appeared to work. All those people were on drupal but there was nothing about the fix that implied there would be any difference for drupal vs another CMS. However, it turned out that drupal was in some way [‘swallowing’ the errors’](https://github.com/civicrm/civicrm-core/pull/12334).
The bug was fixed within 48 hours of being logged in github. It was asked [first in stack exchange](https://civicrm.stackexchange.com/questions/25394/event-fees-not-loading-when-registering-in-back-end) and Pradeep picked it up and submitted a fix. There was an earlier confusing question [logged in stackexchange](https://civicrm.stackexchange.com/questions/25376/cant-submit-credit-card-payment-after-update-to-5-2-1-joomla) which was probably the same bug.
**Context**
This bug was registered first on stack exchange - but there is considerable confusion (still) on how tightly [the stack exchange issue](https://civicrm.stackexchange.com/questions/25376/cant-submit-credit-card-payment-after-update-to-5-2-1-joomla) maps to the bug. A lot of credit is due to Pradeep for determining that there was a bug in the description there and figuring out how to replicate it. I looked briefly at that issue as it had 5.2.1 in the description but did not figure out that there was a replicable bug in there.
**Recommendations going forwards**
1. Figure out how to make our buildkit drupal environments less forgiving. The goal should be that dev environments are the least forgiving environments and production the most and this analysis identifies an instance where that was not the case
2. Think about how we can more quickly identify stack exchange issues. In this case it was Pradeep’s effort that got us there. (We can’t realistically stretch the existing product maintenance team to spend more time on Stack Exchange).
**Update**
Buildkit now has stricter error checking for drupal by default.https://lab.civicrm.org/eileen/Product-maintenance/-/issues/11Regression analysis 5.2.0 (fixed 5.2.1)2018-07-26T00:09:25ZeileenRegression analysis 5.2.0 (fixed 5.2.1)**What happened?**
A patch to make Contribution Detail report compliant with FULL_GROUP_BY mysql mode causes the display of one field (soft_credits) to break.
https://lab.civicrm.org/dev/core/issues/170
**How & When was it addressed**
...**What happened?**
A patch to make Contribution Detail report compliant with FULL_GROUP_BY mysql mode causes the display of one field (soft_credits) to break.
https://lab.civicrm.org/dev/core/issues/170
**How & When was it addressed**
Patch release 5.2.1 released within 48 hours.
**How did the regression fit with our processes?**
The issue was handled in conjunction with our processes at the time and it’s not obvious that someone should have thought to test that field. However, shortly after that fix was made we stopped changing working queries to comply with full group by mode (due to a history of regressions) and started to by-pass full group by where it was known not to work. This & last month’s issue should be the tail end of the FULL_GROUP_BY issues.
This report has known testability problems and we started to address that last month & with the fix for this regression we have a path to ensure any further fixes on this (known to be problematic) report get tests.
**Context**
Unit testing on reports is relatively new and coverage is relatively poor and for some reports issues with how they are constructed has been a blocker. This report is notable on that front and it would have to be described as fairly toxic. In this context I am aiming for a fix that is well tested and solves the unit testing issue going into master. https://github.com/civicrm/civicrm-core/pull/12281
**Recommendations going forwards**
Realistically the steps we have already taken - ie. getting this known-to-be-an-issue report under tests and be less aggressive on full group by fixes.https://lab.civicrm.org/eileen/Product-maintenance/-/issues/10Regression analysis - 5.1.0 (fixed 5.1.1)2018-07-26T00:09:18ZeileenRegression analysis - 5.1.0 (fixed 5.1.1)**What happened?**
A patch release was created for 5.1 (5.1.1) that included a fix for a regression that occurred relating to FULL_GROUP_BY mode for mysql. The fix did not include a pre-requisite patch that was already in the rc and in ...**What happened?**
A patch release was created for 5.1 (5.1.1) that included a fix for a regression that occurred relating to FULL_GROUP_BY mode for mysql. The fix did not include a pre-requisite patch that was already in the rc and in master, and hence it caused a more serious, wide-ranging regression
**How & When was it addressed**
The new regression was fixed & a new patch release was issued within 24 hours
**How did the regression fit with our processes?**
Our processes say that we should issue patch releases to address any regressions that have occurred in the current release (so that we are raising the bar each release). Technically the regression we were trying to fix was actually from the prior release (5.0) and would not meet that bar. The regression was tested by the submitter but there was confusion about the combination of patches tested, ultimately leading to the belief that the one released had been confirmed when it hadn’t (https://lab.civicrm.org/dev/core/issues/106#note_4507)
**Context**
This happened on the second release on the 5.x series. We are definitely still bedding in processes for the new patch release system and communications / timeframes & expectations are still in flux. We have a bottleneck in Tim for the actual releasing of them & it’s hard to know when he will allocate time for what could be described as a ‘regular patch release’.
We put out a patch for a fix that was not actually a 5.1 regression in part because there is a specific intent to get the ‘current 5.x version’ to being really solid & hence we have been very sensitive to regressions, which might have made us jump the gun. We had also had a user complain fairly vigorously that there was a bug fixed in 5.1 that was not released as a patch release on 5.0.
The fix itself was a FULL_GROUP_BY fix (support for recent MySQL & MariaDB releases) - which is an area in which we have had a lot of whack-a-mole and we feel we have a viable approach to this technical problem now (of which this patch is part). Basically the plan is
We have enabled FULL_GROUP_BY mode on the CI box
We are now disabling FULL_GROUP_BY mode for specific problematic queries as a first line of defence
We are trying to fix the queries to comply with the FULL_GROUP_BY standard only in the context of writing unit tests & are trying not to do further fixes on the QUERY builder for them due to it’s complexity & brittleness.
**Recommendations going forwards**
The whole patch release process is in a lot of flux so it’s expected we will have some teething issues. Developing a clearer process / set of expectations is the next step.5.1.1