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-19549 : New pledge data entry page shows hard coded $ for installments #9475

Merged
merged 6 commits into from
Dec 13, 2016
Merged

CRM-19549 : New pledge data entry page shows hard coded $ for installments #9475

merged 6 commits into from
Dec 13, 2016

Conversation

jitendrapurohit
Copy link
Contributor

@jitendrapurohit jitendrapurohit commented Dec 1, 2016 •
edited by civicrm-builder
Loading

sharique and others added 4 commits November 30, 2016 16:48
@sharique
CRM-19549 : New pledge data entry page shows hard coded $ for install…
8dd8e40 
…ments

----------------------------------------
* CRM-19549: New pledge data entry page shows hard coded $ for installments
  https://issues.civicrm.org/jira/browse/CRM-19549
@sharique
CRM-19549 : added back missing text field, using currency symbol only…
28461aa 
… if any

----------------------------------------
* CRM-19549: New pledge data entry page shows hard coded $ for installments
  https://issues.civicrm.org/jira/browse/CRM-19549
@sharique
CRM-19549 : code formatting fixes
f8eabee 
----------------------------------------
* CRM-19549: New pledge data entry page shows hard coded $ for installments
  https://issues.civicrm.org/jira/browse/CRM-19549
additional fixes
1fb7bdc
@jitendrapurohit jitendrapurohit mentioned this pull request Dec 1, 2016
Closed
@jitendrapurohit
Copy link
Contributor Author

jenkin, retest this please

pradpnayak
pradpnayak reviewed Dec 5, 2016
View reviewed changes
templates/CRM/Pledge/Form/Pledge.tpl
}
cj('.currency-symbol').text(val).append(" ").append(eachPaymentAmout);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this is an outdated pattern. We no longer use cj.
The way to update this code would be to wrap the whole thing in CRM.$(function($) { ... }); and replace all cj with $.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR to replace the outdated cj and also fixed validation error mentioned in point 1. @pradpnayak Can you please take a look ?

@pradpnayak
Copy link
Contributor

pradpnayak commented Dec 5, 2016 •
edited
Loading

I tested above patch. The Currency symbol gets changed when Currency is changed from currency option field. However there are still some error's

  1. When there is error on the form, then currency is switched back to $.
    To Replicate :
    Add Pledge -> Don't Select Contact (leave contact blank). Change the currency to INR (₨) ($ is replaced by (₨)) and submit the form. You will see the currency symbol is $ even though INR is selected.

  2. On Edit of Pledge Form, Symbol is not shown. (regression error)

  3. (Design issue) Can we format the Amount Columns to include currency?
    a. Pledged, total paid, Balanced, Next amount under Pledge tab of Contact
    b. Under pledge instalment for Amount due and Amount Paid column?
    c. On Pledge View Form Total Pledge Amount and To be paid rows.

@pradpnayak
Copy link
Contributor

I think #3 is covered under #9476

@jitendrapurohit
Copy link
Contributor Author

@pradpnayak Can I assume point 2 is too covered in #9476 ? Since the currency-symbol do get displayed for me on Edit Pledge Form.

edit_pledge

@pradpnayak
Copy link
Contributor

yes #2 also covered in #9476

@monishdeb
Copy link
Member

@pradpnayak is this PR ready to merge ?

@pradpnayak
Copy link
Contributor

i guess # 1 still need to be addressed

@jitendrapurohit
Copy link
Contributor Author

jitendrapurohit commented Dec 7, 2016 •
edited
Loading

@pradpnayak I see # 1 is included in the update since it works fine for me. Is there something I'm missing?

pledge_validation

@pradpnayak
Copy link
Contributor

Sorry, i will test the PR now .

@pradpnayak
Copy link
Contributor

During validation error, symbol is not shown for missing currency symbol. But the same is shown with Currency name when currency is switched.

@jitendrapurohit
Copy link
Contributor Author

@pradpnayak As per comment written here, its fine to not show symbols for few currencies. I've opened a new ticket to add missing symbols for them https://issues.civicrm.org/jira/browse/CRM-19733

@pradpnayak
Copy link
Contributor

@jitendrapurohit i feel then it should also hold true when we change the currency symbol. Its little confusing when there is a validation error the currency name for missing currency symbol is missing but it appears when we try to change currency from currency drop down.

If the core is happy to accept this design, then please go ahead and merge this PR, as i have tested on local and it works very well besides above mentioned issue.

@jitendrapurohit
Copy link
Contributor Author

@pradpnayak fixed the js part to show symbol only when it is present.

@pradpnayak
Copy link
Contributor

Tested, working fine

@monishdeb monishdeb merged commit 61b5c8c into civicrm:master Dec 13, 2016
@jitendrapurohit jitendrapurohit deleted the CRM-19549 branch December 13, 2016 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pradpnayak pradpnayak pradpnayak left review comments

Assignees
No one assigned
Labels
master
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jitendrapurohit @pradpnayak @monishdeb @civicrm-builder @sharique