#hypothes.is

<sheetaluk> wuff morning everybody!

<chdorner> morning! :sunny:

<chdorner> just joining the standup, have to find my headphones :slightly_smiling_face:

<sheetaluk> ok. i have to turn on the lights

<sheetaluk> Can you hear me? Doesnt look like it

<seanh> robertknight: That problem with the standalone annotation page sounds quite similar to the message not available problem in the sidebar: https://github.com/hypothesis/h/issues/3251 What I was thinking was that the initial API response (whether for all a group's annotations of a document, or for a single annotation) should include the first N replies to each top-level annotation; and then after that, there would b

API to get all replies to a given annotation. Does that sound like what's needed from the client side?

<seanh> nick: Completely agree that trying to always completely encapsulate sqla everywhere wouldn't be a good idea, especially not as an aim in and of itself. I was just suggesting it in simple cases not as a global or hard rule. (And by "functional" API I simply meant functions, like what `storage.py` has, "procedural API" might have been a better word for it.) But if I read right it sounds like you're happier with mo

cases); `views_test.py` (or whatever test module) _not_ mocking sqla or the `models.py` it's using; and then `views_test.py` and `models_test.py` would both be testing against the real `models.py` with potentially some test duplication. In cases where `models.py` is really simple it might not even merit its own `models_test.py`. Do I understand it right? That all sounds good to me

<robertknight> seanh: If I re-use the existing client class (`SearchClient`) for fetching search results then it won't matter what order the replies come back in. It would make sense however to send the top-level annotation first.

<seanh> To be honest those individual annotation pages should be handled entirely by server-side code anyway

<wyan> In https://github.com/hypothesis/h/pull/3375/commi... I'm trying to get the list of members in a FeatureCohort for the edit view to list, but I'm running into a bit of trouble. Whether I pass the list of members or the cohort, the renderer complains that it's not a dictionary.

<wyan> I'm getting a bit confused about what assumptions there are in the view/renderer

<robertknight> @seanh: Well, that depends on what the _ideal_ standalone annotation page looks like. Right now there is useful client-side functionality in those pages, such as the ability to reply to an annotation in response to receiving a notification about it.

<robertknight> For standalone pages, the important thing, assuming the desired behavior is what I suggested (that will need design input), is that there is a way to make a search query that takes an annotation (or reply) ID as input and returns all the annotations in that conversation thread.

<seanh> I shouldn't have said _entirely_ server side, progressive enhancement ++

<chdorner> @wyan: just having a look at your branch

<chdorner> @wyan: is it the cohort edit view?

<wyan> @chdorner: yep

<chdorner> so you're returning `cohort.members` and then access `results.name` (which would be the cohort name itself) at the top of the template

<chdorner> but it looks like a pyramid view has to return an iterable. so you can't return the cohort itself

<chdorner> you might want to return a dictionary like: ``` {"name": cohort.name, "members": cohort.members}

<chdorner> and then use this in the template

<wyan> @chdorner: I thought of that. But then I get a `UndefinedError: 'results' is undefined` error!

<chdorner> ok, let me check how that would work

<chdorner> ah, so when you return a dictionary, then you access the data with the key names as variables

<chdorner> `{"name": ..., "members": ...}` in the view, and then access it in the template like: `{{ name }}` / `{% for member in members %}`

<chdorner> My guess is that when you don't return a dictionary from the view then pyramid/jinja automatically creates a `results` variable for you

<wyan> oh that makes sense… I was really confused about the `results` variable

<wyan> yay! that seems to work

<wyan> thanks a lot @chdorner

<chdorner> :thumbsup: looking good!

<wyan> thanks :slightly_smiling_face: I'm going to `git commit` and `git push` and head to schöneweide now

<nick> @seanh: that sounds about right -- but I want to be clear I'm not averse to wrapping up common or fiddly model operations in helper functions (q.v. the document models).

<nick> And yes, a relatively simple model probably wouldn't merit many tests, beyond testing that you typed it correctly, otherwise you'd just be testing sqla.

<seanh> Helper functions - yes of course

<nick> But even stuff like `storage` is potentially problematic, because it's doing things like flushing prematurely, which in some circumstances might not be a good idea.

<nick> There's only so far you can abstract database operations into simple functions, because it involves interaction between two separate things: a request-scoped database session, and a bunch of model objects. You can't hide the session object without causing yourself a whole new class of problems (such as poor testability, lack of thread-safety, etc.)

<chdorner> one important downside of hiding all SQLAlchemy operations behind helper functions especially for fetching data is that it's too easy to end up with poorly-optimised queries which load data in a loop or things of that nature

<nick> Also, hiding the ORM would make things like `h.paginator` impossible.

<seanh> At no point did I ever even suggest encapsulating _all_ sqlalchemy :-)

<nick> Sure. It's worth reminding ourselves that one of the main reasons you mock the database is that database tests are slow(er). So, if you find yourself running loads of tests (parametrised, say) against some piece of model code in order to test the business logic, it would probably be better to factor out the logic entirely, and test it separately.

<nick> But realistically, database tests *aren't* that slow in our codebase because of the fancy transaction stuff we have wrapping out test harness. So for simple cases, just test against the database.

<chdorner> @nick: if you have a second (won't take much longer) to review this: https://github.com/hypothesis/h/pull/3371 - would make testing things locally much easier

<nick> lol

<nick> > Pull requests that have a failing status can’t be merged from a phone.

<nick> Feel free to merge it yourself!

<chdorner> haha

<robertknight> @nick: We were talking about building app.html/embed.js as part of the front-end build last week. Just wanted to check before I did anything in that direction whether you'd already started on that.

<robertknight> Working through setting up an environment for an extension only build yesterday with Conor it became clear that only needing to set up the toolchain for one language (ie. node, npm, gulp) would simplify things somewhat for such contributors.

<nick> Yep. I have been playing around with some options.

<nick> One thing I'd like to do is preserve as much of the git history as possible.

<nick> And that might be made a bit trickier by moving stuff around within the h repo.

<nick> Not impossible, but trickier.

<nick> So actually the way I've been thinking we could do this is to extract (with some cleanup) h/static/ into its own repo, and then delete from that the web service bits.

<nick> And then set up that repo to build to S3 and/or npm.

<nick> Both extension building and the web service docker build would use those built artifacts rather than running their own build of the client.

<nick> Does that sound vaguely sensible?

<nick> >>I'm now on the Eurostar and will be heading into a tunnel under the sea in a little while.<<

<robertknight> Yes, but I think it would be better to first make the app.html/embed.js build work entirely via the JS toolchain. Then the initial changes in the new repo would be purely removing non-JS code, after which we can move the files around.

<robertknight> OK, I'm heading out of a cafe at Gatwick airport back home.

<nick> @robertknight: maybe, but tbh I think that can come later.

<nick> Having app.html/embed.js stay in the h repo for now wouldn't be the worst thing in the world.

<nick> Yes, it makes the supposedly standalone client harder to use, but I don't know that it's actually a blocker.

<chdorner> @wyan: is #3330 good for reviewing?

<wyan> @chdorner: yep

nickstenn: do you have the link to the hack-day "pad" document?

I forgot to copy down the tinyurl

<conor> bigbluehat: http://tinyurl.com/iannohackday

much thanks conor!