-
Notifications
You must be signed in to change notification settings - Fork 843
Forms: Removes the classic admin initialization call #44538
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
base: trunk
Are you sure you want to change the base?
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 4 files.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
CGastrell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It mostly works without hiccups, except on the Google Export. That process is still relying on class-admin::export_to_gdrive.
The issue seems to be the handler is only declared on admin class, as opposed to the CSV export handler that lives on the plugin class.
Gdrive export handler on class-admin:
add_action( 'wp_ajax_grunion_export_to_gdrive', array( $this, 'export_to_gdrive' ) );CSV export handler on plugin class:
add_action( 'wp_ajax_feedback_export', array( $this, 'download_feedback_as_csv' ) );e37c0c0 to
dbc8b03
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes the classic admin initialization call from the forms package as part of transitioning to a new Admin UI for form responses. The classic admin class is being deprecated but left in place for backward compatibility.
- Removes the
Admin::init()call from the forms utility class - Moves Google Drive export functionality from the deprecated Admin class to the main Contact_Form_Plugin class
- Deprecates the classic Admin class while maintaining its interface for backward compatibility
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/plugins/jetpack/changelog/remove-forms-old-admin | Changelog entry for the Jetpack plugin |
| projects/packages/forms/src/contact-form/class-util.php | Removes the classic admin initialization call |
| projects/packages/forms/src/contact-form/class-contact-form-plugin.php | Adds Google Drive export functionality and AJAX handler |
| projects/packages/forms/src/contact-form/class-admin.php | Deprecates the class and delegates export functionality |
| projects/packages/forms/changelog/remove-forms-old-admin | Changelog entry for the forms package |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
CGastrell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me! I fear those phan failing checks will get us deeper into changes
edanzer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a general test of functionality:
- Admin screens and form inbox, including many different actions
- Adding and submitting normal form
- Adding and submitting form with classic editor (I'm surprised this still works)
Beyond that, I tried to go through the methods in this class and be sure that we're not sneakily depending on them elsewhere. I assume @enejb and @CGastrell both did that, and I think you are both more familiar with that class than I am. Sounds like only issue was with export method that we already moved (that also called Admin->get_export_filename, but I see it was updated to use Util::get_export_filename).
I can't see any other issues or functionality in that class that's needed outside the feedback post type page.
For the record, I feel like that file and class were kind of misnamed. I'd think an Admin class would handle lots of general admin logic, but really this is just a file for the feedback post type screen. I guess for a long time that screen WAS the entire admin interface.
| if ( is_admin() ) { | ||
| add_action( 'wp_ajax_feedback_export', array( $this, 'download_feedback_as_csv' ) ); | ||
| add_action( 'wp_ajax_create_new_form', array( $this, 'create_new_form' ) ); | ||
| add_action( 'wp_ajax_grunion_export_to_gdrive', array( $this, 'export_to_gdrive' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@enejb - I saw we moved this one. @CGastrell had also flagged the download_feedback_as_csv method, also tied to ajax actions. Do we need to move and still load that outside of this admin class?
CGastrell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked the latest commits, still works fine
Co-authored-by: Copilot <[email protected]>
6e8d52b to
7d5b379
Compare
|
Rebased and re-tested, continues to work well, shall we merge it @enejb ? |
|
This PR has been marked as stale. This happened because:
If this PR is still useful, please do a [trunk merge or rebase](https://github.com/Automattic/jetpack/blob/trunk/docs/git-workflow.md#keeping-your-branch-up-to-date) and otherwise make sure it's up to date and has clear testing instructions. You may also want to ping possible reviewers in case they've forgotten about it. Please close this PR if you think it's not valid anymore — if you do, please add a brief explanation. If the PR is not updated (or at least commented on) in another month, it will be automatically closed. |
We are have switched over from the classic admin.
I think we should leave the class and completely remove it in a few version. To give folks that might be calling it chance to migrate to a different code base.
Proposed changes:
Other information:
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Visit the wp-admin.
Does anything break?
Can you export to CSV?
Can you export to Google?
Can you trash, mark as spam feedback?
Try it when you have the classic editor enabled. Does it still work? To add a form?