FEATURE: Publish read state on group messages. (Originally introduced in #7989) (PR #8025)

GitHub

You’ve signed the CLA, romanrizzi. Thank you! This pull request is ready for review.

read_by_member does not sound right to me.

We should be storing the post_number that we are up to. I get the appeal of topic_allowed_groups here, but I think another table makes more sense here.

Non fancy implementation is

TopicGroup (to match TopicUser naming wise)

topic_id, group_id, last_read_post_number

Fancy implementation would maybe have a vector of user -> post_number in a column for top N. But this is way too complex for first iteration.

1 Like

@SamSaffron I just made the changes to pre-calculate last_read_post_number in the new topic_groups table.

Fancy implementation would maybe have a vector of user -> post_number in a column for top N. But this is way too complex for first iteration.

Why this approach would be better?

1 Like

If we ever want to display who in the group is up to where, we do not need that for now

1 Like

Can you upload a few screenshots? Overall the approach here looks right and code looks nice and well tested.

I would wait till monday to merge, but first thing on monday when you start your day maybe merge it? Then gather feedback from community team during your day.

hmmm so this is starting blank? Why not populate it in the migration?

I am mixed here, but it feel like there should be a site setting here to disable this? @coding-horror ? Should this be default on? (read indicator)

Since the feature is disabled by default, the data we insert could become outdated quickly.

This pull request has been mentioned on Discourse Meta. There might be relevant details there: