Is `topic_users.last_visited_at` supposed to be updated on every visit?

DeanMarkTaylor · April 7, 2016, 10:30pm

I’m pretty sure I’m ready to call this a bug.

I’ve been trying to find out where last_visited_at is updated so far I have found:

The spec successfully tests a call to TopicUser::track_visit updates the value.
TopicUser::change - change in notification level watching/tracking etc)
TopicUser::update_last_read - keeps track of the last read post.

The problem seems to be:

There are no calls to TopicUser::track_visit outside of the spec.
TopicUser::update_last_read only updates the last_visited_at value when:

“The user read at least one post in a topic that they haven’t viewed before”

I’m thinking that the following should be reviewed:

1. Should there be an explicit call when track_view is set when requesting the topic? I think there should be.
2. Change the UPDATE_TOPIC_USER_SQL to also update last_visited_at time as used in TopicUser::update_last_read so the value is kept up to date along with reading times.

github.com/discourse/discourse

app/models/topic_user.rb

8f4bc2228


      
          UPDATE_TOPIC_USER_SQL = "UPDATE topic_users
                                          SET
                                            last_read_post_number = GREATEST(:post_number, tu.last_read_post_number),
                                            highest_seen_post_number = t.highest_post_number,
                                            total_msecs_viewed = LEAST(tu.total_msecs_viewed + :msecs,86400000),
                                            notification_level =

3. There is a possibility that last_visited_at and first_visited_at are being set to NULL here when TopicUser::change tracking state changes:

github.com/discourse/discourse

app/models/topic_user.rb

8f4bc2228


      
          def change(user_id, topic_id, attrs)
            # Sometimes people pass objs instead of the ids. We can handle that.
            topic_id = topic_id.id if topic_id.is_a?(::Topic)
            user_id = user_id.id if user_id.is_a?(::User)
          
            topic_id = topic_id.to_i
            user_id = user_id.to_i
          
            TopicUser.transaction do
              attrs = attrs.dup
              if attrs[:notification_level]
                attrs[:notifications_changed_at] ||= DateTime.now
                attrs[:notifications_reason_id] ||= TopicUser.notification_reasons[:user_changed]
              end
              attrs_array = attrs.to_a
          
              attrs_sql = attrs_array.map { |t| "#{t[0]} = ?" }.join(", ")
              vals = attrs_array.map { |t| t[1] }
              rows = TopicUser.where(topic_id: topic_id, user_id: user_id).update_all([attrs_sql, *vals])

But I don’t know enough / have a test environment to check for sure.

4. If point 3 above is not where the NULL is coming from there is either TopicUser rows being put in the table with initially last_visited_at and first_visited_at set to NULL or somewhere else they are being set to NULL.

Thoughts?

Topic		Replies	Views
NULL first_visited_at dates in the topic_users table Development	5	2884	July 2, 2016
Last visit doesn't work for fixed topic list Bug	2	730	February 2, 2019
Already seen posts appearing above Last Visit line? Bug	5	883	January 9, 2025
Mysterious entries in topic_users table Bug	22	3969	May 16, 2016
Request from API doesn't update User#last_seen_at Product Support	8	989	April 15, 2019

Is `topic_users.last_visited_at` supposed to be updated on every visit?

Related topics