search

SuffolkLITLab / docassemble-AssemblyLine

Quickly go from a paper court form to a runnable, guided, step-by-step web application powered by Docassemble. Swap out branding and pre-built questions to meet your needs.
https://suffolklitlab.org/docassemble-AssemblyLine-documentation/
MIT License
41 stars 5 forks source link

Look into a safe way to trigger definition of `defendants` and `plaintiffs` #623

Open nonprofittechy opened 1 year ago

BryceStevenWilley commented 1 year ago

Further details from discussion: https://github.com/SuffolkLITLab/docassemble-AssemblyLine/pull/596 stopped a bug that could occur from using plaintiffs too early in interviews, and switched to using user_ask_role and user_started_case instead.

This could cause issues in templates that are using user_role or plaintiffs directly somehow and also have skip undefined: True set; in that case, that information will just be blank.

We don't think this could be happening from newly-weaved forms, but it might be causing issues in production interviews, if plaintiffs isn't in the interview order but is in the template (same with user_role). We should do a quick search of production interviews to see if this is the case on any of them.

nonprofittechy commented 1 year ago

Probably a lot of interviews directly trigger def of plaintiffs if they use it, as that is the default output of the Weaver. But we should consider doing a quick scan of our production interviews so that we can confirm they have the right behavior.

nonprofittechy commented 1 year ago

I am not sure if this requires changing our code or just adding documentation when people write interviews from scratch? It seems like a pretty niche issue when I re-read it today.

BryceStevenWilley commented 1 year ago

It requires looking through our existing interviews and making sure this isn't happening. I don't think it's niche; you said that lots of interviews directly trigger plaintiffs, and we use skip undefined: True a lot.

Meta: it might be worth having a code survey label, since a lot of issues are "this might be an issue, but we don't know all of the places in our repos where it could be happening". GitHub search isn't quite powerful enough to find issues like this, but all of them do likely need some checklist of all of the repos we manage.

nonprofittechy commented 1 year ago

It's been a while, but I read this as saying that if the interview directly triggers plaintiffs (meaning in a mandatory code block) there's not a problem--and my comment was describing that any interview coming from the Weaver will do this the right way. The problem happens when someone uses plaintiffs in a template but they failed to force plaintiffs to be defined in a mandatory block.

In other words, this is a slightly special instance of using any intermediate variable in a place that won't force it to get defined, like a review screen or template.

I think when we opened this issue it was to handle a programming error and to decide if we can help developers avoid it by changing the code itself. The other option is just adding more examples and documentation.

We probably do need some way to handle "survey all of our repos" issues. Maybe just a tag is enough. But I worry that an idea to survey the code that we don't get to for > a year isn't a good use of the issue tracker.

It may also be worth adding a "developer experience" tag to help distinguish between new functionality, improved API to help developers, and changes to improve the experience of end users. "Refactor" mostly but doesn't entirely capture "developer experience." We probably should make sure we act on a good mix of those over the course of any given quarter.