osmlab / to-fix-backend

The to-fix server
BSD 3-Clause "New" or "Revised" License
15 stars 13 forks source link

Use user OSM IDs for Comment Author Attribution #259

Open recursivefunk opened 6 years ago

recursivefunk commented 6 years ago

For comment creation, we're expecting a username to be specified for the comment author

https://github.com/osmlab/to-fix-backend/blob/master/routes/comment.js#L140 https://github.com/osmlab/to-fix-backend/blob/master/routes/comment.js#L151

But usernames are mutable in that they can be changed. If a user changes their username, and another user signs up with the first user's old username, it will appear as though the new user has made comments they had not made. We should use OSM IDs as user identifiers.

kepta commented 6 years ago

@recursivefunk I think the reason we have username instead of userId's is because of the lack of a user modal in tofix. To put it other way, the client or the server needs to do additional work (network requests) to figure out the id -> username mapping.

There are two ways one could address the problem, with added complexity :/

  1. We create a user modal and handle everything on the server side. But we would still have to rely on Openstreetmap to figure out what id corresponds to what username. Also, if we want to figure out the username's of N id's, we will have to make N network requests to the openstreetmap server. This will definitely not be great for OSM and user experience.

  2. We make the id -> username a client problem. Now this would keep the server simple and move the problem to the client. This is technical decision, I am happy to list out pro's and con's for doing additional network requests on the client side (tofix-internal).

recursivefunk commented 6 years ago

@kepta Good points. It might be helpful to know how often usernames change. If not very often, we could have a component that runs every few weeks to reconcile IDs and usernames. I like the idea of keeping this on the server. The client should be kept simple.

The bottom line is, we need a reliable mechanism to uniquely identify users in OSM over time if we ever hope to do real user management.

batpad commented 6 years ago

My two cents: storing usernames in the database is a definite mistake and should be changed / appropriate migrations written, and all current references to username should be changed to osm user id. This would include the createdBy field on Item for example: https://github.com/osmlab/to-fix-backend/blob/master/database/item.js#L35

I think the question of whether to maintain the id -> username mapping in the to-fix-backend db as a separate model, or rely on an external service for this and make it the client's problem is probably an interesting decision. This is possibly related to the larger question of handling user management more "properly" in the to-fix-backend.

If using an external service, one could use the osm-comments-api, for eg: https://osm-comments-api.mapbox.com/api/v1/users/id/1306 -although one might want to build a separate lightweight service for this if going down this path.

recursivefunk commented 6 years ago

To close the loop here, this work will fall into v2 of the API work