Keep current status for a question when it is updated #7603
Conversation
|
Test the previous changes of this PR with WordPress Playground. |
includes/rest-api/class-sensei-rest-api-question-helpers-trait.php
Outdated
Show resolved
Hide resolved
|
Test the previous changes of this PR with WordPress Playground. |
| } elseif ( ! $is_new ) { | ||
| // If status is not provided, use the current status of the question. | ||
| $post_args['post_status'] = get_post_status( $question_id ); |
There was a problem hiding this comment.
Good job! Solution works perfectly as expected! 🚀
I was just thinking of another solution and wanted to share with you.
Looks like this is a known issue caused by using wp_insert_post for updating posts instead of wp_update_post https://developer.wordpress.org/reference/functions/wp_insert_post/#comment-5150.
So I was just wondering, instead of fetching the status manually here and re-assigning it, do you think it'd be a better and more understandable solution to use the wp_update_post function when we are actually just updating the post? An added advantage will be, we'll be using the right function for the right job.
So we just remove the line above and we replace Line 199 with something like $result = $is_new ? wp_insert_post( $post_args ) : wp_update_post( $post_args );.
WDYT?
There was a problem hiding this comment.
Thanks @Imran92!
Yes, it makes sense for the sake of simplicity.
Thinking about it further, I see a little sense of using a ternary. We can simply use wp_update_post (wp_update_post calls wp_insert_post under the hood).
|
Test the previous changes of this PR with WordPress Playground. |
|
Test the previous changes of this PR with WordPress Playground. |
There was a problem hiding this comment.
LGTM! The solution looks better now! Thanks for making the changes.
Looks like the PHP test is failing - https://github.com/Automattic/sensei/actions/runs/9323311876/job/25666195954?pr=7603 . Probably should be looked into before approval
|
Test the previous changes of this PR with WordPress Playground. |
|
Thanks @Imran92! I had to turn to your suggested approach with a ternary. I missed that in However, it showed another problem. It looks like the behavior of The test checks that On one hand it is a backward incompatibility, because that behavior was documented in tests. I'll think a bit more about it... What is your opinion about it? |
This behavior looks hacky, and in general, I don't like these behaviors. We need to figure out why this behavior is expected in the test and if it breaks anything otherwise, this can be just an erroneous test too.
If the compatible behavior we're looking for is the |
|
Test the previous changes of this PR with WordPress Playground. |
|
Test the previous changes of this PR with WordPress Playground. |
The test was added simultaneously with the method, so I think it wasn't erroneous. But it could be just a reflection of the solution that was used (wp_insert_post). I tested the question UI differently, didn't notice any issues with it. And it worked for 3 years without complaints.
Yep, I think that's the safest solution, updated it here: 485caf5 |
|
PR updated and ready for review. |
|
Test the previous changes of this PR with WordPress Playground. |
Resolves #7601
When we update a question, its status resets to "draft".
The issue appears only for questions that are not in use (are not part of a quiz or of a category).
Proposed Changes
Testing Instructions
Pre-Merge Checklist