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

Issues 2275 #2345

Merged
merged 34 commits into from
Nov 21, 2017
Merged

Issues 2275 #2345

merged 34 commits into from
Nov 21, 2017

Conversation

raftaar1191
Copy link
Contributor

@raftaar1191 raftaar1191 commented Nov 15, 2017

Description

PR to resolve #2275

How Has This Been Tested?

Manually test by adding the currency

Screenshots (jpeg or gifs if applicable):

image

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@raftaar1191
Copy link
Contributor Author

@ravinderk @DevinWalker
Please review and merge the PR

@raftaar1191
Copy link
Contributor Author

@ravinderk Need your help with Travis. As builds is getting failed

@ravinderk
Copy link
Collaborator

@raftaar1191

  1. Please add the unit tests to check all currencies.
  2. Please share you testing details.
  3. Do you think that chosen field will be more helpful to select currency instead of select?

@DevinWalker Do you think we can add bitcoin to the core?
for ref: https://github.com/WordImpress/Give/pull/2345/files#diff-5806e141eaa0104339c5325ec46eec73R559

@raftaar1191
Copy link
Contributor Author

raftaar1191 commented Nov 16, 2017

  1. Please add the unit tests to check all currencies.
    -> Adding PHPUnit

  2. Please share you testing details.
    ->I tested it by changing the currency from the Dashboard of Give Core and then in the frontends making a test donation and checking the value in the Dashboard -> Donation

  3. Do you think that chosen field will be more helpful to select currency instead of select?
    -> Yes, Absolutely. it will be really useful for admin, Adding that

@DevinWalker
Copy link
Member

@raftaar1191 these unit tests have been failing after running several times. We need to figure out why that is happening prior to merging this into core. I think it's having a problem looping through all the new currencies added.

@DevinWalker DevinWalker merged commit 97930d6 into impress-org:release/1.8.17 Nov 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants