Refactoring, unify Omise key(s)-defining into one place.#108
Merged
Refactoring, unify Omise key(s)-defining into one place.#108
Conversation
e689704 to
640be07
Compare
…ep), removing all related code.
640be07 to
5853102
Compare
jonrandy
reviewed
Mar 28, 2019
| @@ -7,11 +7,37 @@ | |||
| } | |||
|
|
|||
| class Omise_Setting { | |||
There was a problem hiding this comment.
Should really be called "Settings" - otherwise the name suggests a class that deals with one setting only
jonrandy
reviewed
Mar 28, 2019
| array( | ||
| 'key' => $this->public_key, | ||
| 'key' => Omise()->setting->public_key(), | ||
| 'ajax_url' => admin_url( 'admin-ajax.php' ), |
There was a problem hiding this comment.
Not sure, but should this be setting()? (should be calling the function?)
There was a problem hiding this comment.
Again, this should definitely be named settings
Contributor
Author
|
@jonrandy @jacstn updated, can you help check f1cc6db However, renaming |
f1cc6db to
f304275
Compare
Contributor
Author
|
|
…r sense of its fucntion.
f304275 to
31f1402
Compare
Contributor
Author
|
Note: Updated Quality Assurance section. All tested. |
jonrandy
approved these changes
Mar 29, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
1. Objective
Currently in Omise-WooCommerce code base, the public key and the secret key have been spread all over the places. By that, it has a high risk and a potential of any bug from code maintenance in the future (as there are many places that need to be taken care of).
This pull request is to unifying Omise-Keys declaration into one place to reduce any potential bug & code mess.
2. Description of change
3. Quality assurance
🔧 Environments:
✏️ Details:
All of cases that we should test would be really huge as this pull request is removing all key-assigning out from all of Omise class. However, the main test could be listed as below:
Make sure that Alipay payment method works properly by creating a new charge with Alipay payment method.
Make sure that Internet Banking payment method works properly by creating a new charge with Internet Banking payment method.
Make sure that Credit Card payment method works properly by creating a new charge with Credit Card payment method on both
auth onlyandauto capture.Make sure that an order can be funded properly
Make sure that user can save a card properly.
Make sure that user can delete a card properly.
4. Impact of the change
No
5. Priority of change
Normal
6. Additional Notes
This change will help on #101 as well (as there is no keys tied up in those methods anymore after this change).