DEV: Ignore bookmarks.topic_id column (PR #14289)

We don’t need no stinkin’ denormalization! This commit ignores the topic_id column on bookmarks, to be deleted at a later date. We don’t really need this column and it’s better to rely on the post.topic_id as the canonical topic_id for bookmarks, then we don’t need to remember to update both columns if the bookmarked post moves to another topic.

GitHub

I really am not sure why I did it this way before. Better to just keep the same deleted exclusion scope that everywhere else has by default.

This is necessary while the old query in update_bookmarks is not because if you move the OP of a topic into a new topic, the PostMover duplicates the post into a new topic, leaving the original post behind in the original topic. This query moves the bookmark over to the new post.

We don’t need to do any other updates for bookmarks in the post mover because the post ID of posts further down the topic do not change when moving to a new topic, and the bookmark table no longer has a topic ID to worry about.

Unrelated JS failures:

not ok 644 Chrome - [undefined ms] - error
    ---
        message: >
            Error: Browser timeout exceeded: 10s
            Error while executing test: Integration | Component | d-editor: replace-text event: selection spanning needle end becomes selection from replacement end
            Stderr: 
             [0913/013407.205285:ERROR:bus.cc(392)] Failed to connect to the bus: Failed to connect to socket /var/run/dbus/system_bus_socket: No such file or directory

Instead of delegation, perhaps an association might be clearer? belongs_to :topic, through: :post

Oh I did mean to do that, I think I just updated the topic has_many through association but not this one. I would like to keep the topic_id delegation though, otherwise there are more plugins I have to update, and I don’t see any reason not to use it.

Ah yes the topic_id one is fine :+1:

    if opts[:category_id]
      result = result
        .joins("INNER JOIN posts ON posts.id = bookmarks.post_id AND posts.deleted_at IS NULL")
        .joins("INNER JOIN topics ON topics.id = posts.topic_id AND topics.deleted_at IS NULL")
        .merge(Topic.in_category_and_subcategories(opts[:category_id]))
    end

The conditional at the end is a little surprising. How about the above instead? Also, I believe you can just do the following as well since we have a default scope on both Post and Topic.

    if opts[:category_id]
      result = result.joins(post: :topic).merge(Topic.in_category_and_subcategories(opts[:category_id]))
    end

We need to mark the column as readonly which helps to keep ensure that no one else is writing into this column . We have a helper for this Migration::ColumnDropper.mark_readonly(table_name, column_name)

technically we can just call topic_id since we set up the delegation in the bookmark model.

Same here, there is a delegation for topic_id

      bookmark1.update!(name: nil, reminder_at: 1.day.from_now)
      bookmark2.update!(name: "Some bookmark note", reminder_at: 1.week.from_now)

Ah yes I forget about these ColumnDropper helpers, thanks.

You’re right on both counts, good call.

Nooo I thought I caught all of these, I knew you would be on the lookout XD

@tgxworld I’ve done the first pass of those review fixes, LMK if there are more.

Just curious, do we need to join here? I think eager loading is sufficient for the use case here.

  def reminder_at_ics(offset: 0)
    (reminder_at + offset).strftime(I18n.t("datetime_formats.formats.calendar_ics"))
  end

How about this instead? I don’t think we expect anyone to pass in a nil?

          .includes(:topic)

I think we can do this now?

A few more minor comments but nothing blocking. LGTM