Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CRM-18713 complete #8628

Closed
wants to merge 5 commits into from
Closed

CRM-18713 complete #8628

wants to merge 5 commits into from

Conversation

jmcclelland
Copy link
Contributor

@jmcclelland jmcclelland commented Jun 27, 2016 •
edited by civicrm-builder
Loading

Alternate, more comprehensive fix to CRM-18713. This is an alternative PR to #8626.

@jmcclelland
CRM-18713 - simple fix, ensure mobile # is not overwritten by primary.
c965c60
@jmcclelland
CRM-18713 - $tokens is not yet defined
fdce7bb 
No point in including it in this if statement.
@jmcclelland
CRM-18713 - remove redundant and confusing code
2df4d61 
This is a more comprehensive fix to CRM-18713 that attempts to remove
some code that was clearly copied/pasted from the email code.
@jmcclelland
CRM-18713 - remove useless code, ensure custom tokens work
ff492c9 
We need tokens to be properly replace with contact tokens or any other
kind of custom token and we shouldn't run any of the token code
if no tokens are present in the message. Also, removed non-working
code that tried to determine if noSMS is selected.
@jmcclelland jmcclelland mentioned this pull request Jun 27, 2016
Merged
@eileenmcnaughton eileenmcnaughton changed the title Crm 18713 complete CRM-18713 complete Jun 28, 2016
@eileenmcnaughton
Copy link
Contributor

wanna tackle the style warning fails?

@jmcclelland
Copy link
Contributor Author

Yup - thanks for the heads up. white space fixes committed.

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@jmcclelland
Copy link
Contributor Author

I don't understand jenkins error. The error link (https://test.civicrm.org/job/CiviCRM-Core-PR/10317/checkstyleResult/error/) takes me to a page that says:

No report files were found. Configuration error?

What does that mean?

@eileenmcnaughton
Copy link
Contributor

Click on console & you get to

https://test.civicrm.org/job/CiviCRM-Core-PR/10317/console

& in there

In "sites/all/modules/civicrm/", apply "https://github.com/civicrm/civicrm-core/pull/8628" on top of "0b0c49f1a3b3e58b4dbacc17f8228b63eadccd68".


  [GitScan\Exception\ProcessErrorException]                                          
  Process failed:                                                                    
  [[ COMMAND: git apply --check  ]]                                                  
  [[ CWD: /home/jenkins/buildkit/build/core-8628-34tem/sites/all/modules/civicrm ]]  
  [[ EXIT CODE: 1 ]]                                                                 
  [[ STDOUT ]]                                                                       
  [[ STDERR ]]                                                                       
  error: patch failed: CRM/Activity/BAO/Activity.php:1447                            
  error: CRM/Activity/BAO/Activity.php: patch does not apply                         



automerge [-R|--rebuild] [-K|--keep] [-N|--new] [--path PATH] [--url-split URL-SPLIT] [--passthru PASSTHRU] [--] [<url>]...

@JoeMurray JoeMurray mentioned this pull request Jul 7, 2016
Closed
@colemanw
Copy link
Member

@civicrm-builder retest this please

@davejenx davejenx mentioned this pull request Oct 12, 2016
Closed
@davejenx
Copy link
Contributor

@jmcclelland You say this is an alternative fix to #8626 which has now been merged, so is this PR still relevant?

@jmcclelland
Copy link
Contributor Author

I think it is up to core team. This pull requests does important re-factoring and code cleanup so it is still relevant. However, it might not be worth the time needed to QA it. See https://issues.civicrm.org/jira/browse/CRM-18713 for a long discussion about the reasons for this PR.

@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor

I'm going to close this - after this long your comment about time to QA it sounds on the money - I suspect at this point it's easier to start from scratch to refactor this as other changes will have happened

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jmcclelland @eileenmcnaughton @seamuslee001 @colemanw @davejenx @civicrm-builder