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: improve namespace and variables naming #65

Merged
merged 5 commits into from
Dec 20, 2023

Conversation

MaferMazu
Copy link
Contributor

Description

This PR:

  • Change the namespace App to OpenedXCommerce.
  • Change the attribute and variable names to add openedx as the prefix, e.g., enrollment_course_id to openedx_enrollment_course_id.
  • Add prefixes to some style variables.

Testing instructions

For testing purposes, you can:

  1. Enter in the WordPress admin of this stage https://share.1password.com/s#mjFE_KtE55oS9OrVUCMw0aZw8r4BvF-6ONKeFxJ2BuI
  2. Test the API calls in Settings > Open edX Sync Plugin (Saving and generating JWT tokens).
  3. Create enrollment requests or save them in WordPress's Open edX Sync option.

Note:

  • I tested locally and remotely in a new or existing environment.

Additional information

This PR is for avoiding WordPress's common mistakes, in particular, the following:

  1. Missing or inadequate function prefixes
    (issue found in approx. 60% of first reviews)

All active plugins in a WordPress installation will be loaded during execution. When two elements use the same name, a collision will happen. With more than 80k plugins in the repository, collisions are increasingly common.

That's why we need plugins to prefix their: function names, class names, namespaces, defines, option names, nonces and own input names with something that is related to this plugin, sufficiently complex and at least 5 characters long.

Example: a plugin named "Moon forms" could choose "moofor_ " or "moonforms_ " as a prefix but "mf_ ", "moon_ " or "forms_ " would be bad choices as they are too short, too generic or both.

This rule applies not only to function and class names (where namespaces are the best alternative) but also to defines, saving options, nonces and own input names, for example:

function moonforms_submit_button(){
class Moonforms_Form {
namespace Moonforms\Submit;

define('MOONFORMS_PLUGIN_URL', plugin_dir_url(FILE) );
define('MOONFORMS_PLUGIN_DIR', plugin_dir_path(FILE) );

update_option( 'moonforms-default-email', $email );

wp_nonce_field( 'moonforms-form', 'moonforms-nonce' );
if ( !empty( $_POST['moonforms-nonce'] ) && wp_verify_nonce( sanitize_text_field( wp_unslash ( $_POST['moonforms-nonce'] ) ), 'moonforms-form' ) ) {

$email = sanitize_email( $_POST['moonforms-email'] );

Note
Before merging this PR, I want to ensure we are not editing some general CSS styles and colliding with other plugins.

Checklist for Merge

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

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

Thanks for the pull request, @MaferMazu! 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.

@MaferMazu
Copy link
Contributor Author

I would like to add a few changes to the CSS variable naming (I want to ensure we are not editing some general CSS styles and colliding with other plugins), but if you have early feedback, please let me know.

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.

@MaferMazu I looked at the changes and it looks reasonable.

In the review it looks like you didn't change the names of anything that becomes a post_metadata, right?
If I'm correct then we should be able to merge and the changes would be backwards compatible with enrollment_requests that were already sent with the old var names. We can verify this by uploading this code in the staging and testing the admin page for the old requests. If everything works we could go ahead and merge.

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.

Generally looks good to me but I had one question about a few variables that were not renamed.

admin/class-openedx-commerce-admin.php Outdated Show resolved Hide resolved
@MaferMazu
Copy link
Contributor Author

Yes, @felipemontoya, I am not touching the post_metadata variables. I have tested the plugin locally and in the Stage with the changes. You can double-check in the Stage with these credentials: https://share.1password.com/s#mjFE_KtE55oS9OrVUCMw0aZw8r4BvF-6ONKeFxJ2BuI.

And I renamed the variables for consistency, as @feanil suggested.

About the CSS variables I wanted to check, I checked, and they don't change the behavior of other plugins.

I think we can go with this and send the ZIP file.

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.

Thanks for the updates and fixes @MaferMazu.

I think we can merge this and present the zip for publication

@MaferMazu MaferMazu merged commit 9f4493d into openedx:main Dec 20, 2023
5 checks passed
@openedx-webhooks
Copy link

@MaferMazu 🎉 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.

4 participants