Review Board 1.0.5.1

Working cut of the tags app

Updated 8 months, 1 week ago

John Peters Reviewers
Basie-Dev
None Basie SVN 
Simple yet functional cut of tagging.

You can view all tags in Basie, and view all objects that have a given tag. An inclusion tag can be added to views in other apps to let you view all tags associated with an object, and also lets you add or remove tags from an object.

Currently, this patch does NOT but SHOULD support:
- performing HTML sanitation on tag names
- filtering the list of objects associated with a tag so you only see the ones you're authorized to see

If all goes smoothly, I prefer addressing the following in a separate patch:
- renaming a tag
- displaying a weighted tag cloud
Tests are present for the business logic layer (service layer) and for the controllers (views.py).
John Peters
Posted 8 months, 3 weeks ago (November 8th, 2009, 12:54 a.m.)
A few specific questions about how Basie does things; want to keep a consistent feel in the tags app.
For each tag displayed, there should be a link for removing that tag from this object. The actual delete should be on a confirmation page with a POST (don't want Google deleting our tags). What's the convention for 'confirm delete' pages in Basie (almost nothing ever gets deleted). Should it be a separate page, or some popup?
  1. Since Ian's latest addition to milestones there is a confirmation dialog for deleting them.
  2. Does this protect us from CSRF (Cross-Site Request Forgery) type of attacks? (don't want anyone with just POST requests deleting our tags).
  3. Re CSRF: Interesting point. Clicking on a remove link will display a Javascript 'confirm', and then generate a POST request. The current user will be checked for sufficient privileges, but yes, CSRF could be a 'vulnerability'.

I don't see the motivation of maliciously adding or removing tags, but if this is a concern, I could have all data-changing requests check the referrer. Presumably there's an easy way to get the domain name of the Basie instance.

(Gah - forgot to hit publish.)
  4. The referrer is send by the client, and I've found out (the hard way) that some "security" apps (at least some version of Norton) will intercept and strip that information out of web requests (for "privacy", I assume).

In short -- checking the referrer is not reliable. CSRF is typically handled by having users sign their requests with unique hashes.

I guess this isn't really tags specific, and would apply to _all_ POST requests.

Perhaps in the next term, someone can implement http://docs.djangoproject.com/en/dev/ref/contrib/csrf/
There can easily be a form with one field here for renaming the tag. But does Basie want to keep view and edit pages distinct (looking at tickets seems to say yes). Seems rather silly to have an edit page with just one field.
  1. I'm leaning towards Greg's idea on this one...
/trunk/apps/tags/urls.py (Diff revision 1) 
 
 
 
 
 
Double check URLs to see if they fit Basie's conventions.
Review request changed
Updated 8 months, 3 weeks ago (November 10th, 2009, 3:02 p.m.)
Fix to the patch - the __init__.py files were not being created, as they had no content (weird, as they still had entries in the patch). Added an empty line to the files.

Responding to the conversation on the mailing list:

Greg: "does "rename" affect one tag on one object or does renaming the tag affect all uses of that tag?  If the former, just let people overtype inline.  If the latter, a page that displays all tags with usage counts seems useful...?"

The detail view for each tag will display that tag's name, as well as all the objects that use that tag (so yes, the user will be able to see the usage for each tag). The rename operation will affect all usages for that tag.

The main tag page currently displays a list of all tags. I could easily display a usage count for each tag on that page, or continue working on a tag cloud.

Tag deletion:

Milestones uses a popup confirmation dialog, so I'll go that route. django-tagging doesn't have anything like enabled/disabled tags, so all deletions would be permanent. I'd have to extend django-tagging's models if we wanted an 'enabled' field for tags and the tag-object join table - using some type of auditing would be cleaner if we really want this feature.
Review request changed
Updated 8 months, 2 weeks ago (November 13th, 2009, 2:13 a.m.)
Changes:
- created bll.py for the business logic layer (service layer) - is this overkill?
- tags can be completely removed.
- objects can be untagged.
- some permissions have been added.

A few more questions:

- It will be very difficult to make my unit tests use YAML fixtures. The TaggedItem join table (used to map tags to objects) refers to objects via their primary key AND the primary key of their ContentType. It's easy to create TaggedItem rows programmatically, but hard to do in YAML short of guessing the ContentType primary key. I could hardcode a value in now that would work, but whenever a new model is added to Basie, this would break the tests. Is it fine if I create TaggedItem rows in the setUp() function of my tests, or is there some YAML magic/YAML alternative that I don't know about?

- I haven't added permission checking to all my views yet. The problem is that the tagging inclusion tag will be placed in many other templates in Basie. If users lack the TAGS_READ privilege, we don't want to stop them from reading a wiki page (or hide the 'Add Tag' form if they lack the TAGS_ADD privilege, etc.). What's the preferred location for hiding the offending information? Should I check user permissions in my views.py and use different templates depending on how privileged the user is? (And create multiple templates with redundant HTML, violating DRY?) Or should I pass the user object to my templates and have them only display the HTML that the user is allowed to see (and violate MVC)? Either works for me.
Tony Targonski
Posted 8 months, 2 weeks ago (November 14th, 2009, 1:58 p.m.)
There's some parsing error on templates when the tag name uses <> and/or single quotes.

In template /Users/tony/basie/trunk/apps/tags/templates/tags/object_tags.html, error at line 5
Caught an exception while rendering: Reverse for 'basie.tag-detail' with arguments '(u'dev_project', <Tag: <script>alert('foo');</script>>)' and keyword arguments '{}' not found.

Line 5 being: {% for tag in tags %} 

-----------
Tagging with "one two" tags item with two separate tags.
Tagging with "one two, three" thrown AttributeError, Multiple tags were given: "one two".

-----------

Should there be any special clean-up facilities for removing a tag from an object, and having the tag link to absolutely nothing after?

------------
  1. Gah! Another thing django-tagging doesn't actually do. Okay, so I'll sanitize my inputs, for HTML at least (if I recall correctly, Django DB sanitized for SQL injects). May end up severely restricting valid characters for a tag, as tag names are currently used in URLs.

-----------

This was simple to fix. django-tagging seems to have a habit of having built-in magic in one area, but none for an almost identical function.

-----------

You mean after untagging all usages for a tag, the tag should be deleted? I added this in my latest patch. If we ever require that tags never be deleted (maybe after setting up auditing), this will need to be changed.
/trunk/apps/tags/bll.py (Diff revision 3) 
 
 
pep8 "expected 2 blank lines, found 1"
  1. Fixed for my files, thanks.
could this be some sort of a human-readable name, instead of it rendering as "ChatConversation object"
  1. So far, I've been deferring each object's string representation to its __str__ function (or __unicode__, or whatever Django templates call). I could do something like {{ item.object.name }}, but then that's yet another restriction on the possible objects that could be tagged (currently, all tagged objects are assumed to have a 'project' property and have a get_absolute_url() function). In this case, {{ item.object.name }} isn't unreasonable - I may change it to that. But still, I'd rather avoid all assumptions about the generic tagged objects when I can.
  2. Oh, I see what's going on now. I can just implement a reasonable __str__ on my own classes then :)
/trunk/apps/tags/views.py (Diff revision 3) 
 
 
How complicated would it be to cloud~ify the tags?
  1. It would be a bit of work, but not a horrible deal of it. django-tagging already has most of the code for it (logarithmic font weights, cutoffs for little-used tags, the works). The problem is that django-tagging assumes you only ever tag one model. I'd have to rewrite portions of their code so the tag cloud is computed for all models. Most of it would be copy and paste, with some modifications here and there.

It's doable. I'd prefer to avoid forking code whenever I can, but I don't think there's a choice in this case.
/trunk/apps/tags/views.py (Diff revision 3) 
 
 
I don't completely understand the way the permissions work; but judging by some other discussion comments, it seems that lack of a particular READ permission should make items invisible. Wouldn't this by-pass any such READ permissions for associated (tagged) objects?
  1. Yep, this is on my TODO list. Currently, clicking on the link won't cause any harm, but you're right - we don't want those links displayed.

The tricky part is figuring out the particular READ permission to use - see my comment later on.
/trunk/apps/tags/views.py (Diff revision 3) 
 
 
405 Method Not Allowed

http://en.wikipedia.org/wiki/List_of_HTTP_status_codes#4xx_Client_Error
  1. Ah great, thanks.

However, after some investigation, it turns out we don't need to check the HTTP method after all. If a 'get_perm' parameter isn't specified in the @permission_required annotation (... decorator?), a HTTP 403 is used. Not the best behaviour, but it seems to be what the rest of Basie does.
/trunk/apps/tags/views.py (Diff revision 3) 
 
 
if form.is_valid() && user.has_permission

check that the user actually has TAG_WRITE permission... and/or OBJECT_WRITE permission?
  1. Again, this would be nice, but figuring out the name for an object's write permission is difficult when dealing with an arbitrary object. I'm new to Django permissions too, so I spent a fair part of today digging into them.

It seems that all permissions take the form `<app_label>.<operation>_<model_name>`. Assuming all permissions follow this format, I'll be able to build the appropriate permission name, as I'm passing around the ContentType anyway.
/trunk/basie/settings.py (Diff revision 3) 
 
 
should be this 'django_tagging' to match buildout.cfg's eggs?
  1. Nope - it has to be 'tagging', and Django won't run with anything else I've tried. I think it's confusing too.
Review request changed
Updated 8 months, 2 weeks ago (November 15th, 2009, 12:32 a.m.)
Thanks a lot for the review, Tony!

- permissions (for tagging, at least - see my comments in Tony's review) have been added.
- tests for the bll.py and views.py have been added.
- responded to most of Tony's review.

Sanitizing input for tag names and needing dual permissions for tagging objects (write permission on both tagging and that object) are the show-stoppers for this patch. I'd rather get stuff into SVN before gunning for nice-to-haves like tag clouds (is the feature freeze this Wednesday or Friday?)

(The review summary will be updated.)
Review request changed
Updated 8 months, 2 weeks ago (November 15th, 2009, 12:36 a.m.)

  • Test files will be added to the patch once I isolate the ones that just affect these changes.

    Tests are present for the business logic layer (service layer) and for the controllers (views.py).

  • Simple yet functional cut of tagging. It doesn't include everything I'm working on locally, but I'm posting the working subset so I can have more eyeballs.

You can view all tags in Basie, and view all objects that have a given tag. An inclusion tag can be added to views in other apps to let you view all tags associated with an object, and also adds tags to an object.

Currently, this patch does NOT support:
- renaming a tag
- removing a tag from an object
- displaying a weighted tag cloud
- filtering the list of objects associated with a tag so you only see the ones you're authorized to see

    Simple yet functional cut of tagging.

You can view all tags in Basie, and view all objects that have a given tag. An inclusion tag can be added to views in other apps to let you view all tags associated with an object, and also lets you add or remove tags from an object.

Currently, this patch does NOT but SHOULD support:
- performing HTML sanitation on tag names
- filtering the list of objects associated with a tag so you only see the ones you're authorized to see

If all goes smoothly, I prefer addressing the following in a separate patch:
- renaming a tag
- displaying a weighted tag cloud
Tony Targonski
Posted 8 months, 2 weeks ago (November 17th, 2009, 2:29 p.m.)
So with the above mentioned show-stoppers, are there more changes coming to this patch? Or should we just review again as-is, and commit with #TODO comments?
Review request changed
Updated 8 months, 1 week ago (November 18th, 2009, 2:05 a.m.)
Here's a minor change - tagging/untagging an object now requires change permission for that object, as well as add/remove permissions for tagging.

I'd prefer addressing the other concerns in separate, smaller review requests. Special characters in tag names is the only thing that breaks this patch, but to fix it The Right Way would require changing AddTagForm. These changes would be easiest to do if adding a tag was also an AJAX operation, and I think that should be part of another ticket.
Ship it!
Tony Targonski
Posted 8 months, 1 week ago (November 20th, 2009, 4:15 p.m.)
I've played around with Tagging again. I guess HTML escaping will go in as a separate patch. It looks good to me otherwise. Ship it.
  1. Changes have been committed to the repository. After updating, remember to run `./bin/buildout -v -c buildout.dev.cfg` and `django syncdb`.