AO3-4359 Mass import bookmarks #2442

ariana-paris · 2016-04-28T15:08:18Z

https://otwarchive.atlassian.net/browse/AO3-4359

Adds bookmark importing capability to the mass import API. Open Doors sometimes import sites which include works that are links to stories on another site which is not being imported (and therefore hasn't granted permission to scrape in the story contents). This will allow those works to be imported as bookmarks on the Archive, referencing the story's currently location rather than importing it.

In order to achieve this, I have also:

refactored the existing code so that the authentication handling and work imports are in separate files
rationalised the API end points so that instead of api/v1/import, we now have api/v1/works and api/v1/bookmarks (paving the way for, say, collections, users or, idk, videos in future, whatever comes up next)
added an original_id field to all the API requests so the remote site can pass in its own id for the story/bookmark it's importing and use that to match up the Archive's response to the relevant item (currently, the OD temporary sites rely on the responses being in the same order as the requests which is risky)
converted routes.rb to use Ruby 1.9 hashes where possible

# Conflicts: # app/controllers/api/v1/import_controller.rb # spec/api/api_spec.rb

…into AO3-4359-Mass-import-bookmarks # Conflicts: # app/controllers/api/v1/bookmarks_controller.rb # app/controllers/api/v1/works_controller.rb # spec/api/api_spec.rb

… in date test

…I client side

houndci-bot · 2016-04-28T15:08:26Z

spec/models/story_parser_spec.rb

@@ -143,12 +136,22 @@ class StoryParser
  end

  describe "#download_and_parse_chapters_into_story" do
+


Extra empty line detected at block body beginning.

sarken · 2016-05-13T23:09:11Z

app/controllers/api/v1/bookmarks_controller.rb

+  # Set messages based on success and error flags
+  def response_message(messages)
+    if @some_success && @some_errors
+      messages << "At least one bookmark was not created. Please check the bookmark array for further information."


Will an archivist know what the "bookmark array" is? (I ask mostly because I'm not looking at the page and I don't know.)

Good point - that's an implementation detail that doesn't need to be in the message. The developer who implements the API will presumably turn the JSON array into something more user friendly, so I'll rephrase that as "individual bookmark messages".

…tails

shalott · 2016-05-24T00:28:27Z

app/controllers/api/v1/bookmarks_controller.rb

@@ -0,0 +1,117 @@
+class Api::V1::BookmarksController < Api::V1::BaseController


Shouldn't this be ExternalBookmarksController? We may well want to add API for regular bookmarks at some point

With all my focus on the Open Doors use case, I hadn't thought of the two bookmark types possibly being used in future - I'll rename this and have a think about whether the URL end-point needs to be changed as well. (i.e. would it still make sense to POST both external and internal bookmarks to api/v1/bookmarks, or should there be a separate api/v1/external-bookmarks, and if so what happens to GET requests when we get around to them? Should both types be returned by a call to bookmarks?)

Hmm, I was thinking about External Works, I realize now. And the actual bookmark object is the same, so all bookmark creation makes sense in BookmarksController, but where the process of creating external works belongs... IDK, probably it should be here after all and just made conditional on whether the url passed is for an AO3 work or not, that seems most parallel to the existing process since External Works controller just makes the new external work object and passes off the actual creation to the Bookmarks controller via the form.

As discussed previously, I've added some error handling. As a general rule, I like to delegate as much as possible to the back-end when writing an API, as the model should be performing essential data validation before anything goes into the database, and this theoretically ensures consistency between the UI and the API.

However, the ExternalWorks model doesn't check for the presence of a fandom and returns a weird message for a missing author, so I've intercepted some invalid requests in the API controller to return helpful messages from there instead, and added tests to check that this is happening.

…-4359-Mass-import-bookmarks

…and external URL

houndci-bot · 2016-05-25T12:35:38Z

app/controllers/api/v1/bookmarks_controller.rb

+      end
+    end
+
+


Extra blank line detected.

shalott · 2016-06-01T03:31:28Z

app/controllers/api/v1/bookmarks_controller.rb

+
+  def create
+    archivist = User.find_by_login(params[:archivist])
+    external_bookmarks = params[:bookmarks]


here is where I think the naming is confusing me -- these are just bookmarks and they happen to be of external works. Can we just call them bookmarks throughout? it looks to me like this code would actually work just fine for a NON external work too.

I'm not sure it would work for the other types as is, because the external_bookmark method in the API bookmark_controller coerces the JSON input into a structure with an external hash in it, which IIRC is the structure passed back by the normal bookmark_controller when creating a bookmark for an external work. I think this brings the external= virtual attribute in the Bookmark model into play.

Having said that, I didn't inspect the other bookmark types, and it's been a while since I unpicked how bookmarks are created, so you might well be right. I'll rename that variable to make it less external-centric which will make it easier to adapt this if we have a use case for creating other types of bookmarks in future.

houndci-bot · 2016-06-01T08:37:41Z

spec/api/api_bookmarks_spec.rb

+    it "should return 400 Bad Request if an invalid URL is specified" do
+      post "/api/v1/import",
+           { archivist: @user.login,
+             bookmarks: [ bookmark.merge!(url: "") ]


Space inside square brackets detected.

shalott · 2016-06-01T12:32:33Z

app/controllers/api/v1/bookmarks_controller.rb

+
+  # Returns a hash
+  def import_bookmark(archivist, params)
+    bookmark_request = external_bookmark(archivist, params)


Sorry to be nitpicky some more: it feels to me like even here it makes more sense to have this remain generic (eg "build_bookmark_request(user, params)") because the only place where it actually becomes specific to external works is in the currently named external_bookmark method and bookmark_errors (which is already named generically) -- and in those two places, I would suggest adding a comment explaining that for the moment this ONLY works for bookmarks for external works. There is basically no difference except if the "external" stuff is passed AFAIK.

…ExternalWork model

houndci-bot · 2016-06-05T21:40:42Z

spec/api/api_bookmarks_spec.rb

+      WebMock.reset!
+    end
+
+


Extra blank line detected.

shalott · 2016-06-10T22:38:21Z

Looks good to me!

ariana-paris added 25 commits October 25, 2015 16:53

AO3-4359 - Add plumbing for bookmark imports

37b8df0

AO3-4359 - Simplify return codes and import bookmarks

c889df4

AO3-4359 - Move works importing code into the works controller

d35995f

AO3-4359 - Handle items collection in incoming json

d26cd15

Merge branch 'master' into AO3-4359-Mass-import-bookmarks

d0aec91

# Conflicts: # app/controllers/api/v1/import_controller.rb # spec/api/api_spec.rb

AO3-4359 - Merge with master branch (WIP - broken tests)

6beecba

AO3-4359 - Add plumbing for bookmark imports

8f97f69

AO3-4359 - Simplify return codes and import bookmarks

43de8d8

AO3-4359 - Move works importing code into the works controller

2593b07

AO3-4359 - Handle items collection in incoming json

ce02679

AO3-4359 - Merge with master branch (WIP - broken tests)

065c23e

AO3-4359 - Document expected Json in code

8f3b4ca

AO3-4383 - Include title

e3cadb5

AO3-4359 - Attempt to correct the tests (WIP)

d69c22d

Merge remote-tracking branch 'origin/AO3-4359-Mass-import-bookmarks' …

52291e7

…into AO3-4359-Mass-import-bookmarks # Conflicts: # app/controllers/api/v1/bookmarks_controller.rb # app/controllers/api/v1/works_controller.rb # spec/api/api_spec.rb

AO3-4359 - Fix incomplete merge with master and related tests

de23cbe

AO3-4359 - Split out API specs for better clarity

e772b47

AO3-4359 - Reinstate change to deploy.rb overwritten by merge

4fde7e9

AO3-4359 - Fix local warning about errors and clamp down on timezones…

3b81147

… in date test

AO3-4359 - Reset mocks after use to better isolate tests

19b1fd3

AO3-4359 - Flatten request json to hide internal Archive details

25e04bc

AO3-4359 - Add original_ref field so remote ids can be used on the AP…

1805c56

…I client side

AO3-4359 - Rename original_ref to original_id

a398315

AO3-4359 - Remove debugging method

8e68a86

AO3-4359 - Refactor routes.rb to use Ruby 1.9 hashes

6de14d9

houndci-bot reviewed Apr 28, 2016
View reviewed changes

sarken reviewed May 13, 2016
View reviewed changes

AO3-4359 - Rephrase user-visible messages to remove implementation de…

4d37439

…tails

shalott reviewed May 24, 2016
View reviewed changes

ariana-paris added 3 commits May 24, 2016 19:53

Merge branch 'master' of git://github.com/otwcode/otwarchive into AO3…

5d7a3bb

…-4359-Mass-import-bookmarks

AO3-4359 - Remove pointless test

1e57672

AO3-4359 - Don't create duplicate bookmarks given the same archivist …

5a99e50

…and external URL

houndci-bot reviewed May 25, 2016
View reviewed changes

app/controllers/api/v1/bookmarks_controller.rb

end

end

Copy link

houndci-bot May 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra blank line detected.

ariana-paris added the Coder Has Actioned Review label May 25, 2016

shalott reviewed Jun 1, 2016
View reviewed changes

shalott added Reviewed: Action Needed and removed Coder Has Actioned Review labels Jun 1, 2016

AO3-4359 - Rename external_bookmark variable to be less specific

f833083

houndci-bot reviewed Jun 1, 2016
View reviewed changes

ariana-paris added Coder Has Actioned Review and removed Reviewed: Action Needed labels Jun 1, 2016

shalott reviewed Jun 1, 2016
View reviewed changes

shalott added Reviewed: Action Needed and removed Coder Has Actioned Review labels Jun 2, 2016

AO3-4359 - Add handling for common errors that aren't handled by the …

d631c11

…ExternalWork model

houndci-bot reviewed Jun 5, 2016
View reviewed changes

spec/api/api_bookmarks_spec.rb

WebMock.reset!

end

Copy link

houndci-bot Jun 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra blank line detected.

ariana-paris added Coder Has Actioned Review and removed Reviewed: Action Needed labels Jun 6, 2016

shalott added Reviewed: Ready to Merge and removed Coder Has Actioned Review labels Jun 10, 2016

sarken merged commit af3a2ec into otwcode:master Jul 10, 2016

sarken mentioned this pull request Jul 24, 2016

AO3-4359 Revert mass import bookmarks #2498

Merged

ariana-paris deleted the AO3-4359-Mass-import-bookmarks branch September 21, 2016 16:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AO3-4359 Mass import bookmarks #2442

AO3-4359 Mass import bookmarks #2442

ariana-paris commented Apr 28, 2016 •

edited

houndci-bot Apr 28, 2016

sarken May 13, 2016

ariana-paris May 15, 2016

shalott May 24, 2016

ariana-paris May 24, 2016

shalott May 24, 2016

ariana-paris Jun 6, 2016

houndci-bot May 25, 2016

shalott Jun 1, 2016

ariana-paris Jun 1, 2016

houndci-bot Jun 1, 2016

shalott Jun 1, 2016

houndci-bot Jun 5, 2016

shalott commented Jun 10, 2016

		@@ -143,12 +136,22 @@ class StoryParser
		end

		describe "#download_and_parse_chapters_into_story" do

		@@ -0,0 +1,117 @@
		class Api::V1::BookmarksController < Api::V1::BaseController

AO3-4359 Mass import bookmarks #2442

AO3-4359 Mass import bookmarks #2442

Conversation

ariana-paris commented Apr 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shalott commented Jun 10, 2016

ariana-paris commented Apr 28, 2016 •

edited