Page MenuHomePhabricator

Triage what to do with civi-core patches: extensions, upstream, or abandon
Closed, ResolvedPublic8 Story Points

Description

Go through all the patches here and decide what to do with them. It would be really nice to put notes from our investigation inside the diff header for each file.

There is a fourth alternative that we can fall back on if there is no other sane way forward for a patch, and it really does need to be special-cased in Civi core. We can add a hook at the point we need to do stuff, and call out to code kept in an extension or drupal module.

Decide which features should have automated tests.

Details

Related Gerrit Patches:
wikimedia/fundraising/crm : civi-4.6.9Fix e-strict errors
wikimedia/fundraising/crm/civicrm : civi-4.6.9CRM-17422 remove prefixing of wildcard to search.
wikimedia/fundraising/crm/civicrm : masterAnnotate CiviCRM patches
wikimedia/fundraising/crm/civicrm : civi-4.6.9CRM-17000 bypass separatevalues for getfields for performance
wikimedia/fundraising/crm : civi-4.6.9Civi46 set contact_default_language on upgrade
wikimedia/fundraising/crm : masterCivi46 set contact_default_language on upgrade
wikimedia/fundraising/crm : civi-4.6.9Civi46 adjust function to get tests passing
wikimedia/fundraising/crm : civi-4.6.9Civi46 update refund test for changed parameter (no 2)
wikimedia/fundraising/crm : civi-4.6.9Civi46 update CiviCRM submodule version
wikimedia/fundraising/crm : masterCivi46 update CiviCRM submodule version
wikimedia/fundraising/crm : masterCivi46 update refund test for changed parameter (no 2)
wikimedia/fundraising/crm : civi-4.6.9-deployCivi46 update refund test for changed parameter (no 2)
wikimedia/fundraising/crm : civi-4.6.9-deployCivi46: adapt to pre-hook standardisation (2)
wikimedia/fundraising/crm : civi-4.6.9-deployCivi46 adjust expected return array in testImport
wikimedia/fundraising/crm : masterExplain some disablement

Related Objects

StatusAssignedTask
OpenNone
Resolvedmepps
ResolvedNone
Resolvedawight
ResolvedEileenmcnaughton
ResolvedNone
ResolvedEileenmcnaughton
ResolvedNone
Resolvedawight
ResolvedDStrine
ResolvedEileenmcnaughton
ResolvedEileenmcnaughton
DeclinedEileenmcnaughton
Resolvedawight
Resolvedawight
Resolvedawight
Resolvedawight
OpenEileenmcnaughton
ResolvedNone
Resolvedawight

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 240925 abandoned by Eileen:
T99836 remove apiv2 Entity Tag call

Reason:
wrong branch

https://gerrit.wikimedia.org/r/240925

Change 235371 merged by Eileen:
Explain some disablement

https://gerrit.wikimedia.org/r/235371

Change 240927 abandoned by Eileen:
T99836 Civi46 update refund test for changed parameter

Reason:
wrong branch

https://gerrit.wikimedia.org/r/240927

Change 240926 abandoned by Eileen:
T99836 Civi46: adapt to pre-hook standardisation

Reason:
wrong branch

https://gerrit.wikimedia.org/r/240926

Eileenmcnaughton added a comment.EditedSep 25 2015, 5:05 AM

Status update on this :I have the tests passing against 4.6 locally except for one issue which I'll elaborate on further down.

I have pushed my working-version to sandbox/eileen/civi-4.6.9-deploy. Now that I somewhat have a handle on gerrit I need to get my ducks in a row before submitting the changes.

I'm not sure how to merge up from master to the feature branch - do I merge it & submit as git review - or is there another process? I think I also need to do things in this order

  1. DONE : Get my trailing white space changes merged into master https://gerrit.wikimedia.org/r/#/c/240959/
  2. Merge from master to the feature branch
  3. Check all the reverts are properly reverted :-)
  4. Get the submodule change merged https://gerrit.wikimedia.org/r/#/c/240953/
  5. Then I can update the submodule commit on the feature branch & bring across all the other commits required to get the tests working.

(most of this doesn't strictly relate to triaging the core patches as the test fixes are for the integration extensions.)

Test fix that isn't fixed:

In 4.6 CiviCRM api validates more fields. I have ensured the preferred_language field is adding the options in as required & updated the existing code for prefix_id & suffix_id, but from the tests it seems there is an expectation that the integration modules are 'allowed to' pass in invalid values for custom fields & if they are not valid they won't wind up on the mail outs. I probably need a bit more context to know where to go with this.

A couple of tests are affected but notably WmfCampaignTest::testMatchingDonation

something to check

In 4.6 CiviCRM expects the 'value' field for any suffix_id or prefix_id option values to be numeric. They may not be in production at the moment.

CiviCRM repo status

I've been focussed on the tests for the last couple of days but I believe it did successfully go onto staging. I haven't done any testing on the refund form so far but would be surprised if it works.

The Giant Rabbit port is still a work in progress although it's worth looking at them to see the button changes as I had to change them to make them more 4.6 friendly and I discovered the view charts actions weren't working (on 4.4 either) and that they don't quite work as 'actions' because it makes sense to combine 'print pdf' with 'view as pie chart'. There is some discussion of the reports over at the issues under https://issues.civicrm.org/jira/browse/CRM-17154 but probably the main thing to say right now is that it is not a completed port or the fixes yet.

Change 240959 merged by jenkins-bot:
T99836 remove trailing spaces

https://gerrit.wikimedia.org/r/240959

Change 241892 had a related patch set uploaded (by Eileen):
T99836 remove apiv2 Entity Tag call

https://gerrit.wikimedia.org/r/241892

Change 241893 had a related patch set uploaded (by Eileen):
T99836 Civi46: adapt to pre-hook standardisation

https://gerrit.wikimedia.org/r/241893

Change 241894 had a related patch set uploaded (by Eileen):
T99836 Civi46 update refund test for changed parameter

https://gerrit.wikimedia.org/r/241894

Change 241895 had a related patch set uploaded (by Eileen):
T99836 Civi46 adapt to DAO BAO rename to CRM_Mailing_DAO_MailingJob

https://gerrit.wikimedia.org/r/241895

Change 241892 merged by Awight:
T99836 remove apiv2 Entity Tag call

https://gerrit.wikimedia.org/r/241892

Change 241893 merged by Awight:
T99836 Civi46: adapt to pre-hook standardisation

https://gerrit.wikimedia.org/r/241893

Change 241894 merged by Awight:
T99836 Civi46 update refund test for changed parameter

https://gerrit.wikimedia.org/r/241894

Change 241895 merged by Awight:
T99836 Civi46 adapt to DAO BAO rename to CRM_Mailing_DAO_MailingJob

https://gerrit.wikimedia.org/r/241895

Change 240303 merged by Awight:
T99836 Civi46: remove calls to pesudoconstant:phoneType

https://gerrit.wikimedia.org/r/240303

Change 241912 had a related patch set uploaded (by Eileen):
T99836 Civi46: ensure that preferred language exists when creating a contact

https://gerrit.wikimedia.org/r/241912

Change 241913 had a related patch set uploaded (by Eileen):
T99836 Civi46 fix ensure_prefix & ensure_suffix for 4.6

https://gerrit.wikimedia.org/r/241913

Change 241914 had a related patch set uploaded (by Eileen):
T99836 fix dumb typo in test

https://gerrit.wikimedia.org/r/241914

Change 241943 had a related patch set uploaded (by Eileen):
T99836 Civi46: reapply commits reverted in master

https://gerrit.wikimedia.org/r/241943

Change 241943 abandoned by Eileen:
T99836 Civi46: reapply commits reverted in master

Reason:
arg wrong branch - I stashed my .review - not to self!

https://gerrit.wikimedia.org/r/241943

Change 241947 had a related patch set uploaded (by Eileen):
T99836 Civi46: reapply commits reverted in master

https://gerrit.wikimedia.org/r/241947

Change 241947 merged by Awight:
T99836 Civi46: reapply commits reverted in master

https://gerrit.wikimedia.org/r/241947

Change 241970 had a related patch set uploaded (by Eileen):
Civi46: adapt to pre-hook standardisation (2)

https://gerrit.wikimedia.org/r/241970

Change 241970 merged by Awight:
Civi46: adapt to pre-hook standardisation (2)

https://gerrit.wikimedia.org/r/241970

Change 241912 merged by Awight:
T99836 Civi46: ensure that preferred language exists when creating a contact

https://gerrit.wikimedia.org/r/241912

Change 241914 merged by Awight:
T99836 fix dumb typo in test

https://gerrit.wikimedia.org/r/241914

Change 241913 merged by Awight:
T99836 Civi46 fix ensure_prefix & ensure_suffix for 4.6

https://gerrit.wikimedia.org/r/241913

@eileen - thanks for this! I'm happy to help talking with Giant Rabbit if
that's helpful. Just let me know and we can figure something out.

@atgo - The issue with the graph buttons is that they assume that because a report class supports graphs the report instance will - but actually the report instance calculates whether it can at run time - basically depending on whether group-by is in use. A second complication is that the graph buttons could be combined with another action (print barchart for example). For now I've taken them out of the actions & put them back separate.

The UI has changed quite a bit in 4.6 so it will be good to look at them with the 4.6 UI in front of you. Having said that - the version of the report interface that I'm working on isn't on staging just yet.

Change 243866 had a related patch set uploaded (by Eileen):
Civi46 set contact_default_language on upgrade

https://gerrit.wikimedia.org/r/243866

Change 243866 abandoned by Eileen:
Civi46 set contact_default_language on upgrade

Reason:
wrong branch - this is the repo I need to edit .gitreview on!

https://gerrit.wikimedia.org/r/243866

Change 243874 had a related patch set uploaded (by Eileen):
Civi46 set contact_default_language on upgrade

https://gerrit.wikimedia.org/r/243874

Once the set of patches currently queued for review is merged and pulled onto staging we need to run the drupal upgrade script and set the 'view report sql' permission for the role 'CiviCRM Admin'

From there I think the focus can move to

  • testing & review of the reports
  • figuring out the deployment repo

Testing focus areas:

  1. Does the ported refund patch work?
  2. Are the exceptions working - because that didn't really port
  3. I tried running some reports & got time outs - I believe that happens on live too - but is it the same degree of evil? I caught a nasty query off one of them & it could obviously stand some optimisation but I should probably identify & focus on the most important reports first.

I should perhaps open a Phab to cover the deployment repo?

Thanks Eileen. Glad to see you're running into the report time outs, since
that's something we can definitely use help with :)

A problem replicated is a problem halved ... isn't that the old proverb?

I've got the patch set I wanted onto staging now - so focus should be testing & review now

(some patches are still in the review queue but they don't have a major impact)

Change 243874 merged by jenkins-bot:
Civi46 set contact_default_language on upgrade

https://gerrit.wikimedia.org/r/243874

Ejegg moved this task from Doing to Review on the Fundraising Sprint UB40 board.Oct 14 2015, 8:33 PM

Change 235359 merged by jenkins-bot:
Annotate CiviCRM patches

https://gerrit.wikimedia.org/r/235359

Change 247462 had a related patch set uploaded (by Eileen):
Fix e-strict errors

https://gerrit.wikimedia.org/r/247462

Change 247748 had a related patch set uploaded (by Eileen):
CRM-17422 remove prefixing of wildcard to search.

https://gerrit.wikimedia.org/r/247748

Change 247748 merged by jenkins-bot:
CRM-17422 remove prefixing of wildcard to search.

https://gerrit.wikimedia.org/r/247748

Change 247462 merged by jenkins-bot:
Fix e-strict errors

https://gerrit.wikimedia.org/r/247462

Change 248268 had a related patch set uploaded (by Eileen):
CRM-17422 remove prefixing of wildcard to search.

https://gerrit.wikimedia.org/r/248268

Change 248268 merged by Awight:
CRM-17422 remove prefixing of wildcard to search.

https://gerrit.wikimedia.org/r/248268

Eileenmcnaughton closed this task as Resolved.Nov 6 2015, 6:54 PM

Change 252477 had a related patch set uploaded (by Eileen):
CRM-17422 remove prefixing of wildcard to search.

https://gerrit.wikimedia.org/r/252477

Change 252477 merged by Eileen:
CRM-17422 remove prefixing of wildcard to search.

https://gerrit.wikimedia.org/r/252477

mmodell removed a subscriber: awight.Jun 22 2017, 9:38 PM