Skip to content

Refactoring plugin-initial code structure - part 3: Organizing Omise_Admin class#96

Merged
guzzilar merged 3 commits intomasterfrom
refactoring-plugin-initiating-structure-part3
Jan 21, 2019
Merged

Refactoring plugin-initial code structure - part 3: Organizing Omise_Admin class#96
guzzilar merged 3 commits intomasterfrom
refactoring-plugin-initiating-structure-part3

Conversation

@guzzilar
Copy link
Copy Markdown
Contributor

@guzzilar guzzilar commented Jan 18, 2019

⚠️ Important: This pull request requires #95: "Refactoring plugin-initial code structure, part 2" to be merged first.

1. Objective

Omise-WooCommerce has always been coupled with WooCommerce plugin, which the plugin cannot be used as a stand-alone plugin and always require WooCommerce plugin to be activated first.
However, the legacy-codebase itself wasn't really aware of a case where WooCommerce plugin is disabled as much as it should be.

Although we have prevented such cases #78, and #83. But by the current design of the plugin-initial step, it still remains some issue (#90) that could be ceased out by redesigning the initial-step with a proper awareness of WooCommerce plugin.

Related issue(s):

2. Description of change

Part 1

See, #94: "Refactoring plugin-initial code structure, part 1"

Part 2

See, #95: "Refactoring plugin-initial code structure, part 2"

Part 3

The last piece of code,
This part is aiming to organizing Omise_Admin class, by moving all admin-related code from Omise class back to Omise_Admin.

3. Quality assurance

🔧 Environments:

  • WordPress: v5.0.3
  • WooCommerce: v3.5.3
  • PHP version: v5.6.30

✏️ Details:

The main test-case will be separated into 3 cases as follows:

  1. Attempting to install Omise-WooCommerce without WooCommerce should give you a warning message that Omise-WooCommerce requires WooCommerce plugin to be activated first.
    Omise-WooCommerce plugin can be activated, but all functions should be disabled, including no Omise menu at the WordPress admin-sidebar.

screen shot 2562-01-18 at 00 12 26

  1. Install Omise-WooCommere plugin while WooCommerce plugin is enabled should give you a full-access to all Omise-WooCommerce feature, including the setting page.

screen shot 2562-01-17 at 23 32 33

  1. Attempting to disable WooCommerce plugin while Omise-WooCommerce is still enabled should give you a same result as the case [1].

screen shot 2562-01-18 at 00 12 26

  1. After the change, charge must be created, captured, refunded as usual.

4. Impact of the change

No

5. Priority of change

Normal

6. Additional Notes

No

Copy link
Copy Markdown

@jonrandy jonrandy left a comment

Choose a reason for hiding this comment

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

As before, cannot fully approve until a quick test is done. Some comments here though

Comment thread omise-woocommerce.php
*/
public function init_error_messages(){
?>
<div class="error">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Again, this REALLY feels wrong. Does WP advocate this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Welcome to WordPress world.
I cannot tell if WP advocates for this but the WP codebase is even worse..
But yup, this is kind of common thing in WordPress (even WooCommerce and many plugins also go with this way everywhere in their code).

We can go a little advance by moving this thing into a template file and load it but I guess that would be out of scope for this pull request. Maybe we can discuss further in a future PR :)

Comment thread omise-woocommerce.php
public function init_error_messages(){
?>
<div class="error">
<p><?php echo __( 'Omise WooCommerce plugin requires <strong>WooCommerce</strong> to be activated.', 'omise' ); ?></p>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Need to add 'The'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍 This will be automatically solved after merge #94

@guzzilar guzzilar force-pushed the refactoring-plugin-initiating-structure-part3 branch from 8d0bb07 to d7f349b Compare January 21, 2019 04:56
@guzzilar guzzilar merged commit 67d09bb into master Jan 21, 2019
@guzzilar guzzilar deleted the refactoring-plugin-initiating-structure-part3 branch January 21, 2019 08:04
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.

3 participants