Extra empty line detected at block body beginning.

Use 2 spaces for indentation in an array, relative to the start of the line where the left square bracket is.

Extra blank line detected.

Extra empty line detected at module body beginning.

Use 2 (not 4) spaces for indentation.

Inconsistent indentation detected.

Inconsistent indentation detected.

Redundant curly braces around a hash parameter.
Space inside square brackets detected.
Space inside parentheses detected.

Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.

Space inside } missing.

Use the new Ruby 1.9 hash syntax.
Space inside square brackets detected.

Align the operands of an expression spanning multiple lines.

Use \ instead of + or << to concatenate those strings.
Space inside string interpolation detected.

Prefer map over collect.

Align the operands of an expression spanning multiple lines.

Really minor complaint -- can this be rephrased to "at one time"/"at once"?

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".

I'm confused by these two last specs. I assumed from the first one that duplicate bookmarks wouldn't be created, and the spec was therefore testing that it would return 200 even if only one bookmark were created. Then the second one seems to expect both to be created? In which case the first spec doesn't seem to be testing "when only some are created".

My gut feeling is an exact duplicate should ideally not be created? So bookmarks.count should be 1 not 2?

Good point about the "some" bookmarks spec. When I originally wrote the works controller, I returned different HTTP responses for these different states (all, none or some created). But as they're application errors, not HTTP errors, I now think 200 is the code to use. Anyway, I copy-pasted those into the bookmarks spec and forgot to clean up. I'll remove that test altogether.

The second one does expect duplicates to be created because there doesn't seem to be a de-duping mechanism in the Bookmark model. If I bookmark the same work multiple times, I get multiple bookmarks (even though only one is listed under the work, interestingly). This may indeed be a unintended flaw in the Bookmark model, but it feels as if fixing it is beyond the scope of this particular ticket which aims to reproduce the web behaviour in the API (though a separate one which fixes the model and this test sounds like a good idea!)

FWIW, my inclination would be it's more urgent to avoid creating the duplicates here because eg if an import fails halfway you would ideally like to be able to re-run without worrying about creating duplicate bookmarks?

Good point. I'll see if it's easy to do without delving too intimately into the Bookmark model - for example, by searching for a bookmark owned by the archivist which already has that URL (sounds easy enough but we'll see!)

Extra blank line detected.

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.

Space inside square brackets detected.

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.

and last question here: what kind of cleaning is happening on this user input and where? I might suggest also a test or two in the spec of submitting bad/malicious content to make sure it doesn't get passed through.