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

Limit number of characters one can type in PCP Personal Note #12044

Closed
wants to merge 1 commit into from
Closed

Limit number of characters one can type in PCP Personal Note #12044

wants to merge 1 commit into from

Conversation

KarinG
Copy link
Contributor

@KarinG KarinG commented Apr 29, 2018

Overview

Textarea must have a limit - as the database field is only a varchar(255)

Before

CiviCRM throws a fatal error
[nativecode=1406 ** Data too long for column 'pcp_personal_note' at row 1]
if donor types in more than 255 characters; donation processes but page throws a fatal error before receipt is dispatched - so donor does not now monies went through - tries again.

After

A very simple addition of maxlength attribute to textarea field prevents this; Note that this was already in place for pcp_roll_nickname (maxlength="30"); that is wasn't in place for pcp_personal_note is most likely a simple oversight;

Steps to reproduce

Set up an Event in CiviCRM (or use an existing one)

in Event Configuration -> under Personal Campaigns tab -> check box: Enable
Select a Supporter Profile (it's a required field)

under Online Registration tab -> enabled Online Registration
no need to send Emails

Create your Personal Campaign page:
civicrm/contribute/campaign?action=add&reset=1&pageId=[id_of_event_just_created]&component=event

on dmaster -> having enabled Personal Campaigns for Fall Fundraiser:
http://dmaster.demo.civicrm.org/civicrm/contribute/campaign?action=add&reset=1&pageId=1&component=event

Click through / fill out Title etc. -> enable checkbox -> Honor roll

To now test the page -> find the PCP page you just created under -> Events -> Personal Campaign Pages -> and Approve it (if it needs approval)

Click on Page title -> Contribute now

click checkbox Show my support in public honor roll
click on include my name and message
add lots of text to message - this will do it:

Lorem ipsum is a pseudo-Latin text used in web design, typography, layout, and printing in place of English to emphasise design elements over content. It's also called placeholder (or filler) text. It's a convenient tool for mock-ups. It helps to outline the visual elements of a document or presentation, eg typography, font, or layout. Lorem ipsum is mostly a part of a Latin text by the classical author and philosopher Cicero. Its words and letters have been changed by addition or removal, so to deliberately render its content nonsensical; it's not genuine, correct, or comprehensible Latin anymore. While lorem ipsum's still resembles classical Latin, it actually has no meaning whatsoever. As Cicero's text doesn't contain the letters K, W, or Z, alien to latin, these, and others are often inserted randomly to mimic the typographic appearence of European languages, as are digraphs not to be found in the original.

on dmaster: http://dmaster.demo.civicrm.org/civicrm/event/register?id=1&pcpId=2&reset=1

image

Results in:
image

Check ConfigAndLog -> for message
[nativecode=1406 ** Data too long for column 'pcp_personal_note' at row 1]

Comments

In production of live site;

as it's only a varchar(255) field in the database;
@eileenmcnaughton
Copy link
Contributor

There was a similar issue recently - #12014 @monishdeb & @seamuslee001 worked it through to a metadata based fix - is that possible here?

@KarinG
Copy link
Contributor Author

KarinG commented Apr 29, 2018

Sure but this is a small/safe fix consistent with the other maxlength attributes (in this PCP code) to address a critical issue: i) a fatal error, ii) that is thrown after monies are processed - so donor thinks his internet blew up and donates again. Org needs to refund monies. This PCP code/section could use more work - but let's address critical issues first and rapidly.

@eileenmcnaughton
Copy link
Contributor

@KarinG - yes the other was very similar - small fatal error - in that case it was one @monishdeb was fixing pro bono & by doing a little extra work he got a fix that was consistent with how we are doing things

@KarinG
Copy link
Contributor Author

KarinG commented Apr 29, 2018

If this does not meet standards feel free to close; But perhaps best not to suggest I don’t do/enough pro bono work for CiviCRM;

@eileenmcnaughton
Copy link
Contributor

@KarinG I don't think that was implied in my comments.

@monishdeb
Copy link
Member

monishdeb commented Apr 29, 2018

@KarinG here's my patch #12014 (comment) which shows that we pick the metdata of a column from respective xml file and addField() does the job quite nicely :)

In here we should use

$this->addField('pcp_personal_note', array('entity' => 'ContributionSoft', 'style' => 'height: 3em; width: 40em;'));

Also you need add HTML property to the xml file as

<field>
    <name>pcp_personal_note</name>
    <type>varchar</type>
    <title>Soft Contribution PCP Note</title>
    <length>255</length>
    <default>NULL</default>
    <add>2.2</add>
    <html>
        <type>TextArea</type>
    <html>
  </field>

and need to run cd <civicrm directory>bin; ./setup.sh -g to pick the properties to respective DAO file.

@monishdeb
Copy link
Member

Oh there's already a PR #12046 So now you need to just do the replacement with addField(). That's it

@eileenmcnaughton
Copy link
Contributor

@monishdeb I actually put up a PR which is approved for merge for the xml change - see above.

I'm not too sure steps to replicate this but if you are & can do so then I think it would be an extra 5 mins of work to submit a replacement PR & if you do that I can merge it

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Apr 29, 2018

@KarinG can you please update the PR with steps to replicate since that will be a requirement for any review (either of this PR or one where we use the line of code that aligns with the way we are trying to write our code)

@monishdeb
Copy link
Member

I have spent 10mins and wasn't able to get the PCP form that renders the pcp_personal_note field. Can you please mention the steps to replicate this issue? Thanks!

@KarinG
Copy link
Contributor Author

KarinG commented Apr 30, 2018

@monishdeb - see above;

@eileenmcnaughton
Copy link
Contributor

Thanks @KarinG I put the version of the code we suggested in an alternate PR - can you please review it. Since it required me to spend time in the code I also fixed the deprecation notices I hit in the process.

@KarinG
Copy link
Contributor Author

KarinG commented Apr 30, 2018

Thank you @Eileen

pcp_roll_nickname is ok with a maxlength attribute? That’s why I originally used it in the line below that;

@monishdeb
Copy link
Member

@KarinG @eileenmcnaughton I am closing this PR in favor of #12056 that include the fix. Also raised a issue with pcp_roll_nickname that needs the same treatment.

@monishdeb monishdeb closed this Apr 30, 2018
@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Apr 30, 2018

@KarinG generally when we touch something we try to ensure we are fixing it in accordance with our current preferred approach (often that is something we learn about from a reviewer in the review process) but it's not feasable to fix everything to be the preferred approach every time we touch the code so expanding to another field is def optional

Did you test the other PR?

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

Successfully merging this pull request may close these issues.

4 participants