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

Ensure we are performing updates on the Lesson, not a revision #2471

Merged
merged 1 commit into from Mar 1, 2019

Conversation

Projects
None yet
4 participants
@alexsanford
Copy link
Contributor

commented Feb 28, 2019

Fixes #2201

More technical details on the cause of the bug are in the commit message.

Ensuring that all checks and updates occur on the parent post fixes the bug.

For testing instructions, see the linked issue. On this branch, editing and saving the lesson should not change its ordering within the module.

Ensure we are performing updates on the Lesson, not a revision
This was causing a bug when $this->lesson_id was the ID of a revision.
Specifically, when checking for meta using `get_post_meta`, we would
check the meta of the revision. However, when updating the meta using
`update_post_meta`, we were updating the meta on the parent.

The trouble was in this code:

```
if ( ! get_post_meta( $this->lesson_id, $order_module_key, true ) ) {
	update_post_meta( $this->lesson_id, $order_module_key, 0 );
}
```

Because $this->lesson_id was a revision, the meta did not exist, and we
would unconditionally call `update_post_meta`, erasing the corresponding
meta on the parent.

@alexsanford alexsanford self-assigned this Feb 28, 2019

@alexsanford alexsanford requested review from jom and donnapep Feb 28, 2019

@jom

jom approved these changes Feb 28, 2019

Copy link
Member

left a comment

Nice sleuthing! Works well; looks good. I was able to reproduce it on release/2.0 and confirm this PR fixes it.

@lkraav
Copy link
Contributor

left a comment

Can we avoid if/else dances?

$parent_id = wp_is_post_revision( $lesson_id );

if ( $parent_id ) {
    $lesson_id = $parent_id;
}

$this->lesson_id = $lesson_id;
@alexsanford

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Can we avoid if/else dances?

@lkraav Not sure I have a really strong opinion either way, but I do try to avoid modifying incoming arguments (i.e. reassigning $lesson_id). I think the if/else is better for code clarity.

@alexsanford alexsanford merged commit cc0eebc into release/2.0 Mar 1, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@alexsanford alexsanford deleted the fix/lesson-ordering branch Mar 1, 2019

@lkraav

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

Can we avoid if/else dances?

@lkraav Not sure I have a really strong opinion either way, but I do try to avoid modifying incoming arguments (i.e. reassigning $lesson_id). I think the if/else is better for code clarity.

Hmm, I wonder if $this->lesson_id = wp_is_post_revision( $lesson_id ) ?: $lesson_id; is the ultimate solution here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.