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

Fix partial filename from paths_prefix #591

Merged
merged 1 commit into from
Jul 25, 2022
Merged

Fix partial filename from paths_prefix #591

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions dandiapi/api/views/asset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,8 @@ def paths(self, request, versions__dandiset__pk: str, versions__version: str, **
is_folder = folder_index >= 0

if not is_folder:
# Ensure base_path is entire filename
base_path = asset.path[asset.path.rfind('/') + 1 :]
Copy link
Member

Choose a reason for hiding this comment

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

could you please add a unittest for this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Apologies for never providing an update: I attempted to add a quick test for this, but ran into logistical issues. Since at the time I wasn't active on this project, and this enhancement came from work on another repository, I had to abandon this to continue my other obligations. Maybe now that I'm back on the project I can finish this up.

Copy link
Member

Choose a reason for hiding this comment

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

Welcome back @AlmightyYakob !

Copy link
Member

Choose a reason for hiding this comment

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

@AlmightyYakob please update on adding a unittest -- this is the eldest (6 mo?) PR ATM. We better decide on its destiny asap

Copy link
Member

Choose a reason for hiding this comment

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

@AlmightyYakob ping on this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding a unit test here would be nice, but requires a refactor of our testing setup, and doesn't provide much value, so I'll just merge this.

files[base_path] = asset
else:
base_path = base_path[:folder_index]
Expand Down