FIX: Bug in setting notification level when the post was moved (PR #9237)

When the post was moved from one topic into a new one, and the user with Automatically track topics I enter preferences, visits that topic for the first time notification_level was not modified.

GitHub

I think it’s quite unexpected that a method named find_xxxx does in fact mutate some attribute, unless Im mistaken.

It looks to me like it doesn’t modify any data - however there is a typo in the name. It’s missing a c

what about this line

https://github.com/discourse/discourse/pull/9237/files#diff-a453d5e1c29398203488ff879ca69664R226

first_visited_at … now? how does that make sense, are we 10000% track_visit! is only every called once?

1 Like

is this protected? cause the way it works just changed here

1 Like

I am not sure here, this is changing

update

  • if missing create

to

update

  • if existing … lookup
  • then update again

This is an extra query… I would prefer to keep the usual use case here as single query.

1 Like

This is a tricky part. TopicUser record are created in two cases. When the user visits the topic for the first time or when post which was already seen by the user is moved to another topic (then we create topic_user with notification level set as regular)

So logic we have here is

  • If there is no topic_user record, create one and while you are creating, check if the user wanted to automatically and immediately track topic

  • if there is already topic_user, without notifications_reason_id (so it was created when the post was moved and notification level was not changed by the user) - check if the user wanted to automatically track topic and do that for them.

I was thinking as well if we should set correct notification level when a post is moved and we create that topic_user. However, if the user was tracking the original topic, it doesn’t mean that they want to track automatically new topic to which post was moved to

I will try to solve without adding a new query

I changed that flow a little bit:

  1. I extracted logic which is finding notification level based on automatic track topic I enter and etc. to separate method called set_notification_level_attribute
  2. Then here: FIX: Bug in setting notification level when the post was moved by lis2 · Pull Request #9237 · discourse/discourse · GitHub We track topic_user and if that is not having notifications_reason_id (notification level was not chosen by the user) we try to assign automatic one. It added one additional query, previously it was just an update query, now we got select and update. It is in defer block so it should not affect users
    Scheduler::Defer.later "Track Visit" do
      TopicViewItem.add(topic_id, ip, user_id)
      TopicUser.track_visit!(topic_id, user_id) if track_visit
    end
  1. If notification level is changed we are sending information to frontend to properly update UI (same thing is happening if your settings are set to for example automatically track topic after 30 seconds)
notification_level_change(user_id, topic_id, attrs[:notification_level], attrs[:notifications_reason_id]) if attrs[:notification_level]

@SamSaffron - could you take another look? What do you think?

I am still super defensive of queries here cause this happens every time you visit a topic.

The change means we load the topic, then update it.

Previous implementation was that we update it… if update fails we create

So at a minimum it is 2 queries now and you pull a full model down.

I prefer that it just updates, if update fails, do a create cause usually you are updating cause you tend to visit topics twice or more.