tests for collection/add and collection/remove

[prabalsingh24]

Problem

tests for POST collection/collectionId/add

[coveralls]

Coverage increased (+0.3%) to 58.45% when pulling bd60964 on prabalsingh24:test-add/remove-items into d1fd35b on bookbrainz:UserCollection.

[MonkeyDo]

OK, this PR is pretty smooth !

A couple of details below, but this is nice and tight !

[MonkeyDo]

Looking great now, well done !
I'm liking the progress on the testing!

[MonkeyDo]

I approved too fast…sorry!
After some thinking here are a couple more edge cases we want to test for; what happens when I:

• add or remove an invalid BBID
• add or remove an entity that doesn't exist
• add an entity of the wrong type for that collection

These 5 tests are currently missing, and raise some questions about the routes' implementation (that's why we love tests! :p).

• From what I can tell, I get a generic 500 server error when I try to add a non-existent or invalid BBID, where I would probably want a more specific "BBID XXXXX does not exist"
• Same for removing, I would probably expect an "BBID XXXX is not in the collection" type of error
• For invalid BBIDs I'd expect an "Invalid BBID" error message (see makeEntityLoader for the check and the error
• I can currently successfully add an author to an edition collection by POSTing to the /add route (not from the website), we want to add an entity type check. Adding works fine, but then an error will be thrown if I tried to visit that collection.

[prabalsingh24]

@MonkeyDo Have a look at this :)

[MonkeyDo]

We're making this more and more solid by the minute, which is great :)
Think of all the errors that could have been thrown in the future which will now be caught and explained to the user… Much less work downstream !

Most of the suggestions below are regarding error message wording.
The general rules are:

• make it clear it's an error
• make the error unambiguous (no way you could misinterpret what's happening)
• give a hint for how to solve the issue, especially if the cause is not completely obvious ('Invalid BBID' is quite obvious for example)
• If there are multiple entries and one fails, let the user know which one ('Invalid BBID: ${bbid}')
• Ask yourself: "as a programmer, if I got this error, would I understand what it's about and how to fix it ?"

[MonkeyDo]

Same on this good PR, one final detail and ready to merge :)

Nice testing !

[MonkeyDo]

🚀 !