Note subscriptions API

[AntonKhorev]

Part 4 of #5283

Adds API endpoints to subscribe/unsubscribe from a note.

This is like the changeset subscribe/unsubscribe
https://wiki.openstreetmap.org/wiki/API_v0.6#Subscribe:_POST_/api/0.6/changeset/#id/subscribe
but since we like resourceful routes, instead of POST .../subscribe and POST .../unsubscribe, this PR adds:

• POST /api/0.6/notes/:id/subscription to add a subscription
• DELETE /api/0.6/notes/:id/subscription to remove a subscription

[gravitystorm]

Looks good to me. Only two small things:

• PR contains a fixup commit (i.e. a commit that changes code that itself is added by this PR)
• I'm not sure if the error messages should come from the translation system. Other usage of report_error is inconsistent whether error strings are translated or not.

[AntonKhorev]

PR contains a fixup commit

Let me guess what's a fixup here. The fact that looking up the note is not required in destroy if there aren't two different error messages (note not found or not subscribed)? Now I changed it to always look up the note. Is there still a fixup?

[gravitystorm]

No, that's not what I meant. A "fixup commit" is a commit that's added to a pull request that fixes problems with code introduced in the same pull request.

From CONTRIBUTING.md (emphasis added)

Avoid including "fixup" commits. If you have added a fixup commit (for example to fix a rubocop warning, or because you changed your own new code) please combine the fixup commit into the commit that introduced the problem. git rebase -i is very useful for this.

In this PR, the first commit includes:

+    rescue ActiveRecord::RecordNotUnique
+     head :conflict

The third commit includes

   rescue ActiveRecord::RecordNotUnique
-      head :conflict
+      report_error "You are already subscribed to note #{note_id}.", :conflict

Since the head :conflict is added and then removed, the PR should be rebased so that that code isn't mentioned in the history (for example, when using git blame.

[AntonKhorev]

There weren't any problems with the code. Not having an error message is not a problem. You could as well have said that not adding destroy together with create is a problem because it requires changing a.k.a. fixing routes and abilities.

[AntonKhorev]

Changed to have error messages right away.

The original version had response bodies intentionally undefined, and they are still intentionally undefined for ok responses.

[gravitystorm]

Merged, thanks for the updated version.

I'm interested to hear any thoughts on the translations:

"I'm not sure if the error messages should come from the translation system. Other usage of report_error is inconsistent whether error strings are translated or not."

We can always refactor this separately, since it's not just this code that would be affected.

[AntonKhorev]

Most of error messages are not translated. report_error receives many from exceptions defined in lib/osm.rb, no translations there.

[nenad-vujicic]

... A "fixup commit" is a commit that's added to a pull request that fixes problems with code introduced in the same pull request.

From CONTRIBUTING.md (emphasis added)

Avoid including "fixup" commits ...

Is catching "fixup" commits (a line added (removed) in one commit and then removed (added) in a subsequent commit within the same PR) using a danger workflow something that could help maintainers? It’s a bit tricky to define what constitutes a "fixup" because, if a commit represents a logical unit, a line could have meaning in one commit but not necessarily in another, thus justifying its removal.

[AntonKhorev]

Is catching "fixup" commits ... could help maintainers?

You can't do that automatically.