Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor of interview list page #787

Merged
merged 6 commits into from
Oct 31, 2023
Merged

Conversation

nonprofittechy
Copy link
Member

@nonprofittechy nonprofittechy commented Oct 21, 2023

Here's the high level overview (also possible to get it from the code. currently this message is duplicated in one long comment at the top of the file)

Customize by:

  1. Editing the global configuration (see below), or
  2. Including this file and overriding the default values in a replacement YAML file

Relevant options and defaults:

  app homepage: https://courtformsonline.org
  assembly line:
    enable answer sets: True
    interview list:
      page title: In progress forms # Title of the tab in your browser
      page question: In progress forms
      page subquestion: null
      new form label: Start a new form
      new form url: https://courtformsonline.org
      logo url: null # Defaults to `app homepage`
      logo image url: https://app.yourserver.org/packagestatic/docassemble.AssemblyLine/ma_logo.png
      logo image alt: Logo
      answer sets title: Answer sets
      logo title row 1: null # Defaults to `interview page title`
      logo title row 2: null # Defaults to `interview page heading`
      exclude from interview list:
        - docassemble.AssemblyLine:data/questions/al_saved_sessions_store.yml
        - docassemble.AssemblyLine:data/questions/interview_list.yml

interview page title, interview page heading, interview page pre, logo url, interview page url, and new form url
can also be set to a dictionary of languages, like:

    interview page title:
      en: In progress forms
      es: Formularios en progreso

These deprecated configuration options will also be respected at a lower priority:

  new form url: https://courtformsonline.org
  assembly line:
    exclude from interview list:
      - docassemble.AssemblyLine:data/questions/al_saved_sessions_store.yml
      - docassemble.AssemblyLine:data/questions/al_saved_sessions_store.yml

As will the following options from the global config:

    interview page title
    interview page heading
    interview page pre

Some authors will prefer to customize this page by importing it and overriding the default values
in the YAML file rather than the global configuration. This is slightly more advance than editing
the configuration, but may be just as easy to maintain.
To do this, make a new YAML file and include your custom theme/settings YAML.
It's important to include your theme AFTER the interview list page:

e.g., my_new_interview_list.yml would only need to contain:

  ---
  include:
    - docassemble.AssemblyLine:data/questions/interview_list.yml
    - docassemble.MyState:my_custom_theme.yml
  ---
  metadata:
    title: |
      In progress forms
    short title: |
      In progress forms

Then you would set the following in your global config:

  interview list: docassemble.MyState:my_new_interview_list.yml

In some instances, this may be all that is required to use your branding on the
interview list page. You can individually override additional variables as follows:

* PAGE_QUESTION
* PAGE_SUBQUESTION
* NEW_FORM_LABEL
* NEW_FORM_URL
* al_sessions_to_exclude_from_interview_list

The following settings are usually best to leave undefined if you want
the ordinary behavior of your theme:
* PAGE_TITLE
* LOGO_URL
* LOGO_IMAGE_URL
* LOGO_IMAGE_ALT
* LOGO_TITLE_ROW_1
* LOGO_TITLE_ROW_2 (defaults to value from the metadata block)

And of course you can also override the YAML block by block if you want to
further customize it.

Copy link
Collaborator

@plocket plocket left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking pretty solid to me. A few thoughts:

My main question is does a regular interview control these in the same way? That is, do regular interviews use the config by default too, etc? I think it'd be great to have them aligned, and maybe they already are.

Another general question: Are the consts in here, like AL_ORGANIZATION_TITLE, for backwards compatibility? I'm trying to think about which should take precedence and I'm not sure what a user would expect. I guess the question is whether the new code would change the behavior of existing interview list page without the user being aware. Would they change their config without expecting it to affect their existing interview list page? I think I'm not familiar enough with how folks have been using these to have a useful take on this.

Smaller questions:

  1. interview page title/heading make me think it's for any interview page, though being under interview list helps.
  2. Not a new thing, but something I've only just now articulated to myself - I'm not sure of the difference between interview page title and interview page heading

Other than that, I'm going to avoid details as I can see those are still changing and improving 👍

@nonprofittechy
Copy link
Member Author

nonprofittechy commented Oct 23, 2023

This very deliberately does not work in the same way as pages that are normally included inside another package. It's installed systemwide and is made so people can change its behavior without forking it. Forking and then changing the global config to point to your custom version of the file is a pretty advanced technique at the moment, while this basic version of the file is installed by the dashboard install script.

I think I described this in more detail on our Friday call.

In contrast, the assembly_line.yml and dependencies are all included into another interview. You can override and control their behavior naturally, by adding a different value for the variable in the file that you already are editing.

Edit: another point is that this is a server wide file. It's not uncommon to co-host orgs on one server. Like Suffolk hosts Northeast Legal Aid and MassLegalHelp content.

Does that all make sense?

The use of AL_ORGANIZATION_HOMEPAGE, etc is only for backwards compatibility with the custom version of this that @BryceStevenWilley made. I didn't document that intentionally. I'm interested to know if he's open to changing or removing his fork or if he thinks this backwards compatibility is still useful.

Copy link
Contributor

@BryceStevenWilley BryceStevenWilley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm interested to know if he's open to changing or removing his fork or if he thinks this backwards compatibility is still useful.

Yeah, please keep the backwards compatibility. IMO, the maintenance and mental burden of 3 lines of YAML in a wrapper interview is much simpler than 15 lines of config, especially for things like logo alt text and interview page pre. (to be clear, I'm apparently in the minority here, disregard my opinion I guess). It's nice that the configs are all in the same place now, but I would 100% need to go look at the actual interview to figure out the difference between "interview page title", "interview page heading", and "interview page pre".

@nonprofittechy
Copy link
Member Author

nonprofittechy commented Oct 23, 2023

I'm interested to know if he's open to changing or removing his fork or if he thinks this backwards compatibility is still useful.

Yeah, please keep the backwards compatibility. IMO, the maintenance and mental burden of 3 lines of YAML in a wrapper interview is much simpler than 15 lines of config, especially for things like logo alt text and interview page pre. It's nice that the configs are all in the same place now, but I would 100% need to go look at the actual interview to figure out the difference between "interview page title", "interview page heading", and "interview page pre".

Agreed that the names are not the clearest. The only reason I'm sticking with those names is because they are existing names from the Docassemble global configuration.

See: https://docassemble.org/docs/config.html#customization

Co-authored-by: Bryce Willey <Bryce.Steven.Willey@gmail.com>
@BryceStevenWilley
Copy link
Contributor

The only reason I'm sticking with those names is because they come from the Docassemble global configuration.

Why? It makes the fallback code you have a few lines shorter, but I thought the point of this PR was to move configs around and make it cleaner. You could add some params to the get_config_fallback function so it has both the new name and the old name.

Agreed that the names are not the clearest.

The names could be better, but to be clear even with better names, I still feel like I'd need to look at the actual interview.

@nonprofittechy
Copy link
Member Author

The only reason I'm sticking with those names is because they come from the Docassemble global configuration.

Why? It makes the fallback code you have a few lines shorter, but I thought the point of this PR was to move configs around and make it cleaner. You could add some params to the get_config_fallback function so it has both the new name and the old name.

Agreed that the names are not the clearest.

The names could be better, but to be clear even with better names, I still feel like I'd need to look at the actual interview.

Any suggestions for better names? This is open to @plocket too.

Cost/benefit of renaming the options that mirror Docassemble's existing options for doing the identical task seems low to me. But I haven't thought of names that are a lot better--maybe it will be worth it?

@BryceStevenWilley
Copy link
Contributor

Any suggestions for better names?

If they matched the DA attribute they're used in, that would be nice

  • interview_page_title could stay the same
  • interview_page_header -> interview_page_question
  • interview_page_pre -> interview_page_subquestion

Cost/benefit of renaming the options that mirror Docassemble's existing options for doing the identical task seems low to me

My general take (idk how right it is) is that DA has 1,000 things to keep track of; unless it's something commonly used in writing an interview, most people won't know what they are, would have to look it up, and probably struggle to do so in docassemble's documentation (i.e. my workflow with settings like this).

@plocket
Copy link
Collaborator

plocket commented Oct 23, 2023

Bryce's choices make more sense to me on principle. I do wish list could be in the name somewhere, but unless we do list_page_subquestion or something like that, it gets pretty long.

@nonprofittechy nonprofittechy marked this pull request as ready for review October 27, 2023 16:54
@nonprofittechy nonprofittechy merged commit dca6959 into main Oct 31, 2023
5 checks passed
@BryceStevenWilley BryceStevenWilley deleted the interview-list-refactor branch November 1, 2023 16:21
nonprofittechy added a commit to SuffolkLITLab/docassemble-AssemblyLine-documentation that referenced this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants