Enable users to create groups #2402

Merged
merged 16 commits into from Jul 31, 2015

Projects

None yet

4 participants

@seanh
Contributor
seanh commented Jul 24, 2015

@nickstenning suggests that (since it's behind a feature flag) this can be merged now even though the groups dropdown list in the sidebar is still a fake.

@tilgovi Want to review this? Since Nick and I have both worked on it.

What this does:

  • Authorized users (anyone who's logged-in) can go to /groups/new to get a form for creating a new group
  • The form has validation and re-rendering with inline errors and user data intact, if the group name is too short or too long
  • On form submission group will be created, user is redirected to group's page including hashid as part of URL. Anyone (logged-in or not, member of group or not) can see this page if they have the link. No functionality on this page yet.
  • In the database we save a bunch of generated stuff: created and updated times, creator, and make the creator the first member of the group
@seanh seanh changed the title from New group to Enable users to create groups Jul 24, 2015
@nickstenning nickstenning commented on an outdated diff Jul 24, 2015
h/groups/views.py
+
+import deform
+import colander
+import hashids
+from pyramid import httpexceptions as exc
+from pyramid.view import view_config
+
+from h.groups import schemas
+from h.groups import models
+from h.accounts import models as accounts_models
+from h import security
+
+
+def _get_hashids(request):
+ salt = security.derive_key(
+ request.registry.settings["secret_key"], "h.groups.hashid", length=20)
@nickstenning
nickstenning Jul 24, 2015 Member

As we discussed, I think this is a dangerous route to go down. Just add a config option for this.

@nickstenning nickstenning commented on an outdated diff Jul 24, 2015
@@ -50,6 +51,7 @@ def run_tests(self):
'gevent>=1.0.2,<1.1.0',
'gnsq>=0.3.0,<0.4.0',
'gunicorn>=19.2,<20',
+ 'hashids>=1.1.0',
@nickstenning
nickstenning Jul 24, 2015 Member

We should pin this to a specific version as recommended on hashids.org.

@nickstenning
nickstenning Jul 24, 2015 Member

Maybe even add a comment explaining why (hashid format may change with updated hashid package, so always pin).

@nickstenning
Member

So, just to be clear, this is butt ugly at the moment and definitely will need more work. My comment about the feature flags is merely observing that merging this doesn't block deployment, so if we're happy with what's here we can merge and carry on in additional PRs.

@landscape-bot

Code Health
Repository health increased by 0.04% when pulling 6a392b5 on new-group into b175b6a on master.

@seanh seanh referenced this pull request Jul 29, 2015
Merged

Share a group #2412

@tilgovi tilgovi commented on the diff Jul 29, 2015
h/groups/models.py
@@ -0,0 +1,58 @@
+# -*- coding: utf-8 -*-
+import sqlalchemy as sa
+from sqlalchemy.orm import exc
+
+from h.db import Base
+from h.textutil import slugify
+
+
+class Group(Base):
+ __tablename__ = 'group'
+
+ id = sa.Column(sa.Integer, autoincrement=True, primary_key=True)
+ name = sa.Column(sa.Unicode(100), nullable=False)
@tilgovi
tilgovi Jul 29, 2015 Contributor

How do we motivate a choice of group name size limit?

@nickstenning
nickstenning Jul 30, 2015 Member

Honestly, I just figured that

East Louisiana State Troopers Chinchilla Brevicaudata Appreciation Society - East Baton Rouge Branch

Was more than long enough for a group name.

@tilgovi tilgovi and 1 other commented on an outdated diff Jul 29, 2015
h/groups/models.py
+ return slugify(self.name)
+
+ def __repr__(self):
+ return '<Group: %s>' % self.slug
+
+ @classmethod
+ def get_by_id(cls, id_):
+ try:
+ return cls.query.filter(
+ cls.id == id_).one()
+ except exc.NoResultFound:
+ return None
+
+
+user_group_table = sa.Table('user_group', Base.metadata,
+ sa.Column('user_id', sa.Integer, sa.ForeignKey('user.id')),
@tilgovi
tilgovi Jul 29, 2015 Contributor

Should this be nullable=False?

@nickstenning
nickstenning Jul 30, 2015 Member

Good call. There's no reason to allow NULLs here, so yes.

@tilgovi tilgovi and 1 other commented on an outdated diff Jul 29, 2015
h/groups/schemas.py
@@ -0,0 +1,10 @@
+# -*- coding: utf-8 -*-
+
+import colander
+
+from h.accounts.schemas import CSRFSchema
+
+
+class GroupSchema(CSRFSchema):
+ name = colander.SchemaNode(colander.String(),
+ validator=colander.Length(min=4, max=100))
@tilgovi
tilgovi Jul 29, 2015 Contributor

Again, I'd like to understand why we choose these minimums and maximums.

@nickstenning
nickstenning Jul 30, 2015 Member

Minimum is so we have something reasonable for a slug. Maximum: see the chinchillas.

@tilgovi tilgovi commented on the diff Jul 29, 2015
h/groups/views.py
+ if not request.feature('groups'):
+ raise exc.HTTPNotFound()
+
+ hashid = request.matchdict["hashid"]
+ slug = request.matchdict.get("slug")
+ group_id = hashids.decode(request, "h.groups", hashid)
+
+ group = models.Group.get_by_id(group_id)
+ if group is None:
+ raise exc.HTTPNotFound()
+
+ if slug is None or slug != group.slug:
+ canonical = request.route_url('group_read',
+ hashid=hashid,
+ slug=group.slug)
+ return exc.HTTPMovedPermanently(location=canonical)
@tilgovi
tilgovi Jul 29, 2015 Contributor

What's the reasoning behind having the redirect?

@nickstenning
nickstenning Jul 30, 2015 Member

Without the redirect, we have to choose between:

  1. 404ing for requests with bad slugs
  2. ignoring the slug entirely and serving the group page as identified solely by hashid

I don't particularly like 1) as it introduces more ways to break a group URL when copying and pasting it (e.g. in emails). I really don't like 2) as it has terrible search engine properties and encourages people to create nasty URLs for groups they don't like (i.e. taking /groups/abc123/whale-lovers and redistributing it as /groups/abc123/zooplankton-haters).

So, if we don't like either those options, the third option is to accept slug typos, but 301 them to the right place. This, incidentally, is the approach taken by Trello, among others. (e.g. https://trello.com/b/o2ungX1n/current-springles redirects to https://trello.com/b/o2ungX1n/current-sprint).

@seanh
seanh Jul 30, 2015 Contributor

This is just to redirect /groups/xKkqeD (with or without trailing slash) to /groups/xKkqeD/radiohead-fans. I have to admit I'm not entirely sure what the use-case is (how would someone even have the link without slug, unless they typed it in manually?), but Trello does it!

It does also redirect /groups/xKkqeD/the-wrong-slug to /groups/xKkqeD/radiohead-fans which may be useful if the group's name and therefore its slug has changed (not that we support renaming groups yet but we probably will one day)

@tilgovi
tilgovi Jul 30, 2015 Contributor

Okay. I would just think really hard about whether it should be a permanent or a temporary redirect. Can one change a group name? I'm still not even convinced of the idea that groups should "have" intrinsic names vs allowing people to rename their groups as they please.

@tilgovi
tilgovi Jul 30, 2015 Contributor

Actually, that last part is really important to me. Can we have a real discussion about intrinsic names before we merge this?

@seanh
seanh Jul 30, 2015 Contributor

Intrinsic names?

My suggestion is this:

  • Each group has a unique (auto-generated, not user-visible) id which is what we actually use to identify it
  • We can encode the id to get the hashid which is what we show to the user in the URL, and decode the hashid to get the id which is how we know what group object a given http request is for
  • The group's name is for display purposes only, not for identification. Well, the users may identify the group by seeing its friendly name, but the system always uses the id/hashid.
  • Name is slugified into URL to make URL friendlier
  • We should allow users to edit the names of groups after creation (not required for v 1.0)

This does seem to mean that these ought to be temporary redirects maybe, since the group name may change and then the URL we want to redirect to would change.

@tilgovi
tilgovi Jul 30, 2015 Contributor

What I'm trying to say is that the group name could be on the join table, and users would be able to personalize the way they name their groups.

You could imagine a class where a teacher has three groups: Section 1, Section 2, and Section 3. The students, however, regardless of which they are in, might call it simply "Philosophy" or maybe another student wants to call it "Mrs. Hermann's Class".

Whether or not these are use cases we care about, or if anyone else thinks personal names for things is a desirable property of objects generally to the extent that I do, is not really what I'm trying to ask here.

It's more like this:

  • I've noticed an assumption baked into this code that groups have a (singular) name.
  • Let me just check in with everybody about whether it was ever considered that maybe they don't, that there exist other ways to go about this.
@tilgovi
tilgovi Jul 30, 2015 Contributor

And probably a big part of why I'm urgently flagging this is because I see a permanent redirect, and those are hard to change. So, I want to understand whether there's any reason this might change, and if we were to adopt such a multiple/personal naming model for groups we would obviously no longer have one single, correct URL that contains a slug for a group. My gut is to say just skip the slug altogether for now.

@nickstenning
nickstenning Jul 30, 2015 Member

Okay, so let's change it to a temporary redirect for now.

For what it's worth, I don't see the permanent redirect as being a massive problem either. No intermediaries (proxies) or crawlers that I know of actually store them permanently (we had this discussion with the Google crawler team when working on GOV.UK), and if the group name/slug were to change the worst that would happen is that they'd go direct to a page that would then issue them another 301 to the correct final URL.

Groups having multiple names -- one per user, is definitely a feature we didn't discuss as part of this card. It's certainly interesting, and by all means let's revisit it in the future.

@nickstenning
nickstenning Jul 30, 2015 Member

I've gone and reread Google's guidance on use of redirects, and actually I think a permanent redirect is correct here.

See, for example: https://support.google.com/webmasters/answer/139066?hl=en

It's worth emphasising: a "permanent redirect" isn't permanent, in this context it just means "this isn't the canonical URL for the page you're requesting, please update your records". Which is exactly the meaning these redirects are intended to convey.

@tilgovi
tilgovi Jul 31, 2015 Contributor

Thanks for making it temporary. I'd like to keep it that way for now because I'm not convinced yet that the place its redirecting to is canonical, yet, until we do some thinking about it.

@tilgovi tilgovi and 2 others commented on an outdated diff Jul 29, 2015
h/textutil.py
@@ -0,0 +1,18 @@
+# -*- coding: utf-8 -*-
@seanh
seanh Jul 30, 2015 Contributor

I think I prefer our own four line function over adding a lib

@nickstenning
nickstenning Jul 30, 2015 Member

Good call. I evaluated a couple which all sucked for unicode, but I missed this one.

@nickstenning
nickstenning Jul 30, 2015 Member

I think I prefer our own four line function over adding a lib

Our own four line function depends on unidecode and regex...

@tilgovi
Contributor
tilgovi commented Jul 29, 2015

Looks fine, just a few small questions.

@landscape-bot

Code Health
Repository health decreased by 0.11% when pulling 6a392b5 on new-group into 3107939 on master.

@seanh
Contributor
seanh commented Jul 30, 2015

It might be nice to tweak some names to make them consistent. Our view callables are create_group_form(), create_group() and read_group(), yet the route names are group_create and group_read, and the templates are again create_group and read_group.

You could argue for verb first or noun first, or in the case of the callables and the templates just verb without noun (create(), read() etc) since they're already in a groups package.

We don't seem to be consistent about this in views.py files across the codebase.

@nickstenning nickstenning and 1 other commented on an outdated diff Jul 30, 2015
conf/production.ini
@@ -15,6 +15,8 @@ use: egg:h
#h.client_id:
#h.client_secret:
+h.hashids.salt: production salt
@nickstenning
nickstenning Jul 30, 2015 Member

I'm inclined to remove this so that if you don't set the environment var in production the application will explode rather than silently using a predictable salt.

@seanh
seanh Jul 30, 2015 Contributor

Iirc correctly I put this in because Travis was crashing otherwise (it builds the Chrome extension with production.ini iirc). We could fix the Travis script instead

@nickstenning
Member
  • Rebased on master
  • Schema migration rebased onto current head
  • Switched to using python-slugify
  • Updated view function names
  • Made the FK fields in the user-group join table NOT NULLABLE
  • Lightly squashed the commit history
@seanh
Contributor
seanh commented Jul 30, 2015

Just fixed most of landscape's issues, I'm happy to leaves it remaining complaints alone

@landscape-bot

Code Health
Repository health increased by 0.23% when pulling 6f1b4e0 on new-group into 3920527 on master.

@seanh
Contributor
seanh commented Jul 31, 2015

@tilgovi We're done with this, merge if you're happy with it

@landscape-bot

Code Health
Repository health increased by 0.07% when pulling a828794 on new-group into cbffd55 on master.

nickstenning and others added some commits Jul 20, 2015
@nickstenning @tilgovi nickstenning Add a group package and data model
This commit adds a package "h.groups", which for now contains only data
models for group membership, and a schema migration to create the
relevant tables in the database.

For now, groups are identified simply by unique id, and they have a
unicode name, which is expected to be their display name.
2bc0e50
@nickstenning @tilgovi nickstenning Add a basic "create a new group" page 4dc009e
@tilgovi Sean Hammond Hook up the new group form to the model
On form submission create a new group in the db and then redirect to the
group's page.

Validation of form params still needs to be done.

We're using the group id direct from the db in the group page's URL,
this needs to be replaced with a hashid based on the group's id and some
salt.
d68394f
@tilgovi Sean Hammond Add validation to new group form
- create_group() validates the posted params with our colander schema,
  rerenders the form on validation failure

- Views now return two items to the template: "form" and "data" (a dict
  of any values the user had entered into the form, e.g. {"name": "My
  New Group"}), so we can rerender the template with the user's data
  intact
1141847
@tilgovi Sean Hammond Add tests for groups views 57d2550
@nickstenning @tilgovi nickstenning Add groupList directive to display user's groups
This commit adds a list of groups to the "topbar" when the groups
feature flag is enabled.
e99d512
@tilgovi Sean Hammond You have to be logged-in to create a group 1a6c5f4
@tilgovi Sean Hammond Record creator when creating a new group 8bc9624
@tilgovi Sean Hammond Use hashids in group URLs
Rather than exposing raw primary keys in the group URLs, we use hashids
so that the URLs cannot be trivially enumerated. In production, we set a
hashid salt that ensures that other people can't generate our hashids.
71c24a8
@tilgovi Sean Hammond Remove test code duplication using fixtures 393065b
@tilgovi Sean Hammond Add tests for group models 649aaf4
@nickstenning @tilgovi nickstenning Slightly better slug redirections
This commit slightly simplifies the handling of requests with missing
slugs, while also catering for another common scenario: mistyped slugs.

Now, given a group with hashid "abc123" and slug "hello-world", all of
the following paths will redirect to "/groups/abc123/hello-world":

    /groups/abc123
    /groups/abc123/
    /groups/abc123/hello-wolrd

Any more path components will throw a 404 as before.

In addition, the redirect is now served as a 301 Moved Permanently.
f4f244f
@nickstenning @tilgovi nickstenning Shorter view function names
These view functions are already namespaced, so referencing groups is
redundant and leads to inconsistencies with the global namespace of
route names.

    create_group_form -> create_form
    create_group -> create
    read_group -> read
a140f5b
@tilgovi Sean Hammond PEP8 cb165ac
@tilgovi Sean Hammond Add some docstrings 8c2bf50
@tilgovi Sean Hammond Remove a couple of unused names
1ca0236
@tilgovi
Contributor
tilgovi commented Jul 31, 2015

Rebased over the simple conflict with the admins permissions PR. Waiting for the green light just to be paranoid, but it looks good and I'll hit merge when that's good.

@tilgovi tilgovi merged commit 0b1f85a into master Jul 31, 2015

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.6%) to 62.854%
Details
@tilgovi tilgovi deleted the new-group branch Jul 31, 2015
@tilgovi
Contributor
tilgovi commented Jul 31, 2015

\o/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment