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.

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

Fix Soft credit personal note ton limit to 255 characters (DB limit). #12056

Merged
merged 1 commit into from
May 2, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Fix Soft credit personal note ton limit to 255 characters (DB limit).

Alternate to #12044 - replication details are the same & max impact is the same. As I needed to go into the code to do the fix proposed on the other PR I also fixed all the deprecation notices I hit at the same time (although not the one e-notice at the very end)

@KarinG can you please test & review this - it solves the same problem as your PR but per comments on that PR it uses the approach that we suggested on your PR

@@ -453,7 +453,7 @@ public static function buildPcp($pcpId, &$page, &$elements = NULL) {
$page->_defaults['pcp_is_anonymous'] = 0;

$page->add('text', 'pcp_roll_nickname', ts('Name'), array('maxlength' => 30));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as @KarinG indicated, this also need to be changed to $page->addField('pcp_roll_nickname', ['entity' => 'ContributionSoft', 'context' => 'create']);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - I don't want to scope-creep this to other fields

@eileenmcnaughton
Copy link
Contributor Author

Note this requires @KarinG to confirm she has tested it

@KarinG
Copy link
Contributor

KarinG commented Apr 30, 2018

I'm sorry but I get cannot apply patch on this PR/8 files (and the prerequisite XML PR/1 file) to the 4.7.x site that I have PCP Events in production on - or I would roll this out to our sites right now. So all I could do is do exactly what you have already done - on a brand new vanilla civibuild 5.x - I don't need to replicate that; Time is very limited at the moment with CiviCamp Calgary only weeks away now.

@eileenmcnaughton
Copy link
Contributor Author

@KarinG - OK well we need someone to replicate before we can merge somewhere - that's our requirement. If you can't do it then it can wait until someone does want to put the time into doing that.

On a vanilla 5.x is fine but it does need to go through the review hoops

@eileenmcnaughton
Copy link
Contributor Author

@KarinG why don't you do it at the sprint - it's probably a twenty minute job to finish review at this point & the sprint might be a good time

Note that some special characters can still cause it to spill over but that will affect other fields too & we
should consider a bigger fix if we need to.

Also fixed deprecation notices
@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

(test fail unrelated but still better to get a green)

@monishdeb
Copy link
Member

I have tested the patch for PCP pcp_personal_note field and as now I can fill more then 255 characters. Also in addition the changes made to use CRM_Core_Pseudoconstant::key() instead of CRM_Core_OptionGroup::getValue doesn't result into any indented regression and successfully get rid of notices.

Hence merging this PR.

@monishdeb monishdeb merged commit 90db4f6 into civicrm:master May 2, 2018
@eileenmcnaughton eileenmcnaughton deleted the pcp branch May 2, 2018 10:35
@eileenmcnaughton
Copy link
Contributor Author

Thanks @monishdeb I know you are busy and this did not affect any of your customers but I appreciate getting it wrapped up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants