#hypothes.is

<nick> So @seanh for this get_by_id thing, I only count six uses of `User.get_by_id`, so yes, I think that just removing that class method is the way to go

<nick> and perhaps to make life easier for yourself you can add a `util.split_user_local` helper which takes the request

<nick> (and returns `None` if the domains don't match)

<seanh> But if I have to replace every call to get_by_id(request, userid) with a call to split_user_local(request, userid) and then a call to get_by_username(), that doesn't help me much

<seanh> Because I still need the request to do that

<nick> I'm not sure I understand why that's a problem?

<seanh> What I'm wondering is whether all uses of get_by_id() (including several new ones that I'm adding) really need to do that domain check

<nick> You need a request to access the database.

<nick> Sometimes that can be implicit, because of Zope magic.

<nick> Sometimes it can't be

<nick> Passing the request all over the place can be a bit of a pain, but there's literally only one other way to write a web application with shared state: thread locals.

<seanh> Because lots of places that don't have the request object want to get users, now I need to pass the param through multiple modules just to get it to one place where it's needed, so it's just a bit of a pita

<nick> What's the exact use case you're talking about here?

<seanh> Right, well I guess it's implicit, you don't need a request to call User.get_by_username() (it has cls.query)

<nick> Indeed, and that is convenient magic.

<seanh> So what I'm saying is can't we just use the convenient magic here? / I don't understand what the domain check is reall for

<seanh> Let me dig out a case...

<nick> I guess the question is really whether `request.authenticated_userid` can ever end up being a user with a different domain.

<nick> And I'm not sure it can at the moment.

<nick> Because it either comes from the session (as set [here](https://github.com/hypothesis/h/blob/9f5674f0a5c6bc0ba09e838ad18c08e726c63463/h/accounts/views.py#L128-L129)),

<nick> or from the token

<seanh> Exactly, when would the domain be differen? And if it's only in one weird case, can't the relevant code just do the check itself?

<seanh> And we _can't_ actually store users from different domains in the database, even. Domain isn't in the db, and username is unique'd in the db, which confuses me further

<seanh> So this kind of looks like an unnecessary check, that we can remove from the code, but I guess tilgovi is the only one who'd know if there is in fact a case where it's needed

<nick> as commented

<seanh> Great

<seanh> It might be interesting (one day) to track all the place where we accept or expose userids or usernames from the externla

<seanh> external

<seanh> And 1 make sure those are always full acct: userids

<seanh> and 2 transform them to/from userids as close to the edges as possible, work with simple usernames in as much of our code as possible

<seanh> Until a time when we actually have user accounts from different domains in the db

<seanh> I'll add that to the pr actually

<seanh> (as a comment)

hypothesis/h#7436 (publish-to-a-group - 7e21e59 : Sean Hammond): The build is still failing.

Change view : https://github.com/hypothesis/h/compare/cac4f21...

Build details : https://travis-ci.org/hypothesis/h/builds/74386694

<seanh> nick: What do you think about introducing a low specificity approach into our CSS? Even though the existing code doesn't do it

<seanh> I'm talking about .group-list-dropdown-menu-group-name

<seanh> Instead of .group-list .dropdown-menu .group-name

<seanh> Just realised what a pita debugging CSS selectors is

<nick> I'm not sure I know enough to have a sensible opinion

<seanh> I guess I don't really need to, I fixed my problem without it

<nick> But I think the goal is probably to have *modular* CSS, so that we don't have to understand all of it to understand any of it.

hypothesis/h#7438 (publish-to-a-group - cff6a58 : Sean Hammond): The build is still failing.

Change view : https://github.com/hypothesis/h/compare/7e21e59...

Build details : https://travis-ci.org/hypothesis/h/builds/74388565

<nick> And that's something that both BEM and SMACSS attempt to provide

<nick> http://getbem.com/, https://smacss.com/

<seanh> It's advice from the Trello guide and makes a lot of sense to me: You just have really long class names, so finding all applications of a class is just a grep, you don't have to take the (changing) structure of the HTML into account and debug selectors, have selectors breaking when you change HTML

hypothesis/h#7440 (publish-to-a-group - 8d4eb06 : Sean Hammond): The build is still failing.

Change view : https://github.com/hypothesis/h/compare/cff6a58...

Build details : https://travis-ci.org/hypothesis/h/builds/74389401

<nick> `git rebase --autosquash` is my new best friend

<seanh> Another problem with these group dropdowns on the annotation cards is that they also appear in replies

<seanh> So I can reply to an annotation in group foo, and put my reply in group bar, or make it public

hypothesis/h#7444 (publish-to-a-group - 76ce8db : Sean Hammond): The build is still failing.

Change view : https://github.com/hypothesis/h/compare/8d4eb06...

Build details : https://travis-ci.org/hypothesis/h/builds/74392144

<seanh> I think it's pretty clear that they just need to go

<seanh> So replies to annotations contain the original annotation's id in a "references" field. And replies to replies contain both the original reply and the annotation that was a reply to in the references list. These references are put there by the client

<seanh> We need these replies to have the same group as the original annotation, for the groups filtering

<seanh> I'm tempted to implement that server-side instead of in the client

<seanh> The client can even go ahead and send the wrong group if it wants - server will overwrite it

<seanh> I'm guessing the first item in the references field is always the id of the top-level annotation, looks like

<nick> Ugh.

<nick> This is one of my least favourite features of our data model.

<nick> Replies aren't annotations.

<nick> They're replies.

<nick> Anyway

<nick> Carry on.

<nick> I concur with your assessment :)

<seanh> This is something I don't like: https://github.com/hypothesis/h/blob/master/h/a...

<seanh> What we should have there is a nice function for getting an annotation given its id

<seanh> But thanks to traversal, it's unusable

<seanh> Not convinced at all about traversal, still

<nick> To be fair, you're getting a lot of extra functionality there. It's not a logic function, it's a view function.

<nick> What are you trying to do? Getting an annotation given its id is straightforward enough: `

<seanh> Oh just being lazy

<seanh> It always surprises me how complicated stuff is when you actually get to writing it

<seanh> There's a lot of details to this groups stuff

hypothesis/h#7448 (publish-to-a-group - 095038d : Sean Hammond): The build is still failing.

Change view : https://github.com/hypothesis/h/compare/76ce8db...

Build details : https://travis-ci.org/hypothesis/h/builds/74396104

<seanh> You get the feature 80% working and then you still have a ton to do

<seanh> Anyway, lunch

<seanh> One possible value for the "public" group would be "fish", hashids will never generate that because it tries not to put those letters next to each other to avoid swearing

<seanh> Or we could use a hashid generated with the number 0, since our postgres autoincrementing ids start from 1

<seanh> I think we might want a system that lets us add more "special" group names later, in case we want to, though

<seanh> So I'm leaning towards simple namespacing like group: "public" and group: "hashid:hashid"

<seanh> The only thing is that the whole point of having group: "public" is for consistency, every annotation will have a group field whose value is a string (as opposed to if we just omitted the group field from public annotations)

<seanh> But if we use "hashid:hashid" then code will have to split on that : and then that will fail for "public" annotations so you've lost the consistency anyway

<seanh> We could use the hashids length controls

<seanh> Or we could use a custom alphabet, only lowercase letters in hashids, then use "PUBLIC"

<seanh> I think we should use either the lowercase alphabet or we should just omit the group field entirely for non-group annotations

<nick> @seanh: hashids use base62 by default, right?

<nick> so just call it "__public__"

<seanh> I have no idea what base62 has to do with that

<nick> base62 doesn't include underscores

<seanh> Right, yeah there's no underscores in the default alphabet

<seanh> I see, so 26 + 26 + 10 = 62 (lower and upper case alphabets and digits)

<seanh> Cool

<nick> I think `group:admin` and `group:staff` should probably become `group:__admin__` and `group:__staff__` for the same reason

<seanh> I'm thinking of going for "__none__" though because this special group is not really a group (it's just a special value we're putting into the groups field for consistency/ease of coding) and because I think it's weird for private annotations to have group: "__public__" (or "world" etc) in them

<seanh> Admin and staff are an odd one, we have two different features that we're calling groups, ideally two different names for the features might be goodf

<nick> maybe

<nick> it's back to "too hot to think" here in Berlin...

<seanh> Well, "group:admin" is only ever used as a principal in Pyramid

<seanh> And we're also adding these new user-created groups as principals as well

<seanh> So that's the only place where these two might clash

<seanh> If we just used "usergroup:foo" for the principal string for example, but that's not a very satisfactory alternative name

<seanh> Honestly, "group:admin" should probably just be "admin"

<seanh> And then foo can be "group:foo"

<seanh> That's how we talk about the admin and staff features: we just call them admins and staff. We don't call these features groups. So