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

Enhancement/23 Code Cleanup + Min Version Bumps + PHP Compat Testing #24

Merged
merged 20 commits into from
Aug 3, 2022

Conversation

faisal-alvi
Copy link
Member

@faisal-alvi faisal-alvi commented Aug 2, 2022

Changes made in this PR:

  • This PR re-structures the entire plugin and separates the codes intro relative class files.
  • Cleans up the code from the plugin.
  • Standardized the code, use escaping functions.
  • Makes strings translatable.
  • Adds new files: composer.json, phpcs.xml, .gitignore.
  • Adds a PHPCS Compatibility testing Github Action.
  • Bumps min versions: WordPress to 5.6, WooCommerce to 6.0, PHP to 7.0
  • Also fixes the issue of the resource costs not being imported (Resource Costs empty when imported #25).

Closes #21, Closes #22, Closes #23, Closes #25.

@faisal-alvi faisal-alvi added this to the Future Release milestone Aug 2, 2022
@faisal-alvi faisal-alvi self-assigned this Aug 2, 2022
@faisal-alvi faisal-alvi marked this pull request as ready for review August 2, 2022 09:51
@faisal-alvi
Copy link
Member Author

@dkotter the PHP Compat check test is not running, does it require a merge in the master branch?

@dkotter dkotter self-requested a review August 2, 2022 21:45
@dkotter
Copy link
Contributor

dkotter commented Aug 2, 2022

@faisal-alvi This looks good to me! I made a few changes to some PHPCS issues I saw getting flagged when I checked this out locally. If you want to do a double-check on those to make sure I didn't break anything, I think we'll be good to get this merged in.

Those changes are:
88642bc
227a419

@faisal-alvi
Copy link
Member Author

faisal-alvi commented Aug 3, 2022

@dkotter Thanks for the changes, I've checked, and it's working fine. However, while testing, I found an issue for which I have created a separate ticket (#25) and applied a fix here. (this is irrelative to the changes you've made)

By the way, when I checked PHPCS in my local system, it didn't show any errors/warnings. I have used the following command (the same as the yml file). Which command have you used?

./vendor/bin/phpcs *.php includes -p --standard=PHPCompatibilityWP --extensions=php --runtime-set testVersion 7.0-

@faisal-alvi
Copy link
Member Author

After implementing a new export feature on the product page as mentioned in https://github.com/woocommerce/woocommerce-bookings/issues/3376, we will have single zip with both data, 1) Global Rules and 2) Bookable Product. We have to enhance the import functionality here so that Rules and a Product can be imported through a single zip.

@dkotter I have a couple of questions related to this enhancement:

  1. Should we add a checkbox (ticked by default) so HEs can decide if they also want rules to be imported? When unchecked, only the product will be imported.
  2. Should we implement this enhancement in this PR or create a new one from this PR branch (as it has a new structure)?

@dkotter
Copy link
Contributor

dkotter commented Aug 3, 2022

By the way, when I checked PHPCS in my local system, it didn't show any errors/warnings. I have used the following command (the same as the yml file). Which command have you used?

These were getting flagged by my editor which is using the Extendables standards (basically WooCommerce standards which inherits all the WordPress standards).

I have a couple of questions related to this enhancement:

Should we add a checkbox (ticked by default) so HEs can decide if they also want rules to be imported? When unchecked, only the product will be imported.
Should we implement this enhancement in this PR or create a new one from this PR branch (as it has a new structure)?

So I guess I'll answer your question with one of my own :). Is it worth changing this to support importing the individual json files instead of a zip file? Not sure if there's any downsides to that (other than an extra unzip step HEs would have to do) but that would allow us to keep two separate import steps, one for the product json and one for the rules json.

If that's not feasible or not a good idea, I do think having a checkbox that can be used to determine if they should be imported will be good. I'd say probably not checked by default, just so they don't unintentionally add rules that they don't want. And yes, either way I'd handle this in a separate PR. Going to merge this in now

@dkotter dkotter merged commit 3e75a5d into master Aug 3, 2022
@dkotter dkotter deleted the enhancement/23 branch August 3, 2022 16:52
@faisal-alvi
Copy link
Member Author

(other than an extra unzip step HEs would have to do)

The purpose of all of these changes was to reduce the efforts+time of HEs, keeping that in mind, I would suggest going with a checkbox instead of letting them unzip and import 2 JSON files one by one. I will create a PR for that soon.

@jeffpaul jeffpaul modified the milestones: Future Release, 1.0.4 Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants