Skip to content

Add Flask interview list endpoint - #1104

Open
rajeswari1301 wants to merge 9 commits into
mainfrom
flask-endpoint
Open

rajeswari1301 wants to merge 9 commits into
mainfrom
flask-endpoint

Conversation

@rajeswari1301

Copy link
Copy Markdown
Contributor

Replaced the redundant session query in interview_list.yml, which was executing 2–3 times per page load and now runs only once, tested it at 50 - 2375 saved sessions - and it was about about 2-3 times faster each time, getting better the bigger the list gets.

It supports all the same features - viewing, searching, renaming, deleting, copying, and paging through results and pulls its settings from the same config the old page already uses.

#1102

@nonprofittechy nonprofittechy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor nits. Biggest question to consider: should this definitely use HTML encoded into the file? An alternative is to use a traditional external HTML template. You'd need to load it in as a string though given that Docassemble only looks for Flask templates in one hardcoded folder.

Comment thread docassemble/AssemblyLine/interview_list_endpoint.py Outdated
@rajeswari1301

Copy link
Copy Markdown
Contributor Author

Minor nits. Biggest question to consider: should this definitely use HTML encoded into the file? An alternative is to use a traditional external HTML template. You'd need to load it in as a string though given that Docassemble only looks for Flask templates in one hardcoded folder.

Thank you for the review! I fixed the CSS and package name.

Right now, the page wrapper currently uses a separate file. Registering the package's template folder directly with flask would be the fix rather than another workaround, but that would mean updating every link on the page and handling the check that stops this route from loading twice

@nonprofittechy

Copy link
Copy Markdown
Member

Minor nits. Biggest question to consider: should this definitely use HTML encoded into the file? An alternative is to use a traditional external HTML template. You'd need to load it in as a string though given that Docassemble only looks for Flask templates in one hardcoded folder.

Thank you for the review! I fixed the CSS and package name.

Right now, the page wrapper currently uses a separate file. Registering the package's template folder directly with flask would be the fix rather than another workaround, but that would mean updating every link on the page and handling the check that stops this route from loading twice

We cannot register the package's template folder. But we can put templates in the data/sources or data/templates folder, and read them in as a string using a normal filesystem operation.

I think that would be more readable and maintainable than hardcoding the template in the Python module. What do you think?

Copy link
Copy Markdown
Member

Expanding on the suggestion to move the HTML out of interview_list_endpoint.py, I think we can make the separation fairly complete without registering a new Flask template directory.

Proposed implementation plan

  1. Keep the current filesystem-loading approach. Put the full page template in a package data directory (for example data/templates/al_interview_list_page.html or another directory we know is packaged), load it with Path(...).read_text(), and continue to use render_template_string(). This avoids depending on Flask template-path registration.

  2. Move essentially all page markup into the Jinja template. The template should own the heading, new-form button, intro text, search/filter form, saved-session table, rename/copy/delete controls, pagination, and delete-all form. The current Python helpers that return HTML strings (_toggleable_action_icon(), _toggleable_action_form(), _render_session_rows()) can then go away.

  3. Use Jinja macros for repeated UI pieces. Rename/copy controls are good candidates for macros. This keeps the template readable while still making the presentation reusable.

  4. Have the Flask route prepare data, not HTML. The GET route should be responsible for request parsing, fetching/filtering sessions, config, CSRF token generation, filename choices, and pagination state, then pass those values into the template. Something along the lines of:

    return render_template_string(
        PAGE_TEMPLATE,
        sessions=sessions,
        filename_options=filename_options,
        cfg=cfg,
        csrf_token=csrf_token,
        page=page,
        keyword=keyword,
        limit_filename=limit_filename,
        page_size=PAGE_SIZE,
        package_name=_package_name(),
    )
  5. Optionally prepare a small view-model for each session in Python. For example, Python can calculate title, subtitle, interview URL, formatted modification date/time, and progress markup. That keeps AssemblyLine-specific helper calls out of the template while still leaving the actual HTML structure in Jinja.

  6. Let Jinja handle normal escaping. Right now a large generated HTML blob ultimately gets inserted with {{ content|safe }}. Moving the markup into Jinja lets normal values use Jinja autoescaping and limits |safe to values that are intentionally trusted HTML (for example, if radial_progress() returns markup). That gives us a safer default for future edits.

  7. Keep CSS in interview_list.css, and ideally move inline behavior/style out over time. The current inline style=, onclick=, and onsubmit= attributes can work initially, but once the HTML is in a real template they will also be much easier to migrate to CSS/JS if desired.

  8. Leave route registration and endpoint names alone. The existing if "al_interview_list" not in app.view_functions: guard can stay exactly where it is, and the template can call url_for() directly, so moving the markup should not require changing the route-loading strategy or URLs.

  9. Add/adjust tests around rendered behavior rather than generated strings. At minimum I would cover: empty list, normal rows, answer-set controls on/off, search/filter state preserved, pagination links preserving query parameters, and escaping of user-controlled titles/names.

The end state I have in mind is: Python handles auth/request/data/mutations; Jinja handles layout and conditional presentation; CSS handles styling. That should make this endpoint considerably easier to review and maintain while keeping the current docassemble/Flask constraints intact.

Moved the page's HTML out of Python and into the Jinja template. The route now just hands the template the data it needs - session info, config, page and search state and lets Jinja render it, with its autoescaping doing the job manual escape() calls used to do. Also moved the template file into data/templates and added tests covering the empty list, normal rows, search and pagination, the answer-sets toggle, and escaping of session titles.
@rajeswari1301

Copy link
Copy Markdown
Contributor Author

Minor nits. Biggest question to consider: should this definitely use HTML encoded into the file? An alternative is to use a traditional external HTML template. You'd need to load it in as a string though given that Docassemble only looks for Flask templates in one hardcoded folder.

Thank you for the review! I fixed the CSS and package name.
Right now, the page wrapper currently uses a separate file. Registering the package's template folder directly with flask would be the fix rather than another workaround, but that would mean updating every link on the page and handling the check that stops this route from loading twice

We cannot register the package's template folder. But we can put templates in the data/sources or data/templates folder, and read them in as a string using a normal filesystem operation.

I think that would be more readable and maintainable than hardcoding the template in the Python module. What do you think?

Thanks for the thorough review and suggestion! I checked the existing conventions in this repo, and since ALDashboard already stores full-page templates in data/templates, I moved the HTML out of the python module and into 'data/templates/al_interview_list_page.html'. I also refactored the helper functions, updated the tests to cover the rendered template, it's ready for another look whenever you have a chance. Thanks again!

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.

2 participants