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

Repair edit job listing screen. #2802

Merged
merged 5 commits into from
Apr 16, 2024
Merged

Repair edit job listing screen. #2802

merged 5 commits into from
Apr 16, 2024

Conversation

gikaragia
Copy link
Contributor

@gikaragia gikaragia commented Apr 9, 2024

Oveview

This fixes an issue to the edit job listing screen. To reproduce:

  1. Create a job and publish it
  2. Set up a recaptcha and enable it in the settings (any version will do, go to https://www.google.com/recaptcha to create keys if you don't have some already)
  3. Go to the job dashboard
  4. Edit the job
  5. After doing some change try to save the listing
  6. At this point the update fails and nothing happens

The problem was that we added the actions to the WP_Job_Manager_Recaptcha constructor. This way the hooks were added even when we wanted to call utility methods and the captcha was also enqueued to the dashboard page.

Changes Proposed in this Pull Request

  • I moved the hook registration from the constructor to equeue_scripts. This way we only enqueue the captcha when we need to.

Testing Instructions

  • Test together with: https://github.com/Automattic/wpjm-addons/pull/481
  • Follow the steps from the overview and observe that nothing is broken now.
  • Also try testing recaptcha functionality in general to ensure that nothing else is broken.
  • Additionally, we should test how the changes in reCAPTCHA work in relation to different plugin versions:
    • The newest version of the paid plugins should work with the previous version of WPJM (i.e. 2.2.2) but with v2 reCAPTCHA only
    • The previous versions of the paid plugins should work with latest version of WPJM but with warnings printed as we deprecated the old methods.

Plugin build for 42aae9c
📦 Download plugin zip
▶️ Open in playground

@gikaragia gikaragia requested a review from a team April 9, 2024 16:23
@@ -136,7 +136,7 @@ public function process() {
* @param array $atts Attributes to use in the view handler.
*/
public function output( $atts = [] ) {
WP_Job_Manager\WP_Job_Manager_Recaptcha::enqueue_scripts();
WP_Job_Manager\WP_Job_Manager_Recaptcha::enqueue_recaptcha();
Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't tested yet, but it looks fragile to add the hooks that process submit fields in the output and enqueue functions. Those might not get called or get called later than process(). Does it only work now because the submit job form has multiple steps?

Since reCAPTCHA is only used on specific forms I think the abstract WP_Job_Manager_Form class, which has the common code for every form, isn't the place to set it up. Looks like there are separate WP_Job_Manager_Form_Edit_Job and WP_Job_Manager_Form_Submit_Job classes, should the hooks be added from here?:

WP_Job_Manager\WP_Job_Manager_Recaptcha::instance();

(Unfortunately WP_Job_Manager_Form_Edit_Job extends WP_Job_Manager_Form_Submit_Job so we still need something to not load it for editing. Maybe a ...Submit_Job::init_recaptcha() method that the ...Edit_Job class overrides?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point, I also thought about this initially but I ended up taking the quick approach since it worked. I'll look into refactoring

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok so I moved everything inside the recaptcha class. I also did some fixes regarding versioning as it seems that previously the latest version of the paid plugins didn't work with old WPJM versions. I have updated the testing instructions to reflect that.

Previously the hooks where added on instance creation which
would cause the hooks to be added on the job edit form and break
it.
@gikaragia gikaragia requested a review from yscik April 11, 2024 15:32
Copy link
Contributor

@yscik yscik left a comment

Choose a reason for hiding this comment

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

Looks good! :shipit:

includes/abstracts/abstract-wp-job-manager-form.php Outdated Show resolved Hide resolved
includes/class-wp-job-manager-recaptcha.php Outdated Show resolved Hide resolved
gikaragia and others added 2 commits April 16, 2024 11:58
Co-authored-by: Peter Kiss <[email protected]>
@gikaragia gikaragia merged commit 0e2c21f into trunk Apr 16, 2024
8 of 10 checks passed
@gikaragia gikaragia deleted the fix/edit-job-form branch April 16, 2024 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants