-
Notifications
You must be signed in to change notification settings - Fork 198
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
Avoid duplicate POST on post-submit quiz page reload #4123
base: trunk
Are you sure you want to change the base?
Conversation
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 think this is a good tweak. Just a bit concerned about compatibility (see comment below), but if we can do this without changing the action then let's go for it!
@@ -35,10 +35,10 @@ public function __construct( $file = __FILE__ ) { | |||
add_action( 'template_redirect', array( $this, 'reset_button_click_listener' ) ); | |||
|
|||
// Fire the complete quiz button submit for grading action. | |||
add_action( 'sensei_single_quiz_content_inside_before', array( $this, 'user_quiz_submit_listener' ) ); | |||
add_action( 'template_redirect', array( $this, 'user_quiz_submit_listener' ) ); |
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.
Can we do this without changing the action? This would be a breaking change for any customizations that remove or modify these hooks.
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.
Hm, changing the action to what's better aligned with the callbacks' purpose is sort of the whole point here.. 🤔
There's also prior art right around these add_action
calls that use template_redirect
, so we also achieve better systemic alignment here.
Breaking change: yes - what is Sensei's policy for versioning? Such is usually handled by new major versions, but I'm aware WP and WC don't follow semver.
It seems like this change is fairly well "updatable" in customizations, if we highlight it well enough in release documentation.
Your thoughts?
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.
We have this deprecation policy that we try to adhere to, but it doesn't cover changing actions like this 🤔
I was thinking that we might be able to fix the problem (refresh causing quiz submit) without changing the hook, but I agree that moving the functionality out of the rendering actions ultimately makes more sense.
Let me circle back on this.
Changes proposed in this Pull Request
This became a problem when we implemented a quiz re-take delay mechanism.
People would reload quiz page to see how much time remaining to wait, but this re-POSTs quiz and resets our timer.
Solution, and relevant part to Sensei core - proper hygiene in general, is to have a simple pre-output phase GET redirect to quiz URL, that upon reload will not resubmit quiz data.
RATIONALE
Quiz submit should be an intentional operation - page reload should not cause it in the background, since it has implications about "when was quiz last submitted" and potentially other related data (our timer use case), important to online schools, being incorrect.
I don't see any specific usefulness for these callbacks to live on frontend rendering phase hooks. Please enlighten me if I'm missing something.
Testing instructions