Add support of TODO tags

[asaunier]

In short: adds the possibility to tag routes as "TODO". The UI part would be a "TODO" button in route pages, a bit like the "Follow" button in user profiles.

I have tried to be a bit generic, in order to support other kind of tags later (eg. "done/tick", etc.).

TODO:

• unit tests
• use the new tag as a filter when searching for routes => would allow to list the routes tagged as "TODO" by a given user along with other existing criterias (eg. activities, regions, etc.)
• restrict access of TODOs to their owner only?
• add a Log table to record tagging/untagging of documents (needed for instance to update the ES index)
• add tests for DocumentTagLog
• update ES sync (routes tagged and untagged)
• handle case where documents are deleted
• handle case where documents are merged
• handle case where a user is anonymised
• add tests for merging

UI PR at c2corg/c2c_ui#1147

[cbeauchesne]

About genericity, it would be great to also allow tags on all document types.

From API point of view, tag system would be functionally agnostic, it's simply a unique user_id/document_id/tag_name keys store with search capabilities. And one tag name hasn't any special consequence on API from another one.

Then, on UI side, the way it use tag name describe the functionality.

I see some use cases for this genericity :

• So, TODO, obviously
• We also could have a follow list for contributors : having a way to monitor modifications made on a list of document, if /whatsnew page have a tag filter
• We also could implement a vote system for cotations, with a view /tags/<document_id>
• and a lot of other stuff, it would be very powerful

---

restrict access of TODOs to their owner only?

IMO, no :

• more complexity
• It's not compatible with the spirit of the project : outings are publics
• It will disallow some capabilities
• gut feeling, users don't care. And if they do, they can simply not use it for their very secret project.

[momomaniac]

About genericity, it would be great to also allow tags on all document types.

This sounds like a good idea, and seems easier to maintain while remaining open for further use cases.

restrict access of TODOs to their owner only?

I think a TODO list is user related data, it should be only visible to logged-in users (for example if the list is a section of the user profile). It would be nice though to leave the user the choice to make it public or private or visible to all other members (or maybe just to selected other users, tagging users as friends, as friends for climbing, skiing etc., sorry this is getting off topic)

[cbeauchesne]

Let's ask this point on the forum :)

https://forum.camptocamp.org/t/tick-list-privee-ou-publique/253028

[asaunier]

Hi @cbeauchesne @momomaniac

Thanks for the feedbacks! I would prefer this topic only discusses the technical aspects of the suggested feature. Using the forum would be more appropriate for debating possible evolutions and ethical issues.

Tags were a key feature that we wanted to add to v6 but had to dismiss because of lack of time and budget. We planned for instance predefined tags (eg. "todo"), either personal or global (for instance for routes with restricted access, etc.). Users would also be able to create their own personal/private tags as in Gmail for instance. There are probably more info about this in the "v6" forums or specs.
Both of you have also suggested very interesting uses for tags.

On the other hand I would like to keep this PR quite simple, at least for the initial version of the tool. First because I don't have much time for it and second because it could grow too big, increasing chances that the feature would not be delivered. In addition I think that the key feature (often asked for in the forum) is actually to add routes to one's todo list, other uses are more marginal I think.

I have decided to use a generic name "tag" instead of "todo", in order to easily be able to extend a bit the tool. For instance.

• "tick list": instead of creating an empty outing, people could marked the route as done (on a given date?) => we would need to add fields date and type (defaulting to todo) to table routes_tags
• a public todo list would be interesting to find team mates and plan a trip to such routes

I have the feeling that it is important that by default todo lists are private (personally I won't use the feature if any one can see my projects if I have not chosen to make them public).
Not sure it adds much complexity: when the "todo" parameter would be passed to a route search, the API would get the current user id and adds it to the search filters.

Making some todo items public could be done by adding public field to table routes_tags that would default to False, etc.
But again that's not in the extent of the current PR.

[cbeauchesne]

Hi @asaunier ,

The question on the forum was only about what wants the users (and not guessing it). The answer is clear :

• Most of them will use whatever it's private or not
• 15% -ish want it to be private

In consequence, privateness is a nice to have, just driven by the tech difficulties / our availabilities => If you want to implement it, feel free :)

[cbeauchesne]

Suggestion : IMO, having a unique REST service, leveraging on REST model would be better :

• GET /routes/tag/{route_id}=> returns {"todo": True/False}
• PUT /routes/tag/{route_id}=> , empty body as now, set the tag on (use PUT, not POST, as it's idempotent)
• DELETE /routes/tag/{route_id}=> , empty body as now, set the tag off

[asaunier]

@cbeauchesne Your suggestion makes sense but

• this code is inspired from https://github.com/c2corg/v6_api/blob/master/c2corg_api/views/user_follow.py which itself uses the same approach than other services
• I don't remember exactly but I think it is related to the way Cornice works

As a result I prefer to stick to this approach in order to keep code consistency and to avoid spending too much time using another approach whereas the existing one has proved it worked.

[cbeauchesne]

Anotheer suggestion : tag exists on article, and are linked to a single article.

As this new object is linked to a single Route/User, rename tag to UserTag :

• RouteUserTag for models
• routes_users_tags for table
• RouteUserTagRestfor view
• /routes/user_tag for URI

[asaunier]

I have made a few progress, adding some tests but am currently stuck because of a "405 Method Not Allowed" error when posting to the /routes/tag service. Don't know why because the code is almost the same than https://github.com/c2corg/v6_api/blob/master/c2corg_api/views/user_follow.py

https://cornice.readthedocs.io/en/latest/exhaustive_list.html says that

In cornice, one path equals one service. If you call a path with the wrong method, a 405 Method Not Allowed error will be thrown (and not a 404), like specified in the HTTP specification.

[momomaniac]

@asaunier : I will start working on the update of the SERAC database. I'll have to create a new alembic version (also based on master HEAD) Since the project has no alembic branches, I suggest we just have two individual alembic files and do the conflict resolution just before merging the respective branches. OK for you ?

[asaunier]

@momomaniac No problem: I will upgrade the alembic source version if your PR is merged before mine (which is quite likely :P)

[mikocot]

I have made a few progress, adding some tests but am currently stuck because of a "405 Method Not Allowed" error when posting to the /routes/tag service. Don't know why because the code is almost the same than https://github.com/c2corg/v6_api/blob/master/c2corg_api/views/user_follow.py

https://cornice.readthedocs.io/en/latest/exhaustive_list.html says that

In cornice, one path equals one service. If you call a path with the wrong method, a 405 Method Not Allowed error will be thrown (and not a 404), like specified in the HTTP specification.

Hey guys, what is the current status? Have you managed to overcome the 405 and move on? Not sure if I can help with this one , but maybe together we can push it forward :)

What I think may be a problem here is that /routes/tag actually hits /routes/{id} of route.py, which does not define post itself -> hence 405 error to a POST request.
On the other hand user.py doesn't have this issue as routes there are more elaborate (/users/register, /users/validate_new_password) etc.

The way around would be to change the route from /routes/tag to something like /routetags or just /tags.

[asaunier]

Hi @mikocot

what is the current status? Have you managed to overcome the 405 and move on?

I have made no progress on this task since my last report.

What I think may be a problem here is that /routes/tag actually hits /routes/{id} of route.py, which does not define post itself -> hence 405 error to a POST request.
On the other hand user.py doesn't have this issue as routes there are more elaborate (/users/register, /users/validate_new_password) etc.

That's a good hint! I have used the users view as a reference to write the new tag view. Perhaps I have missed something.

The way around would be to change the route from /routes/tag to something like /routetags or just /tags.

That's worth trying. I guess /tags is enough and more generic, in case we extend tags to all documents later.

[asaunier]

Note about how to run the tests for this part:

docker-compose exec api .build/venv/bin/nosetests -s  c2corg_api/tests/views/test_route_tag.py:TestRouteTagRest.test_tag

[cbeauchesne]

The way around would be to change the route from /routes/tag to something like /routetags or just /tags.

+1

Actually, there are a lot of usages with the possibility of linking documents to users profiles. The way to make things generic would be :

• Having a generic /usertags entry point, and leverage on HTTP model (GET, POST, DELETE)
• DB model would be :
• columns
  • user_id FOREIGN_KEY
  • document_id FOREIGN_KEY
  • tag_name STRING
  • tag_value STRING
• Unique key => user_id/document_id/tag_name
• Entry point would accept any of combinaison of user_id/document_id/tag_name arguments

Thus, as now, we actually do not need such a genericity, so first a first shoot, a simplier model would do the job. But if this model could be in that direction, it would be great :)

• Having a generic /usertags entry point, and leverage on HTTP model (GET, POST, DELETE)
• DB model would be :
  • columns
  • user_id FOREIGN_KEY
  • document_id FOREIGN_KEY
  • Unique key => user_id/document_id
• Entry point would only accept a mandatory user_id=<current_user_id> argument.

[asaunier]

Hi all

Thanks to the suggestion of @mikocot I have succeeded in making the tests pass: I have renamed the routes to tags/add instead of routes/tag that was indeed caught in the standard routes/{id} pattern.

I have finally decided to generalize a little the tool in order to tag whatever document type (not only routes) even though I will only deal with routes in this PR.

What is already possible now:

• tag a document (as "todo" as default => there is no other tag type in this PR)
• untag a document
• know if a document is tagged (as "todo")

What I would like to do still:

• add a criteria to filter routes that were tagged by a given user
• make this list private (ie. only accessible to the current user)

As for now there is no restriction on the kind of document you may tag (could be a waypoint or an article). Could it be a problem?
In addition there is no distinct tag type (implicitly set to "todo").

Edit: Travis fails (OK in my local instance though) but the failed test seems no related (is it?).

[cbeauchesne]

Edit: Travis fails (OK in my local instance though) but the failed test seems no related (is it?).

No, it's not related, outing1 and outing4 has the same elevation, that leads to a random result for sorting feature, and t make the test randomly fails.

Can you put 1499 instead of 1500 here : https://github.com/c2corg/v6_api/blob/master/c2corg_api/tests/views/test_outing.py#L1174 ?

ping @momomaniac ;)

As for now there is no restriction on the kind of document you may tag (could be a waypoint or an article). Could it be a problem?

IMO, no, we will restrict the usage with the UI. And thank's a lot for this move, it will offer a lot of possibilities (even if i do not have yet a good vision what will be really useful 😄 )

Is the PR ready for review ?

[asaunier]

@cbeauchesne Thanks for the hint 👍
I have pushed commit 44ea54e Hopefully it will fix the tests.

The PR is not really ready for review, I have a "codacy" issue to fix and I would like to add the possibility to filter routes on the "todo" tags. I'll work on this tonight I think.
But no problem if you want to start reviewing what's already in the PR.

[cbeauchesne]

You're welcome!

No, I'll wait for the final version, mid-work review are often a waste of time for both reviewer and worker :)

[cbeauchesne]

Sorry, I missed the "ready for review", I'll make a review today or tomorrow.

[asaunier]

Hi @cbeauchesne
Thanks, but don't worry, I had not asked for review yet, I wanted to write a small summary of the changes I did (and of things I have finally not done - yet?) and also to review my changes first to make sure everything seems in order. Hopefully I do this tonight, than no hurry :)

[asaunier]

Commands ported from https://github.com/c2corg/v6_api/wiki/Development-environment-on-Linux#run-the-application

Not 100% sure though, as I need to run them (or only run-syncer?) to have the tags actually taken into account in ES, but on the other hand after doing so the simple search no longer works in my instance (no more results)!?

[cbeauchesne]

First answer : run-syncer only is not enough

[cbeauchesne]

Side note, for this point, we need to put them in the wiki: the README must be the simplest possible help to run the dev env.

[asaunier]

Also tested manually in the UI with PR c2corg/c2c_ui#1147

I have realized that tags are correctly transfered when merging 2 documents but there is still a problem when merging 2 users:

• tags and tag_logs are correctly transfered (I have made a fix with 290763d to only recreate the last logs of actual document tags - not the ones that have been cancelled in the meantime)
• but the ES indexing is not restarted as expected => the DB is updated but not the ES index! (it works when merging documents though) => the target user list of tagged route is not updated after merging

I wonder if the ES notifying at https://github.com/c2corg/v6_api/blob/routes-todo-list/c2corg_api/scripts/users/merge.py#L131 actually works (including in the master branch)...

After restarting ES sync by hand, restarting the following command

docker-compose exec api make -f config/docker-dev run-syncer

the target user TODO list is indeed updated.

FIXME: not transfered tags (because already tagged by the target user) are not removed from the ES index (thus still appear in the source user target list)

[cbeauchesne]

the UI adds a generic criteria to the routes list URL, eg. routes?tag=todo

Yep, it would far far better:

• it will be more coherent as the u filter would have only one meaning among the entire API instead of two,
• and if we extend the tag system later, we won't have to redo the API and UI part : routes?tag=other_tag.
• It'll implement the tag's privacy by design, letting us choose how we want to expose it.

the API replaces this criteria by the currently authenticated user id (no clue how to do this yet, any idea? :P)

I'll have a look on this before starting the review.

[asaunier]

I would suggest to support a list of tags: routes?tags=todo,foo,bar.

[cbeauchesne]

Well, fail, even before starting ^^

I didn't see a test on /routes/u=<xxx>, so I tried to add it :

class TestDocumentGetTaggedRoutes(BaseDocumentTagTest):

    def setUp(self):  # noqa
        super().setUp()
        self._prefix = '/routes'

    def test_get_routes(self):

        request_body = {
            'document_id': self.route1.document_id
        }

        self.post_json_with_contributor(
            '/tags/add', request_body, status=200, username='contributor2')

        self.session.flush()
        
        headers = self.add_authorization_header(username='contributor2')

        response = self.app.get(
            '{}?u={}'.format(self._prefix, self.contributor2.id),
            status=200, headers=headers)
        body = response.json

        self.assertEqual(body['total'], 1)

The body response is

{
    "total": 0,
    "documents": []
}

@asaunier , by chance, do you see something obvious I missed ?

[asaunier]

do you see something obvious I missed ?

@cbeauchesne Nothing obvious. Your code seems to make sense.
I don't think you need the self.session.flush().

Perhaps the test doesn't work because ES does not really work in test mode? I don't know much about that.
Have you tried to check for a similar test with the outings? eg. outings?u=12345

[asaunier]

Hi guys
Any reviews? :)

I suggest that we stick to the current approach of routes?u=1234 even though if it implies that todo lists are more or less public (but only by forging the UI URL).

The approach of routes?tags=todo would be better and more consistent with possible tags evolution. But I won't have much more time working on this task before the summer season.

[cbeauchesne]

Sorry, did not have a lot of time, but I really want to try. If I did not succeed in two weeks, we merge. Ok for you ?

[momomaniac]

i'll have a look, since I am currently in hospital. However, the internet connection is soso, and I don't have all the dev/build tools available right now. But I should be set up by next week to go into the detail of the PR. And then maybe implement the UI part. Thanks for the great work, I am so eager to try out the todo tags.

…

On Fri, Jun 5, 2020 at 10:17 AM Charles de Beauchesne < ***@***.***> wrote: Sorry, did not have a lot of time, but I really want to try. If I did not succeed in two weeks, we merge. Ok for you ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#756 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANY5RCUJBUWCM5FBQVPDH7LRVCSZRANCNFSM4JNS3POQ> .

[momomaniac]

what is the purpose of the document_type ? It seems to me redundant with the information in guidebook.documents

the document_type does not seem to be used in this PR ? can it be removed ?

[asaunier]

The document type is used by the ES syncer to build its index:

• get the list of documents to update in the index:

  v6_api/c2corg_api/scripts/es/sync.py

  Lines 35 to 40 in b1ec975

  	changed_documents = \
  	get_changed_documents(session, last_update) + \
  	get_changed_users(session, last_update) + \
  	get_changed_documents_for_associations(session, last_update) + \
  	get_deleted_locale_documents(session, last_update) + \
  	get_tagged_documents(session, last_update)

• type of the changed documents is then used at

  v6_api/c2corg_api/scripts/es/sync.py

  Lines 275 to 279 in b1ec975

  	def sync_documents(session, changed_documents, batch_size):
  	client = elasticsearch_config['client']
  	batch = ElasticBatch(client, batch_size)
  	with batch:
  	docs_per_type = get_documents_per_type(changed_documents)

Except this the document type is as for now indeed not that relevant because all tagged documents are routes ("todo" tag). But the goal of this PR is to be extended for other kind of documents with other kind of tags.

[momomaniac]

OK, thanks for your explanation.
still the data is redundant, couldn't it be handled with an sql view which queries the document type from guidebook.documents via the foreign key ? I don't feel comfortable with adding the same data once more into a second table and risk inconsistent data.

[asaunier]

The concern about data inconsistency makes sense indeed.

We could use views or other systems to get the type directly from the document itself but wouldn't it make things even more complicated? Perhaps we could make sure the type is correctly retrieved when creating the tags.

The current PR makes sure the document about to be tagged is actually a route, see https://github.com/c2corg/v6_api/blob/routes-todo-list/c2corg_api/views/document_tag.py#L32-L42

Please also note the document tags system is significantly inspired from the association system (dedicated table for the the actual associations + a table for the logs, as for the tags system). Associations do have document type fields duplicated in the association tables:
https://github.com/c2corg/v6_api/blob/master/c2corg_api/models/association.py#L45-L52

[momomaniac]

ah, that's why. Yes associations also have the document type. Actually I started working on #804 and already forgot about the document types and potential inconsistency.

I checked in the db, it is really easy to retrieve the doc type directly from the table guidebook.documents (without further joins,...) so it's probably not much more complicated to retrieve the document type on the fly. Performance-wise, the foreign key is indexed, so it should be OK ?

[momomaniac]

ok, I reindexed ES locally, and now also the routes?u= entrypoint is working (I tested it manually, but I'll look also into the test written by @cbeauchesne )

[cbeauchesne]

Sorry, did not have a lot of time, but I really want to try. If I did not succeed in two weeks, we merge. Ok for you ?

Well, I've not a single second of free time, and it won't be better for one month. Sorry @asaunier, I really wanted to have some times to look at this, but it won't be reasonable to wait for so long. I release my lock on this :) @momomaniac I give you the hand!