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

Fix prefix_jobid issue #128

Merged
merged 9 commits into from
Apr 23, 2022
Merged

Fix prefix_jobid issue #128

merged 9 commits into from
Apr 23, 2022

Conversation

tcmoore3
Copy link
Member

@tcmoore3 tcmoore3 commented Apr 21, 2022

Description

See #127. After a lot of testing, I have found that the only way to get the desired behavior for the default download name is to add as_attachement and download_name to where we call flask.send_from_directory() in views.get_file(). This initial change is obviously a bad solution: I'm hard-coding the filename. Some observed behavior:

  • If I set as_attachment=True and do not send anything in download_name, I still do not get the correct default download file name in my Save As dialog box. The only thing this seems to change is the Content-Disposition response header, which changes from inline; filename=filename.txt when as_attachment=False to attachment; filename=energy-plot-timesteps.txt when as_attachment=True
  • If I set as_attachment=True and set an undefined variable as download_name (e.g., download_name=x), the correct filename shows up on the download box, but a TypeError is thrown and the download errors out
  • If I set as_attachment=True and pass any string into download_name, the dowloaded file's name is that string by default

Motivation and Context

Resolves #127

I do not know how to proceed. I cannot figure out how to get the correct download name into that function. Any pointers would be greatly appreciated.

Checklist:

@tcmoore3 tcmoore3 requested review from bdice and a team as code owners April 21, 2022 21:25
@bdice
Copy link
Member

bdice commented Apr 23, 2022

I'm going to try and fix this, but I want to lay out some architectural information so that others may hopefully feel less confused by the layout of code. If this is helpful, we can drop this info into developer docs or ARCHITECTURE.md or something like that. My time to work on signac-dashboard is limited, so I want to help others gain the ability to work independently and make fixes / features.

  1. Flask is a minimalistic web framework and doesn't impose a specific design pattern. The signac-dashboard Flask application loosely adopts an extremely popular design pattern known as Model View Controller. This article describes it and is essential reading for the remainder of this post: https://realpython.com/the-model-view-controller-mvc-paradigm-summarized-with-legos/
    a. The "model" is the signac Project which manages, reads, and writes data from the signac database. In most websites, this is backed by a SQL or NoSQL database and interactions occur through a library like SQLAlchemy. There is very little code in this package for the model -- it's mostly handled by the signac package.
    b. The "controller" is the signac-dashboard Dashboard. It manages the application state as a whole and responds to a set of defined routes. The Dashboard class is extensible, allowing user-supplied modules that can add functionality and alter the application behavior. This is mostly encapsulated in dashboard.py.
    c. The "view" is the function that produces the output shown in the browser (or sometimes used to refer to the output itself). There are several views defined in views.py, and they render templates from the templates directory in accordance with Flask conventions.

  2. As described above, Flask apps have routes. That's the key thing we need to use here.
    a. Routing is a process that takes an incoming request like /jobs/abc123/file/grumpycat.jpg and determines what Python function to call (which view to use), which in turn determines what content to return.
    b. Most of the common routes are added through Dashboard.add_url. These are added in _register_routes, which is called in _prepare, which is called on construction in Dashboard.__init__:

    self.add_url("views.home", ["/"])
    self.add_url("views.settings", ["/settings"])
    self.add_url("views.search", ["/search"])
    self.add_url("views.jobs_list", ["/jobs/"])
    self.add_url("views.show_job", ["/jobs/<jobid>"])
    self.add_url("views.get_file", ["/jobs/<jobid>/file/<path:filename>"])
    self.add_url("views.change_modules", ["/modules"], methods=["POST"])

    c. Note: We use Flask's add_url_rule instead of the simpler @app.route decorator because signac-dashboard specifies some additional options, e.g. using lazy views. (I don't remember all the architectural reasons/constraints for this at the moment but I spent a lot of time testing different API patterns and settled on this.)

(Submitting this comment now so I don't lose my work. I may edit later.)

Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

@tcmoore3 @cbkerr This looks fine to me. As far as I can tell, the current state of this PR is working as intended.

@bdice
Copy link
Member

bdice commented Apr 23, 2022

Please merge in master to get the changes from #129 and add a changelog entry before merging.

@bdice bdice mentioned this pull request Apr 23, 2022
5 tasks
@cbkerr cbkerr merged commit 0b27c1c into master Apr 23, 2022
@cbkerr cbkerr deleted the fix/prefix-jobid branch April 23, 2022 19:09
@tcmoore3
Copy link
Member Author

Confirming that this fix gives me the desired behavior and appoving.

Copy link
Member Author

@tcmoore3 tcmoore3 left a comment

Choose a reason for hiding this comment

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

LGTM

@cbkerr cbkerr added this to the v0.3.0 milestone Jun 22, 2022
@cbkerr cbkerr mentioned this pull request Jun 22, 2022
javierbg pushed a commit to javierbg/signac-dashboard that referenced this pull request Nov 28, 2022
* Add as_attachment and download_name to get_file()

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add optional download_name to get_file requests

* [pre-commit.ci] auto fixes from pre-commit.com hooks

* Remove comment.

* Use kwarg.

* Update changelog

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Corwin Kerr <cbkerr@umich.edu>
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 63.21%. Comparing base (a070f06) to head (b33ddf4).

Files with missing lines Patch % Lines
signac_dashboard/views.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #128      +/-   ##
==========================================
- Coverage   63.30%   63.21%   -0.10%     
==========================================
  Files          18       18              
  Lines         665      666       +1     
==========================================
  Hits          421      421              
- Misses        244      245       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

FileList not respecting prefix_jobid on HPC cluster
3 participants