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

fix: file and class names corrected based on wordpress requirements #53

Merged
merged 5 commits into from
Nov 9, 2023

Conversation

julianramirez2
Copy link
Contributor

@julianramirez2 julianramirez2 commented Oct 18, 2023

Description

The file names were changed due to WordPress restrictions when uploading the plugin, which prohibited the use of reserved words such as 'plugin' or 'woocommerce.' It was decided that this name change would be made to 'openedx_ecommerce' instead of 'openedx_woocommerce_plugin.' Similarly, in accordance with WordPress code best practices, the names of the classes and namespaces used in the code were also changed.

Testing instructions

Pull the branch locally and run the 'composer install' command to install all plugin dependencies and test. Additionally, these changes have already been uploaded to the staging environment where you can verify that everything is functioning correctly.

Checklist for Merge

  • Tested in a remote environment
  • Updated documentation / NA
  • Rebased master/main / NA
  • Squashed commits / NA

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 18, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Oct 18, 2023

Thanks for the pull request, @julianramirez2! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@mphilbrick211
Copy link

mphilbrick211 commented Oct 19, 2023

Hi @julianramirez2! Thanks for this contribution! Please let me know if you have any questions regarding filling out the CLA form. If you are submitting on behalf of an organization, please have your manager reach out to [email protected] in order to be added to your organization's entity agreement.

@julianramirez2
Copy link
Contributor Author

Hi @julianramirez2! Thanks for this contribution! Please let me know if you have any questions regarding filling out the CLA form. If you are submitting on behalf of an organization, please have your manager reach out to [email protected] in order to be added to your organization's entity agreement.

Hi @mphilbrick211, thank you for providing the information. I have just completed the CLA form and am now awaiting the email confirmation of my CLA approval.

MaferMazu
MaferMazu previously approved these changes Oct 20, 2023
Copy link
Contributor

@MaferMazu MaferMazu left a comment

Choose a reason for hiding this comment

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

This comes because we needed to change the plugin name to submit to the WordPress org, and we want to maintain the congruency of the names.
I was a little afraid because this could generate breaks, but I already tested in stage, and this doesn't break anything.

@MaferMazu MaferMazu requested a review from feanil October 20, 2023 21:27
@feanil
Copy link
Contributor

feanil commented Oct 23, 2023

@julianramirez2 @MaferMazu calling it ecommerce is a bit complicated, is the problem that they don't want us to use woocommerce in our name? Could we go with openedx-commerce-plugin instead?

@MaferMazu
Copy link
Contributor

We can't use Woocommerce or Plugin in the name of a WordPress plugin. Is it okay to stay with openedx-ecommerce, or is it better to change it to openedx-commerce, @feanil?

@feanil
Copy link
Contributor

feanil commented Oct 25, 2023

Let's go with openedx-commerce

@feanil
Copy link
Contributor

feanil commented Oct 25, 2023

Also, can we add an ADR or something about this so we have a record of what the restrictions were when we set the name?

@MaferMazu
Copy link
Contributor

MaferMazu commented Oct 25, 2023

@julianramirez2, can you help me with the naming and updating #54? I can create the ADR for this.

@mphilbrick211
Copy link

H @julianramirez2! It looks like your CLA check is still failing. You mentioned you submitted your CLA form last week, but if you are contributing on behalf of eduNEXT, you'll need to have your manager reach out to [email protected] to be added to eduNEXT's entity agreement (if they haven't done so already).

If this has already been done, please let me know and I can check on the status for you. Thanks!

Copy link
Contributor Author

@julianramirez2 julianramirez2 left a comment

Choose a reason for hiding this comment

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

Everything looks good

@mphilbrick211
Copy link

@julianramirez2 great! It looks like a few conflicts popped up - would you mind taking a look?

@MaferMazu MaferMazu force-pushed the file-names-correction branch 2 times, most recently from 7a504bb to 53a9ef3 Compare November 7, 2023 22:34
@MaferMazu
Copy link
Contributor

This change is working well
image
And we already changed the name to commerce and added the ADR; what do you think, @feanil?

MaferMazu
MaferMazu previously approved these changes Nov 7, 2023
felipemontoya
felipemontoya previously approved these changes Nov 8, 2023
Copy link
Member

@felipemontoya felipemontoya left a comment

Choose a reason for hiding this comment

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

Good work @MaferMazu, with the successful test of the code correctly loading and working I think this is good to merge.

@MaferMazu MaferMazu dismissed stale reviews from felipemontoya and themself via 794b3af November 8, 2023 23:33
@MaferMazu MaferMazu force-pushed the file-names-correction branch from 53a9ef3 to 794b3af Compare November 8, 2023 23:33
Copy link
Contributor

@MaferMazu MaferMazu left a comment

Choose a reason for hiding this comment

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

I rebase this and It is working well
image

Tested:

  • In new instalation
  • Stage
  • And with Olive and Quince

Copy link
Contributor

@feanil feanil left a comment

Choose a reason for hiding this comment

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

Name change looks good to me.

@MaferMazu MaferMazu merged commit 79bdd6a into openedx:main Nov 9, 2023
4 checks passed
@openedx-webhooks
Copy link

@julianramirez2 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants