FEATURE: Promote bookmarks with reminders to core functionality (PR #9369)

  • Remove logic based on whether enable_bookmarks_with_reminders is true. This site setting is now obsolete, the old bookmark functionality is being removed.

GitHub

This is a breaking change in that it removes the old web API for updating bookmarks. Have we done any research to see if any plugins / third party services are using the old API? In this case it doesn’t even see to be deprecated, it’s simply removed.

for the record I prefer the original format here. One problem with lining up is that if the original line changes in length you have to move all subsequent lines which pollutes the history. This is a bikeshed though.

Can you use manual SQL here? Using AR in migrations leads to many problems down the road.

I think in the case of bookmarks the old feature was so useless that break it and see who complains is the right choice.

On Mon, Apr 20, 2020 at 9:35 AM Robin Ward notifications@github.com wrote:

@eviltrout commented on this pull request.

This is a breaking change in that it removes the old web API for updating bookmarks. Have we done any research to see if any plugins / third party services are using the old API? In this case it doesn’t even see to be deprecated, it’s simply removed.

In app/controllers/topics_controller.rb https://github.com/discourse/discourse/pull/9369#discussion_r411520452:

@@ -232,10 +232,10 @@ def posts fetch_topic_view(options)

 render_json_dump(TopicViewPostsSerializer.new(@topic_view,
  •  scope: guardian,
    
  •  root: false,
    
  •  include_raw: !!params[:include_raw]
    
  • ))
  •                                              scope: guardian,
    

for the record I prefer the original format here. One problem with lining up is that if the original line changes in length you have to move all subsequent lines which pollutes the history. This is a bikeshed though.

In db/migrate/20200409033412_create_bookmarks_from_post_action_bookmarks.rb https://github.com/discourse/discourse/pull/9369#discussion_r411521743:

@@ -0,0 +1,61 @@ +# frozen_string_literal: true

+class CreateBookmarksFromPostActionBookmarks < ActiveRecord::Migration[6.0]

  • def up
  • SiteSetting.enable_bookmarks_with_reminders = true
  • post_action_bookmarks = PostAction

Can you use manual SQL here? Using AR in migrations leads to many problems down the road.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/discourse/discourse/pull/9369#pullrequestreview-396619219, or unsubscribe https://github.com/notifications/unsubscribe-auth/AALTWVNX77TEAFHN4HYSL23RNR2TNANCNFSM4MC2HTNQ .

Ah I think my vim autoformatter did this, I agree with you that the original was better

@eviltrout you are right, I wrote this as a rake task originally and moved it into a migration but didn’t consider the AR issue. I will change this.

@coding-horror @eviltrout I will just remove the old API and see who complains as Jeff suggests. The new endpoints in bookmarks_controller.rb can be used in their place, if there is something special I need to do to make an endpoint work for API keys I will do that otherwise just leave it.

Looks good now!