gpb-sync-bookings-on-entry-trash-restore.php: Added conflict handling and orphan retry for trash/restore booking sync and refactored to class-based snippet.#1237
Open
SebastianWiz wants to merge 4 commits into
Conversation
…ng and orphan retry for trash/restore booking sync and refactored to class-based snippet.
WalkthroughRefactors entry trash/restore handling into a new GPB_Trash_Sync singleton that tracks deleted bookings via a meta flag, retries recreation of orphaned bookings, logs conflicts/notes, and surfaces transient admin notices for retry/results. Changes
Sequence Diagram(s)sequenceDiagram
participant AdminUI as Admin UI
participant GFEntry as Gravity Forms Entry
participant GPB as GPB_Trash_Sync
participant Booking as Booking System
participant DB as Database/Meta
AdminUI->>GFEntry: Trash or Restore entry
GFEntry->>GPB: trigger handle_status_change(entry_id, new, old)
GPB->>DB: set/clear FLAG_KEY meta on trash/restore
GPB->>Booking: attempt delete bookings on trash
Booking-->>GPB: success / conflict error
alt conflict or failure
GPB->>GFEntry: add note (conflict/error)
GPB->>DB: keep FLAG_KEY for retry
else success
GPB->>DB: clear FLAG_KEY
GPB->>GFEntry: add success note
end
GPB->>GPB: retry_orphaned_bookings(form_id, skip_id) (on trash)
GPB->>Booking: recreate orphaned bookings for flagged entries
Booking-->>GPB: result
GPB->>AdminUI: queue transient notices (ajax_flash_notices / print_flash_script)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ng and orphan retry for trash/restore booking sync and refactored to class-based snippet.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gp-bookings/gpb-sync-bookings-on-entry-trash-restore.php`:
- Around line 92-104: GFAPI::get_entries() can return WP_Error, so guard the
$entries result before iterating: after the GFAPI::get_entries(...) call, check
is_wp_error($entries) and handle it (e.g., log the error via error_log or a
logger and return early), and also ensure $entries is an array
(is_array($entries)) before running the foreach that processes entries; update
the code around the GFAPI::get_entries call and the iteration to perform these
checks (reference: GFAPI::get_entries and the $entries variable and the
subsequent foreach).
- Line 100: The inline associative array "( 'key' => 'date_created', 'direction'
=> 'ASC' )" triggers the PHPCS rule; replace that inline array with a multiline
array declaration so each key/value is on its own line and include a trailing
comma, e.g. change array( 'key' => 'date_created', 'direction' => 'ASC' ) to a
multiline array( \n 'key' => 'date_created',\n 'direction' => 'ASC',\n) at
the call site where the array is passed (the array literal in the code that ends
with "), array( 'key' => 'date_created', 'direction' => 'ASC' ) ); ).
…ng and orphan retry for trash/restore booking sync and refactored to class-based snippet.
…ng and orphan retry for trash/restore booking sync and refactored to class-based snippet.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@gp-bookings/gpb-sync-bookings-on-entry-trash-restore.php`:
- Around line 157-165: The ajax_flash_notices() endpoint currently only calls
check_ajax_referer() but doesn't check the current user's permissions; update
ajax_flash_notices to verify an appropriate capability (e.g.,
current_user_can('gform_edit_entries') or another Gravity Forms entry access
capability) before reading/deleting the transient (use the same transient_key()
reference) and returning notices; if the capability check fails, respond with
wp_send_json_error() or wp_die() to deny access.
- Around line 85-92: The catch block currently treats every \Throwable as a
"slot occupied" conflict; change the handler in the catch (\Throwable $e) around
the recreate-on-restore logic to distinguish real booking-conflict errors from
other exceptions (e.g. check for a BookingConflictException type or implement an
is_conflict_exception($e) check that inspects exception class/code/message), and
only call GFFormsModel::add_note(..., 'gpb_restore_conflict') and the
conflict-specific queue_notice/log when that check is true; otherwise add a
generic failure note via GFFormsModel::add_note with the actual error message,
emit a non-conflict queue_notice, and log the full exception using $this->log so
non-conflict errors are not mislabeled.
Comment on lines
85
to
+92
| } catch ( \Throwable $e ) { | ||
| if ( function_exists( 'gp_bookings' ) ) { | ||
| gp_bookings()->log_debug( sprintf( 'Restore recreation failed for entry %d: %s', $entry_id, $e->getMessage() ) ); | ||
| $note = sprintf( | ||
| 'Booking could not be recreated on restore — the time slot is occupied by another entry. This entry will be automatically rebooked if the conflicting booking is freed. (Error: %s)', | ||
| $e->getMessage() | ||
| ); | ||
| GFFormsModel::add_note( $entry_id, 0, 'GP Bookings', $note, 'gpb_restore_conflict' ); | ||
| $this->queue_notice( 'error', sprintf( 'Could not recreate booking for entry #%d — slot is occupied. Automatic retry enabled.', $entry_id ) ); | ||
| $this->log( sprintf( 'Restore recreation failed for entry %d: %s', $entry_id, $e->getMessage() ) ); |
There was a problem hiding this comment.
Avoid labeling every restore failure as a slot conflict.
On Line 85-Line 92, all \Throwable paths are treated as “slot occupied”. That can misclassify non-conflict failures (DB/runtime/plugin errors) and leave misleading entry notes.
💡 Suggested adjustment
} catch ( \Throwable $e ) {
- $note = sprintf(
- 'Booking could not be recreated on restore — the time slot is occupied by another entry. This entry will be automatically rebooked if the conflicting booking is freed. (Error: %s)',
- $e->getMessage()
- );
+ $note = sprintf(
+ 'Booking could not be recreated on restore due to an unexpected error. Automatic retry remains enabled. (Error: %s)',
+ $e->getMessage()
+ );
GFFormsModel::add_note( $entry_id, 0, 'GP Bookings', $note, 'gpb_restore_conflict' );
- $this->queue_notice( 'error', sprintf( 'Could not recreate booking for entry #%d — slot is occupied. Automatic retry enabled.', $entry_id ) );
+ $this->queue_notice( 'error', sprintf( 'Could not recreate booking for entry #%d. Automatic retry enabled.', $entry_id ) );
$this->log( sprintf( 'Restore recreation failed for entry %d: %s', $entry_id, $e->getMessage() ) );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch ( \Throwable $e ) { | |
| if ( function_exists( 'gp_bookings' ) ) { | |
| gp_bookings()->log_debug( sprintf( 'Restore recreation failed for entry %d: %s', $entry_id, $e->getMessage() ) ); | |
| $note = sprintf( | |
| 'Booking could not be recreated on restore — the time slot is occupied by another entry. This entry will be automatically rebooked if the conflicting booking is freed. (Error: %s)', | |
| $e->getMessage() | |
| ); | |
| GFFormsModel::add_note( $entry_id, 0, 'GP Bookings', $note, 'gpb_restore_conflict' ); | |
| $this->queue_notice( 'error', sprintf( 'Could not recreate booking for entry #%d — slot is occupied. Automatic retry enabled.', $entry_id ) ); | |
| $this->log( sprintf( 'Restore recreation failed for entry %d: %s', $entry_id, $e->getMessage() ) ); | |
| } catch ( \Throwable $e ) { | |
| $note = sprintf( | |
| 'Booking could not be recreated on restore due to an unexpected error. Automatic retry remains enabled. (Error: %s)', | |
| $e->getMessage() | |
| ); | |
| GFFormsModel::add_note( $entry_id, 0, 'GP Bookings', $note, 'gpb_restore_conflict' ); | |
| $this->queue_notice( 'error', sprintf( 'Could not recreate booking for entry #%d. Automatic retry enabled.', $entry_id ) ); | |
| $this->log( sprintf( 'Restore recreation failed for entry %d: %s', $entry_id, $e->getMessage() ) ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gp-bookings/gpb-sync-bookings-on-entry-trash-restore.php` around lines 85 -
92, The catch block currently treats every \Throwable as a "slot occupied"
conflict; change the handler in the catch (\Throwable $e) around the
recreate-on-restore logic to distinguish real booking-conflict errors from other
exceptions (e.g. check for a BookingConflictException type or implement an
is_conflict_exception($e) check that inspects exception class/code/message), and
only call GFFormsModel::add_note(..., 'gpb_restore_conflict') and the
conflict-specific queue_notice/log when that check is true; otherwise add a
generic failure note via GFFormsModel::add_note with the actual error message,
emit a non-conflict queue_notice, and log the full exception using $this->log so
non-conflict errors are not mislabeled.
Comment on lines
+157
to
+165
| public function ajax_flash_notices() { | ||
| check_ajax_referer( 'gpb_flash_notices', 'nonce' ); | ||
|
|
||
| $notices = get_transient( $this->transient_key() ); | ||
| delete_transient( $this->transient_key() ); | ||
|
|
||
| wp_send_json_success( array( | ||
| 'notices' => is_array( $notices ) ? $notices : array(), | ||
| ) ); |
There was a problem hiding this comment.
Add authorization check to the AJAX notice endpoint.
On Line 157-Line 165, check_ajax_referer() validates intent but not permissions. Please gate this action with a capability check (e.g., Gravity Forms entry access capability) before returning notices.
🔐 Suggested adjustment
public function ajax_flash_notices() {
check_ajax_referer( 'gpb_flash_notices', 'nonce' );
+ if ( ! current_user_can( 'gravityforms_view_entries' ) ) {
+ wp_send_json_error( array( 'message' => 'Unauthorized.' ), 403 );
+ }
$notices = get_transient( $this->transient_key() );
delete_transient( $this->transient_key() );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public function ajax_flash_notices() { | |
| check_ajax_referer( 'gpb_flash_notices', 'nonce' ); | |
| $notices = get_transient( $this->transient_key() ); | |
| delete_transient( $this->transient_key() ); | |
| wp_send_json_success( array( | |
| 'notices' => is_array( $notices ) ? $notices : array(), | |
| ) ); | |
| public function ajax_flash_notices() { | |
| check_ajax_referer( 'gpb_flash_notices', 'nonce' ); | |
| if ( ! current_user_can( 'gravityforms_view_entries' ) ) { | |
| wp_send_json_error( array( 'message' => 'Unauthorized.' ), 403 ); | |
| } | |
| $notices = get_transient( $this->transient_key() ); | |
| delete_transient( $this->transient_key() ); | |
| wp_send_json_success( array( | |
| 'notices' => is_array( $notices ) ? $notices : array(), | |
| ) ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gp-bookings/gpb-sync-bookings-on-entry-trash-restore.php` around lines 157 -
165, The ajax_flash_notices() endpoint currently only calls check_ajax_referer()
but doesn't check the current user's permissions; update ajax_flash_notices to
verify an appropriate capability (e.g., current_user_can('gform_edit_entries')
or another Gravity Forms entry access capability) before reading/deleting the
transient (use the same transient_key() reference) and returning notices; if the
capability check fails, respond with wp_send_json_error() or wp_die() to deny
access.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
⛑️ Ticket(s): https://secure.helpscout.net/conversation/3224123263/97555?viewId=8614371
Summary
This PR improves trash/restore booking sync by handling conflicts and recovering orphaned entries. Previously, restoring a trashed entry whose time slot was already rebooked would silently fail, leaving an entry with no booking and no way to recover.
Changes: