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

Verify the order before checking for error flags #1295

Open
wants to merge 1 commit into
base: production
Choose a base branch
from

Conversation

dd32
Copy link
Member

@dd32 dd32 commented Apr 23, 2024

While looking at the checkout process as part of #1296 I noticed that verify_order() is run after the check for error_flags, which means if verify_order() flags any issues with the order, the checkout still proceeds.

Some of the checks within the verify_order() method is duplicated within the checkout process already, but this ensures that any issues encountered during validation do properly stop the checkout process.

This might reduce the number of ticket_excess issues that some WordCamps can run into during high ticket loads.

@dd32
Copy link
Member Author

dd32 commented Apr 23, 2024

This is less problematic than I originally thought.

verify_order() is being called multiple times.

During checkout; verify_order() is called:
https://github.com/WordPress/wordcamp.org/blob/production/public_html/wp-content/plugins/camptix/camptix.php#L5296-L5299

Errors from that instance of verify_order() are respected.

Then form_checkout() is run
https://github.com/WordPress/wordcamp.org/blob/production/public_html/wp-content/plugins/camptix/camptix.php#L5386-L5387

which then calls verify_order again:
https://github.com/WordPress/wordcamp.org/blob/production/public_html/wp-content/plugins/camptix/camptix.php#L7175-L7180

In that 2nd instance of verify_order running, it'll not respect any new errors thrown by verify_order.. but there shouldn't be any at that point, so the errors are being ignored, because there's unlikely to be any errors at all.

As a result, this is fairly safe to merge I think, as nothing should be changing, since the check is already happening.. but this is also now a low-priority thing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

1 participant