Possibly a bug?

clarkwinkelmann

CyberGene the read status is stored as a combination of the discussion ID and the last read post number. The new reply state is computed based on the difference between the last read post number and the last post number.

So yes, I suspect deleting the last post of a discussion can mess this up, since it doesn't revert the unique post number counter for that discussion.

I'm not 100% sure about those permanent deletes, but I've definitely seen similar issues with posts that only some people can see, like posts under approval or soft-deleted posts. Since the user can't see them, those posts won't be marked as read and the user therefore can't have the full discussion marked as read until new posts become visible.

There's one "button" that's unaffected by this, it's the "Mark as read" control of a discussion that will always save the last post number as the last read number, even if that post is not visible. But that button is well hidden, it's not in the controls dropdown, it's what happens when you slide a discussion left or right from the discussion list on mobile.

CyberGene

clarkwinkelmann thanks for the explanation. So, if I understand correctly, there's some room for improvement when soft-deleting and hard-deleting posts (including auto-generated ones). Can this be fixed for a future version?

clarkwinkelmann

CyberGene yes it's probably something that will change in a future Flarum version.

I don't know if there has been any discussion yet as to what exactly would be a good fix.

I see 2 main solutions:

Always mark the discussion completely read when reaching the bottom, even if the number of the last post doesn't match with the last visible post.
- Pros: no database or performance changes, consistent with mobile "mark read" slide button
- Cons: if a post was under approval, when it's published it won't appear as a new unread post even if it's still the last post of the discussion
Add a new property "last public/visible post number" that's used instead of the absolute last post number
- Pros: better flexibility around posts that might become hidden or restored afterwards
- Cons: requires a new database column and additional events. Maybe the existing lastPost relationship could be used for this but that would impact performance when it comes to the is:unread gambit. We also probably can't rely on the guest visibility scope to find the last visible post because that would make read status completely broken in private discussions where no posts will ever be visible to guests.

luceos

Haven't read all of this. But I think the problem might lie in the new logic of setting post numbers from the database side. So if a post is deleted a new post might get the same number as a previous one. This assumes post read state table uses post numbers (which seems most logical).

A solution would be to update post read states on post deletion.

CyberGene

Thanks for the replies. Is a fix planned for this? Or would you like me to create a PR?

luceos

CyberGene @Sami SychO can you confirm my train of thought? If my assumption is correct CyberGene should be able to provide a PR. I think there's a Meta discussion updater that would be the best spot for this...

Sami SychO

luceos Yup the new logic of calculating the new post number would definitely have caused the first mentioned issue in this discussion (deleting last posts causes discussion to remain as read even with newer posts).

What needs to be done is to update the discussion_user table on post deletion, so that last_read_post_number of the discussion for any user where that value was equal to the deleted post number is lowered. And we should add an integration test for this as well.

Issues/PRs are welcome!

clarkwinkelmann

Sami SychO What needs to be done is to update the discussion_user table on post deletion

There's still the problem of soft-deleted posts and posts only visible to a subset of groups that will cause a similar issue.

Sami SychO

clarkwinkelmann the one you mention about a discussion remaining unread when there are new invisible posts to the user? not sure about that one, can't reproduce it, and as far as I can tell that's why there is last_post_number column, it is always equal to the last visible post to everyone, and it is used to determine read status.

clarkwinkelmann

Sami SychO ah yes, my bad. It seems like it's actually correctly handled already.

The other use case I was thinking of was when a visibility scope restricts access to a post that has is_private=false, but in that case the post would also end up being saved as the discussion's lastPost relationship so any extension that does such a thing should already employ custom code to redact the post from both places.

It would be nice if there was a visibility scope for extensions to impact the output of $discussion->comments() which is used to update both attributes/relationships.

One odd extension is my collapsible posts extension, where a post might be set as lastPost relationship but won't be visible in the discussion until the group of posts is expanded, so I assume the discussion won't be marked as read until this is done. But it should also be less common to collapse the end of a discussion. In any case that's something that could be solved in the extension itself if needed. Just an interesting use case.

Sami SychO

clarkwinkelmann ah yeah, that's an interesting case scenario 🤔