CRM-21693 show Display Name in online pay now UI #11571

magnolia61 · 2018-01-22T19:39:02Z

Overview

Show the Display Name in the Online Pay Now UI
JIRA: https://issues.civicrm.org/jira/browse/CRM-21693

Before

Using the special constructed PayNow link it was not clear for which person the payment was for. This, In our case, especially confused parents who needed to make a payment and had been mailed several payment links.

After

The Online Pay Now form is still extremely basic but shows the Display Name that belongs to the person the contribution belongs to.

Technical Details

Just inserted the display name in case !empty($ccid)

Comments

Currently the price set of the contribution is shown. All other fields (name, email) are hidden.
Ideally the form UI should also show the 'reason' for the contribution.
For an event that would be the event name, for a membership the membership type.
This gives the person who is about to pay the confidence that all details of the payment are correct which means he or she is more likely to proceed.

eileenmcnaughton · 2018-01-22T19:51:16Z

Makes sense to me - any chance you can squash the 2 commits into one (git rebase -i origin/master) & add the JIRA issue to the commit message

@jitendrapurohit @agh1 @JoeMurray is there any downside to this? Could it 'leak data' or is that protected by a checksum?

magnolia61 · 2018-01-22T20:14:31Z

@eileenmcnaughton I never did what you suppose before (the squash/rebase thing).
Going to read into it, but takes time probably. Want to learn though ;-)

eileenmcnaughton · 2018-01-22T20:30:32Z

@magnolia61 try git pull --rebase upstream master first (to make sure it's clean before you start)

agh1 · 2018-01-22T20:49:37Z

So I don't know much about the pay now feature, but this has one of two problems:

If the $display_name is assigned to the template when an anonymous visitor arrives at the form with a ccid set, you could snoop on everyone's invoice totals by going to http://mysite/civicrm/contribute/transact?&reset=1&id=1&ccid=1, ...&ccid=2, etc.
However, it doesn't look like that given this line in CRM/Contribute/Form/Contribution/Main.php is where the display name is assigned and it's only filled if there's a contact ID set. Instead, the $display_name value is going to be undefined and the page will have the header "Contribution Information - ".

agh1 · 2018-01-22T20:52:12Z

templates/CRM/Contribute/Form/Contribution/Main.tpl

@@ -91,7 +91,7 @@
  {elseif !empty($ccid)}
    {if $lineItem && $priceSetID && !$is_quick_config}
      <div class="header-dark">
-        {ts}Contribution Information{/ts}
+        {ts}Contribution Information{/ts}  - {ts 1=$display_name}%1{/ts}


It appears that you have two spaces ahead of the hyphen--there should be just one. Also, shouldn't this be an en dash (–)?

eileenmcnaughton · 2018-01-22T21:51:23Z

hmm - it's still got tonnes in - you need to do a clean rebase over the upstream master branch first & push that up with

git push -f yourrepo yourbranch

magnolia61 · 2018-01-22T22:04:59Z

trying and messing with my local git, git pull --rebase upstream master gives some errors.
hang on, i will figure it out ;)

magnolia61 · 2018-01-22T23:02:37Z

ok, learned some more on all kind of git commands. Think the PR is ok now ;-)

eileenmcnaughton · 2018-01-22T23:07:41Z

well done!

Did you have any thoughts on #11571 (comment)

agh1 · 2018-01-22T23:19:48Z

templates/CRM/Contribute/Form/Contribution/Main.tpl

@@ -91,7 +91,7 @@
  {elseif !empty($ccid)}
    {if $lineItem && $priceSetID && !$is_quick_config}
      <div class="header-dark">
-        {ts}Contribution Information{/ts}
+        {ts}Contribution Information{/ts} &ndash; {ts 1=$display_name}%1{/ts}


Sorry, I should've noticed this earlier: your {ts} isn't wrapping any string to translate--just a variable to drop in as-is. You could either:

get rid of the ts and just drop it in as

{ts}Contribution Information{/ts} – {$display_name}

or wrap the whole thing in a ts like

{ts 1=$display_name}Contribution Information – %1{/ts}

The latter causes more work for our translators (since it introduces a new string to translate), but it allows for localization of how the header is ordered and punctuated. I don't know which is preferred as a general rule.

(and this may depend upon how you address the other concern about whether the $display_name is populated)

magnolia61 · 2018-01-22T23:21:48Z

@agh1 The Pay Now form can only be accessed by a special link which includes the checksum, contribution id and (optionally) cid. It cannot be accessed anonymously. Aside of that, for the purpose of this PR I haven't analysed the underlying mechanisms and checks on preventing disclosures I must admit. This PR only focuses on adding the display name ;-)

agh1 · 2018-01-22T23:30:05Z

@magnolia61 glad to know you can only get there authenticated. The code on the online contribution forms is so jam-packed with all the tangentially-related available features that it wasn't really clear what happens when ccid is present but there was not authentication or checksum.

Thanks for your improvement!

magnolia61 · 2018-01-22T23:37:51Z

@agh1 ;-( unfortunately I just figured out the constructed link can also be used anonymously. In that case the Display Name is not retrieved. Also when both the checksum and the contact id are being omitted, the contribution strangly moves to user 1, which I think should not happen and is a bug.
All of this is related to the Online Pay Now functionality but is not part of this issue/PR. Maybe we should put this PR on hold, and further discuss in another jira issue? Actually I reported this anomaly quite some time ago in: https://issues.civicrm.org/jira/browse/CRM-20697

eileenmcnaughton · 2018-01-22T23:39:28Z

@magnolia61 is user 1 your logged in user? or are you testing logged out?

magnolia61 · 2018-01-22T23:40:46Z

No, this happens when I am not logged in, eg. when I use the PayNow link in a new private window. When using the PayNow button when logged in (from the contact dashboard) only the contribution id and contribution page are in the url. I think the corresponding contact ID and checksum should also be in that url and be required. Maybe even use a uniform url that also includes the checksum when logged in. So enforce the url to look like: https://www.ourdomain.nl/civicrm/contribute/transact?reset=1&id=7&ccid=3887&cs=986e9c03ddf2b4b980a929a_1492524606_1440&cid=316
Including: contribution Page (id), contribution id (ccid), checksum (cs) and Contact ID (cid).
Only if ccid, cs & cid match the page should be shown as a safeguard probably

PS. moving this info to the related issue so the discussion is in the right place

BTW. being able to mail the Pay Now link to users that still have to pay something is great, lots of our participants pay this way. I even think it would be awesome to include this next to the email invoice button, or include the PayNow link in that email)

agh1 · 2018-01-23T00:09:26Z

@magnolia61 well, the way I see it, your change is neither causing the bug, exacerbating the bug, nor exposing private information due to the bug, so I don't see any reason to hold this change up just because it uncovered the other issue.

eileenmcnaughton · 2018-01-23T00:35:51Z

I guess arguably it is valid to send a pay now link to someone (e,g your mum) to make a payment for you & for (not easy to come up with reasons in this scenario) your mum is not you so should not see your name.

magnolia61 · 2018-01-23T09:36:55Z

BTW. shall I remove the dash so it will look whether the displayname is shown or not?

agh1 · 2018-01-23T15:14:25Z

@magnolia61 I think it would be best to wrap the display name and dash in a conditional, e.g.

{ts}Contribution Information{/ts}{if $display_name} &ndash; {$display_name}{/if}

magnolia61 · 2018-01-23T15:39:27Z

I changed it to be conditional but when I think about it, I think the changed Jitendra made to solve the other issues with the PayNow link, assure the contact id is always set and thus the display name is always known. What do you think? #11578

agh1 · 2018-01-23T15:48:00Z

It doesn't hurt to have the condition here, especially since you have ambitions to make it work properly for anonymous visitors later.

magnolia61 · 2018-02-26T10:46:48Z

ok to merge?

colemanw · 2018-02-26T22:43:56Z

I'm happy with the review this has gotten. Yes.

magnolia61 · 2018-04-11T11:31:17Z

I think we should change this PR. It works ok in almost all scenarios, except when an admin accesses the Pay Now payment screen via the user dashboard. I think we should change this so that the displayed name is that of the owner of the contribution and not the logged in user. Now sure if that would be simple though.

civicrm-builder added the master label Jan 22, 2018

magnolia61 changed the title ~~Crm 21693 show name in online pay now ui~~ Crm 21693 show name in online pay now UI Jan 22, 2018

magnolia61 changed the title ~~Crm 21693 show name in online pay now UI~~ Crm 21693 show Display Name in online pay now UI Jan 22, 2018

agh1 reviewed Jan 22, 2018

View reviewed changes

magnolia61 force-pushed the CRM-21693_Show_Name_in_OnlinePayNow_UI branch 4 times, most recently from 7354847 to d856052 Compare January 22, 2018 23:00

agh1 reviewed Jan 22, 2018

View reviewed changes

magnolia61 force-pushed the CRM-21693_Show_Name_in_OnlinePayNow_UI branch from d856052 to c3e97ca Compare January 22, 2018 23:27

magnolia61 force-pushed the CRM-21693_Show_Name_in_OnlinePayNow_UI branch from c3e97ca to 1bb88ea Compare January 22, 2018 23:30

magnolia61 force-pushed the CRM-21693_Show_Name_in_OnlinePayNow_UI branch 2 times, most recently from f6e72b7 to 1bb88ea Compare January 23, 2018 15:32

CRM-21693: Show Display Name in PayNow screen

3265f68

magnolia61 force-pushed the CRM-21693_Show_Name_in_OnlinePayNow_UI branch from 1bb88ea to 3265f68 Compare January 23, 2018 15:35

colemanw merged commit 29791fd into civicrm:master Feb 26, 2018

mlutfy changed the title ~~Crm 21693 show Display Name in online pay now UI~~ CRM-21693 show Display Name in online pay now UI Mar 6, 2018

mlutfy added this to the 4.7.32 milestone Mar 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-21693 show Display Name in online pay now UI #11571

CRM-21693 show Display Name in online pay now UI #11571

magnolia61 commented Jan 22, 2018 •

edited

Loading

eileenmcnaughton commented Jan 22, 2018

magnolia61 commented Jan 22, 2018

eileenmcnaughton commented Jan 22, 2018

agh1 commented Jan 22, 2018 •

edited

Loading

agh1 Jan 22, 2018

eileenmcnaughton commented Jan 22, 2018

magnolia61 commented Jan 22, 2018

magnolia61 commented Jan 22, 2018

eileenmcnaughton commented Jan 22, 2018

agh1 Jan 22, 2018

agh1 Jan 22, 2018

magnolia61 commented Jan 22, 2018

agh1 commented Jan 22, 2018

magnolia61 commented Jan 22, 2018 •

edited

Loading

eileenmcnaughton commented Jan 22, 2018

magnolia61 commented Jan 22, 2018 •

edited

Loading

agh1 commented Jan 23, 2018

eileenmcnaughton commented Jan 23, 2018

magnolia61 commented Jan 23, 2018

agh1 commented Jan 23, 2018

magnolia61 commented Jan 23, 2018

agh1 commented Jan 23, 2018

magnolia61 commented Feb 26, 2018

colemanw commented Feb 26, 2018

magnolia61 commented Apr 11, 2018

CRM-21693 show Display Name in online pay now UI #11571

CRM-21693 show Display Name in online pay now UI #11571

Conversation

magnolia61 commented Jan 22, 2018 • edited Loading

Overview

Before

After

Technical Details

Comments

eileenmcnaughton commented Jan 22, 2018

magnolia61 commented Jan 22, 2018

eileenmcnaughton commented Jan 22, 2018

agh1 commented Jan 22, 2018 • edited Loading

agh1 Jan 22, 2018

Choose a reason for hiding this comment

eileenmcnaughton commented Jan 22, 2018

magnolia61 commented Jan 22, 2018

magnolia61 commented Jan 22, 2018

eileenmcnaughton commented Jan 22, 2018

agh1 Jan 22, 2018

Choose a reason for hiding this comment

agh1 Jan 22, 2018

Choose a reason for hiding this comment

magnolia61 commented Jan 22, 2018

agh1 commented Jan 22, 2018

magnolia61 commented Jan 22, 2018 • edited Loading

eileenmcnaughton commented Jan 22, 2018

magnolia61 commented Jan 22, 2018 • edited Loading

agh1 commented Jan 23, 2018

eileenmcnaughton commented Jan 23, 2018

magnolia61 commented Jan 23, 2018

agh1 commented Jan 23, 2018

magnolia61 commented Jan 23, 2018

agh1 commented Jan 23, 2018

magnolia61 commented Feb 26, 2018

colemanw commented Feb 26, 2018

magnolia61 commented Apr 11, 2018

magnolia61 commented Jan 22, 2018 •

edited

Loading

agh1 commented Jan 22, 2018 •

edited

Loading

magnolia61 commented Jan 22, 2018 •

edited

Loading

magnolia61 commented Jan 22, 2018 •

edited

Loading