Remove special case notifying author of threads

[MoshiKoi]

Instead of special casing creating a notification, we just have them follow normally automatically. Resolves #1025

[MoshiKoi]

By the way (I forgot to mention), this does mean that everyone will no longer be following new threads on their posts since this change isn't retroactive. They'd have to actually click the follow new threads button on their posts. Someone could presumably make a one-off migration script somewhere to fix this, I'm not confident enough to do so myself.

[cellio]

By the way (I forgot to mention), this does mean that everyone will no longer be following new threads on their posts since this change isn't retroactive. They'd have to actually click the follow new threads button on their posts. Someone could presumably make a one-off migration script somewhere to fix this, I'm not confident enough to do so myself.

Oh, that would be bad; we don't want to quietly cancel notifications on older posts. I don't know how to write the migration script either, but the other way to do it would be to keep the extra check until we do -- so do make the change where we implement notifications as auto-follows on new posts, but in the code that decides whether to notify, keep the check for "is my post" along with "is followed" until we can fix existing follows.

[trichoplax]

Problems discovered during testing

I also don't know how to write the database script, but happy to work together on learning how once we know what we're aiming for. Before that, I've found some odd behaviour.

I've tested on this new branch and also on develop for comparison. The problems seem to stem from existing problems that predate this new branch. I don't understand the findings, but I'm sharing them here as they suggest there is more happening than expected from reading the changed code.

I checked the contents of the thread_followers database table at each step. I've noted what changed but I haven't established which part of the codebase causes these changes.

Summary of the problems

• On develop:
  • Pressing "unfollow new" on your own post does not prevent notifications for new threads.
  • Pressing "follow new" on your own post causes a duplicate row in the database. As a result you have to press "unfollow" twice on a thread before it takes effect.
• On this new branch:
  • Pressing "unfollow new" on your own post does not prevent notifications for new threads (it only prevents a notification for the initial comment of a new thread - notifications are still received for subsequent comments in that thread).
  • Pressing "follow new" on your own post causes a duplicate row in the database. As a result you have to press "unfollow" twice on a thread before it takes effect (Same problem as on develop except now it applies to a new post immediately as the post now defaults to the post author following new threads).

Tests on develop

(Differences from the new branch in bold.)

Test 1

• UserA posts a new question.
• No new rows in thread_followers.
• UserB posts a new thread.
• 2 new rows appear in thread_followers, 1 for each of UserA and UserB, each with null post_id and non-null comment_thread_id.
• UserA receives a notification.

Test 2

• UserA presses "follow new" on the question (which they posted in the previous test).
• 1 new row is added to thread_followers for UserA, with null comment_thread_id and non-null post_id.
• UserB posts a new thread.
• 1 new row appears in thread_followers for UserB, as before, but 2 new rows appear for UserA.
• UserA still only receives 1 notification (no unexpected behaviour from the user perspective).

Test 3

• UserA presses "unfollow new" on the question.
• The previously added row in thread_followers for UserA with non-null post_id is hard deleted.
• UserB posts a new thread.
• 2 new rows appear in thread_followers, 1 for each of UserA and UserB, as in test 1.
• UserA receives a notification, as in test 1.

Test 4

• UserA opens the thread from test 2 for which UserA has 2 identical rows in the thread_followers table.
• UserA presses "unfollow" for this thread.
• Instead of changing to "follow", it remains "unfollow".
• In the thread_followers table, the first (smallest id) of the 2 rows is hard deleted.
• UserA presses "unfollow" again.
• This time it changes to "follow" as expected.
• In the thread_followers table, the second of the 2 rows is hard deleted, so the table no longer contains any rows to cause notifications for UserA for this thread.

The same tests on this new branch

(Differences from develop in bold.)

Test 1

• UserA posts a new question.
• 1 new row is added to thread_followers for UserA, with null comment_thread_id and non-null post_id.
• UserB posts a new thread.
• 1 new row appears in thread_followers for UserB, but 2 new rows appear for UserA, each with null post_id and non-null comment_thread_id.
• UserA receives a notification.

Test 3

(Order switched with test 2 to align with the tests on develop, since on this branch new questions start out with the post author already following new threads.)

• UserA presses "unfollow new" on the question (which they posted in the previous test).
• The previously added row in thread_followers for UserA with non-null post_id is hard deleted.
• UserB posts a new thread.
• 2 new rows appear in thread_followers, 1 for each of UserA and UserB.
• UserA does not receive a notification.
• UserA does show as being a follower (the thread page shows "unfollow").
• When UserB posts a further message in the same thread, UserA receives a notification.

Test 2

(Order switched with test 3 to align with the tests on develop, since on this branch new questions start out with the post author following new threads.)

• UserA presses "follow new" on the question.
• 1 new row is added to thread_followers for UserA, with null comment_thread_id and non-null post_id.
• UserB posts a new thread.
• 1 new row appears in thread_followers for UserB, as before, but 2 new rows appear for UserA.
• UserA still only receives 1 notification (no unexpected behaviour from the user perspective).

Test 4

• UserA opens the thread from test 2 for which UserA has 2 identical rows in the thread_followers table.
• UserA presses "unfollow" for this thread.
• Instead of changing to "follow", it remains "unfollow".
• In the thread_followers table, the first (smallest id) of the 2 rows is hard deleted.
• UserA presses "unfollow" again.
• This time it changes to "follow" as expected.
• In the thread_followers table, the second of the 2 rows is hard deleted, so the table no longer contains any rows to cause notifications for UserA for this thread.

Thoughts

Although these problems are mostly pre-existing (not caused by this branch) we should probably ensure we understand them well enough to fix them before we merge a pull request that requires adjusting the database. We need to understand well enough to ensure our database script takes into account the unintended state of some of the historical data.

Should we aim to put the database in a correct state and have the code trust that the database is always as expected in future, or should the code take into account that the database may not always be in the expected state (as a failsafe)? For example:

• When unfollowing a thread the code could ensure all matching rows are deleted rather than the current approach of assuming there is only one row and just removing the first row found.
• When following a thread the code could add a row only if none already exist.

This might help protect against any bugs we have not yet discovered, and any historical data problems we haven't yet spotted.

I'm not looking to make a case either way, just wondering which approach people would prefer.

[MoshiKoi]

Sorry it took so long to address those issues! Life (and accidental database corruption) got in the way. The last commit should fix them (apparently there comment following logic is spread out over all three of PostsController, CommentsController and the CommentThread model, which made it hard to track down where the ThreadFollowers were being created

Test 1

• UserA posts a new question.
• 1 new row is added to thread_followers for UserA, with null comment_thread_id and non-null post_id.
• UserB posts a new thread.
• 1 new row appears in thread_followers for each of UserB and UserA, each with null post_id and non-null comment_thread_id.
• UserA receives a notification.

Test 3

• UserA presses "unfollow new" on the question (which they posted in the previous test).
• The previously added row in thread_followers for UserA with non-null post_id is hard deleted.
• UserB posts a new thread.
• 1 new rows appear in thread_followers, UserB.
• UserA does not receive a notification.
• UserA does not show as being a follower (the thread page shows "follow").
• When UserB posts a further message in the same thread, UserA does not receive a notification.

Test 2

• UserA presses "follow new" on the question.
• 1 new row is added to thread_followers for UserA, with null comment_thread_id and non-null post_id.
• UserB posts a new thread.
• 1 new row appears in thread_followers for each of UserB and UserA.
• UserA still only receives 1 notification (no unexpected behaviour from the user perspective).

Test 4

• UserA opens the thread from test 2
• UserA presses "unfollow" for this thread.
• It changes to "follow"
• the table no longer contains any rows to cause notifications for UserA for this thread.

[trichoplax]

Thanks for fixing all the problems!

Please don't apologise for life getting in the way - life comes first. I'm not just saying that because of being distracted myself recently - it's my view in general.

I don't have access to approve pull requests but it's looking good to me. Is the database script you mentioned previously the only thing remaining now? I'm guessing it would need to do the following:

• Make the author of every post that can have comments a follower of new threads

It looks like there are some one-off scripts in db/scripts and some of them are .rb files and others are .sql files. I have no experience of writing database scripts in Ruby but happy to have a go in SQL if you'd like a hand. I think writing it should be straightforward but I don't know how long it will take to run - maybe someone with access could test it on a backup of the live database to make sure it's going to complete quickly enough.

[cellio]

We could set this up on the dev server for testing when the DB-update stuff is ready.

[Taeir]

@MoshiKoi So users need to follow their own posts now, that would be
rails generate migration FollowOwnPosts
on the command line. This will generate a new file like db/migrate/<timestamp>_follow_own_posts.rb which will be initially mostly empty.

Then modify this file and change the change method as follows:

  def change
    to_insert = Post.where.not(post_type_id: [PolicyDoc.post_type_id, HelpDoc.post_type_id])
                    .where.not(user_id: nil)
                    .pluck(:id, :user_id)
                    .map { |post_id, user_id| { post_id: post_id, user_id: user_id } }
    ThreadFollower.insert_all(to_insert)
  end

This will transform each post into an entry { user_id: <post_author_id>, post_id: <post_id> }, which will be bulk inserted as ThreadFollowers using insert_all. Performance wise, this should be about as good as we can do. It will run the following two queries:

SELECT id, user_id FROM posts WHERE post_type_id IN (?, ?) AND user_id IS NOT NULL;
INSERT INTO thread_followers (post_id, user_id, created_at, updated_at) VALUES (?, ?, CURRENT_TIMESTAMP(), CURRENT_TIMESTAMP()), (?, ?, CURRENT_TIMESTAMP(), CURRENT_TIMESTAMP()), ...

In rails, it avoids constructing all the post objects, instead constructing only an array of the ids and user ids, which is then transformed into an array of hashes to be able to be passed to insert_all.

[cellio]

@Taeir will that migration preserve the following for existing posts? (Do I understand that correctly?)

@MoshiKoi , does the new code auto-follow new threads on your own posts (at post-creation time)?

[trichoplax]

I'm looking at this pull request because it affects comment threads, which are also going to be changed by #767. This makes me want to get this pull request merged first in order to fix the bugs addressed by this pull request before changing how comment threads work.

My understanding

@Taeir will that migration preserve the following for existing posts? (Do I understand that correctly?)

@cellio the migration will make every post author a follower of every one of their own posts. This initially sounds like it will destroy information (an author who had unfollowed one of their own posts would now be made a follower again). However, in testing the develop branch I cannot find any way for a post author to successfully unfollow their own post. Clicking "Unfollow new" under your own post currently changes the state of the button, but will not prevent you from being notified of new threads on your post. This is a bug, which this pull request will fix.

@MoshiKoi , does the new code auto-follow new threads on your own posts (at post-creation time)?

@cellio yes, once this pull request is merged, your own future posts will default to following all new threads.

In summary:

• The database migration will make you a thread follower of all of your own existing posts.
• The code changes will make you a thread follower of all of your own future posts.
• You will then be free to unfollow any of your own existing or future posts.

Can we test this on the dev server?

I believe this branch is ready to be tested on the dev server. Can this be done without deleting the existing dev server database so that the migration can be tested on the existing data?

The database migration script currently only exists in @Taeir 's comment, so it would need to be generated following the instructions in that comment before this could be tested on the dev server. I'm happy to generate it myself but will wait for any replies before proceeding.

Technically data corruption

I'm not concerned about this personally, but I'd like to describe what I expect to happen in case anyone objects to this, or believes the outcome will be different than I expect.

• The database migration will add a row to the thread_followers table for every post, marking the post author as a thread follower for that post.
• For most posts this will result in the existence of exactly one row in the thread_followers table, as intended.
• For any post that already has one or more rows in the thread_followers table for the post author, this will add an additional row, which will be redundant.

The reason that some posts may already have one or more rows in the thread_followers table is that pressing "Follow new" under your post will add a row to this table (even though currently the bug prevents it having any effect). Currently, pressing "Unfollow new" will only remove one row from the table, even if several exist. This pull request fixes this, so pressing "Follow new" or "Unfollow new" in future will result in exactly 1 row or exactly 0 rows, respectively.

I am not aware of any negative effect of having these duplicate rows (which likely already exist in the live database due to the bug). Before merging this pull request they are ignored, and after merging this pull request they will have the intended effect even when duplicated, and will have the duplication fixed if ever the post author presses "Unfollow new".

Testing the duplicate rows

We could test that there is no negative effect by pressing "Follow new" or "Unfollow new" on a few posts on the dev server before the database migration script is run.

Removing the duplicate rows

Alternatively, we could add a step to the migration before inserting the new rows. This could remove all existing rows for which the user who is following a post is also the post author, so that we start with zero rows for every post for its post author, and end up with no duplication and no way for future duplication to be introduced. The reason I'm not pushing for this is that I expect the total number of existing rows to be very small (they should only exist where the post author has pressed the "Follow new" or "Unfollow new" buttons).

I'm not proposing an approach where we only add new rows if none exist already, as that would not solve the problem of existing rows that are already duplicates (more than 1 exists already).

[cellio]

@trichoplax , that all sounds good to me. I don't feel qualified to sign off on the "technically data corruption" section -- sounds fine to me and I'm twitchy about data deletion, but I don't know if this duplication causes any issues -- and defer to @ArtOfCode- .

One additional thing for the migration script: do we need to do anything special to account for deleted post authors? Deleted users still exist in the users table, so adding them to the followers table anyway seems like it shouldn't break anything. It feels like we should do that for consistency, and against the extremely-unlikely-but-not-impossible case that someday we undelete a deleted user. But if that's going to cause problems, we might want to skip them and our hypothetical undeleted user comes back without follows and we live with that edge case.

[trichoplax]

One additional thing for the migration script: do we need to do anything special to account for deleted post authors? Deleted users still exist in the users table, so adding them to the followers table anyway seems like it shouldn't break anything. It feels like we should do that for consistency, and against the extremely-unlikely-but-not-impossible case that someday we undelete a deleted user. But if that's going to cause problems, we might want to skip them and our hypothetical undeleted user comes back without follows and we live with that edge case.

At present, the migration script is including deleted posts but excluding posts for which the user is nil. I don't know why a post would have its user set to nil, or whether any such posts exist at present. My only guess is that this might correspond to destroyed (rather than just deleted) users.

For deleted (rather than destroyed) users, your thinking sounds reasonable to me (having a follower row for a deleted user shouldn't break anything, and we'd want the row to be present if ever we undeleted the user).

If anyone is aware of a problem that would be caused by a deleted user being a follower of their own post, then this will likely also be a problem for the code (not just the migration script), since every user that is deleted in future will already be a follower of all of their own posts (if this pull request has been merged by that point).

[cellio]

At present, the migration script is including deleted posts but excluding posts for which the user is nil.

Deleted posts, or posts whose users are deleted?

Migrating posts with deleted (but non-nil) authors sounds like the right thing to me. I think users who are actually destroyed are nil, and we definitely have some of those in prod (and other instances might), so we want to exclude those. While we're removing "destroy" from the UI in the mod-tools branch, we might still occasionally need to do that (directly in the DB) to comply with data-privacy laws, so our code should assume that it can happen.

[trichoplax]

At present, the migration script is including deleted posts but excluding posts for which the user is nil.

Deleted posts, or posts whose users are deleted?

Good catch - I meant posts whose users are deleted. That sentence should have said:

At present, the migration script is including posts for which the user is deleted but still has a user_id, but excluding posts for which the user is nil.