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.

Sign up for GitHub

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

Jump to bottom

Allow supporting files to be added to requests #175

Merged
merged 11 commits into from
Mar 19, 2024
Merged

Allow supporting files to be added to requests #175

merged 11 commits into from
Mar 19, 2024

Conversation

rebkwok
Copy link
Contributor

@rebkwok rebkwok commented Mar 15, 2024 •
edited
Loading

Fixes #128

  • Gives a RequestFile a type, either OUTPUT or SUPPORTING
  • A file can still (for now at least) only be added to a request once. I.e. a file can be either an output or a supporting file, but not both (it can't be an output in one group and a supporting file for another group)
  • RequestFileGroup still has files (which are output files) and now also has a supporting_files attribute
  • The tree highlights supporting files, and shows an alert in the content pane when viewing the file

Adding a file now has a field for filetype, defaulting to Output
Screenshot from 2024-03-15 16-09-24

Screenshot from 2024-03-15 18-05-02

@rebkwok rebkwok force-pushed the supporting-files branch 2 times, most recently from 369413f to 8fbf9a9 Compare March 15, 2024 18:06
bloodearnest
bloodearnest reviewed Mar 18, 2024
View reviewed changes
Copy link
Member

@bloodearnest bloodearnest left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks great, a lot of careful work here.

I have left a bunch of comments on various bits.

My only main question is how we build and store the information about supporting files in the nav tree. Think, rather than use relpath_to_html_class, output_files_with_group and supporting_files_with_group, we should build the supporting file info right into PathItem and the tree building.

Something like this:

  • Add method .is_supporting_file(relpath) to AirlockContainer
    • Workspace (always returns False).
    • ReleaseRequest (looks up RequestFile, returns True if supporting) (I just added a way to look up a file by path in my PR)
  • Add PathItem.supporting_file boolean property
  • Use that to add supporting class in .html_classes
  • When creating a PathItem in get_path_tree, set supporting=container.is_supporting_file(relpath)
  • In html_classes, add supporting if self.supporting

This way, the PathItem itself has all the information needed at construction time to a) render itself in the nav tree and b) render its file metadata/contents.

local_db/models.py Outdated Show resolved Hide resolved
airlock/views/workspace.py Outdated Show resolved Hide resolved
airlock/views/workspace.py
except bll.APIException as err:
# This exception is raised if the file has already been added
# (to any group on the request)
messages.error(request, str(err))
else:
messages.success(
request, f"File has been added to request (file group '{group_name}')"
request,
f"{filetype.name.title()} file has been added to request (file group '{group_name}')",
Copy link
Member

Choose a reason for hiding this comment

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

Nice improvement. I'm a big fan of messages containing as specific info as possible.

airlock/templates/file_browser/index.html Show resolved Hide resolved
tests/functional/test_e2e.py Show resolved Hide resolved
airlock/templates/file_browser/contents.html Outdated Show resolved Hide resolved
airlock/business_logic.py Outdated Show resolved Hide resolved
@rebkwok rebkwok force-pushed the supporting-files branch 6 times, most recently from 53ee532 to 3d64ec6 Compare March 18, 2024 13:44
@rebkwok
Copy link
Contributor Author

rebkwok commented Mar 18, 2024

@bloodearnest sorry for stupid question. You said:

I just added a way to look up a file by path in my PR

Can you point me at what you're referring to here? I'm looking at your iframes PR and I can't see an obvious lookup-RequestFile-by-path sort of thing.

@bloodearnest
Copy link
Member

@bloodearnest sorry for stupid question. You said:

I just added a way to look up a file by path in my PR

Can you point me at what you're referring to here? I'm looking at your iframes PR and I can't see an obvious lookup-RequestFile-by-path sort of thing.

I just realised its in a WIP branch, not yet committed!

Let me get it up

bloodearnest
bloodearnest approved these changes Mar 18, 2024
View reviewed changes
Copy link
Member

@bloodearnest bloodearnest left a comment

Choose a reason for hiding this comment

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

Brilliant, thanks for making those changes!

airlock/file_browser_api.py Outdated Show resolved Hide resolved
@rebkwok
Refactor supporting file info onto PathItem
c044a26 
Give a PathItem knowledge of whether it is a supporting
file or not.
@rebkwok rebkwok merged commit 7f9bc14 into main Mar 19, 2024
8 checks passed
@rebkwok rebkwok deleted the supporting-files branch March 19, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bloodearnest bloodearnest bloodearnest approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow supporting files to be added to a request
2 participants
@rebkwok @bloodearnest