Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

@lis2
Copy link
Contributor

lis2 commented Mar 19, 2020

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.

@lis2 lis2 requested a review from gschlager Mar 19, 2020
app/models/topic_user.rb Outdated Show resolved Hide resolved
app/models/topic_user.rb Outdated Show resolved Hide resolved
@lis2 lis2 force-pushed the lis2:automatic-tracking-when-post-is-moved branch from bcb0764 to 6e7fd0f Mar 20, 2020
if rows == 0
change(user_id, topic_id, last_visited_at: now, first_visited_at: now)
end
change(user_id, topic_id, last_visited_at: now, first_visited_at: now)

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Mar 25, 2020

Member

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

))
end

def set_notification_level_attributes(user_id, topic_id, attrs)

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Mar 25, 2020

Member

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

@@ -131,6 +131,9 @@ def change(user_id, topic_id, attrs)

if rows == 0
create_missing_record(user_id, topic_id, attrs)
elsif topic_user = TopicUser.find_by(topic_id: topic_id, user_id: user_id, notifications_reason_id: nil, notifications_changed_at: nil)

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Mar 25, 2020

Member

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.

This comment has been minimized.

Copy link
@lis2

lis2 Mar 25, 2020

Author Contributor

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

…ne topic into another.

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.
@lis2 lis2 force-pushed the lis2:automatic-tracking-when-post-is-moved branch from 6e7fd0f to 0c93e73 Mar 26, 2020
@lis2 lis2 force-pushed the lis2:automatic-tracking-when-post-is-moved branch from 0c93e73 to a62c06a Mar 26, 2020
@lis2

This comment has been minimized.

Copy link
Contributor Author

lis2 commented Mar 26, 2020

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: https://github.com/discourse/discourse/pull/9237/files#diff-a453d5e1c29398203488ff879ca69664R175
    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?

@SamSaffron

This comment has been minimized.

Copy link
Member

SamSaffron commented Mar 26, 2020

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.

@lis2 lis2 closed this Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.