feat: move subscription to merged discussion #50
Conversation
| }) | ||
| ->update([ | ||
| 'discussion_id' => $discussion->id, | ||
| 'last_read_post_number' => $discussion->last_post_number, |
There was a problem hiding this comment.
Does this mean the new merged discussion will be marked completely as read if any of the merged ones was being followed by the user? Even if the user hadn't read any of the posts involved in the merge?
There was a problem hiding this comment.
Yea, it only applies if they've never read the target discussion though, setting the number to 1 might be better in retrospect.
There was a problem hiding this comment.
I think i'd rather set it to the last post number where the user has read all previous posts - but that requires either checking old & new post numbers or created times. More complicated. And not limited to flarum/subscriptions either.
There was a problem hiding this comment.
I'm not sure that it's doable, post numbers change when merging from discussion A, B, C to discussion Y. Which post number to pick from A B C and how to pick it with its new value seems like a lot of complexity.
Also, since they haven't read discussion Y at all, setting a post number other than 1 means original posts they haven't read gets marked as read.
There was a problem hiding this comment.
Yeah, so what I described was keeping the original "last read post" references of each discussion and comparing their new post numbers, picking the smallest one as the one to save in the new discussion. This way it'd resume them at the point where new posts appear that they may have not read, with the ones they definitely had before them.
Eg.
| Original Discussion ID | Last Read Post's Old Number | Last Read Post's New Number |
|---|---|---|
| 1 | 4 | 6 |
| 2 | 1 | 3 |
| 3 | 1 | 8 |
We know they must've read posts #1-3 at least in the merged discussion, so we can set last post read number to 3. This doesn't necessarily mean the next post will not have been read, but it guarantees the ones beforehand were.
Of course, this could be also done by calculating which post is first encountered in the new numbering that wasn't read, but that seems more consuming.
Or we can do none of this cause It's not worth it 🤷. Not in the scope of this PR anyways.
I did start second guessing myself while I wrote this 😅 but I think what I wrote is correct.
There was a problem hiding this comment.
Hmm, thinking back again maybe it's too much to introduce this PR logic in the extension, might be better to keep it simple. How about just a MergingDiscussions event dispatched instead. That way it can be used for custom behavior without burdening current extension.
Agreed, after further thought, i think it'd be better to dispatch an appropriate event instead. |
Changes proposed in this pull request:
Confirmed
composer test).Required changes: