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

[Document Repository] Fixing bug in pathing #4466

Merged
merged 2 commits into from
Jun 3, 2019
Merged

[Document Repository] Fixing bug in pathing #4466

merged 2 commits into from
Jun 3, 2019

Conversation

Jkat
Copy link
Contributor

@Jkat Jkat commented Apr 18, 2019

Brief summary of changes

Got rid of $fullPath usage. Code was checking for a directory named by the file, creating a directory named by the file, and uploading the file inside this new directory.

This resolves issue...

Fixes uploading and downloading files in Document Repository

To test this change...

  • upload and download a file

@Jkat Jkat added the Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) label Apr 18, 2019
@driusan
Copy link
Collaborator

driusan commented Apr 18, 2019

Is this supposed to fix #4462?

@driusan driusan requested a review from johnsaigle April 18, 2019 17:57
@Jkat
Copy link
Contributor Author

Jkat commented Apr 18, 2019

@driusan I think my PR covers a bit more but yes it should fix #4462 completely and we can close the issue after merging

@zaliqarosli zaliqarosli added the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Apr 29, 2019
@zaliqarosli
Copy link
Contributor

will approve after rebase

@zaliqarosli zaliqarosli added the Passed Manual Tests PR has undergone proper testing by at least one peer label Apr 29, 2019
johnsaigle
johnsaigle reviewed May 7, 2019
View reviewed changes
Copy link
Contributor

@johnsaigle johnsaigle left a comment

Choose a reason for hiding this comment

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

Looks good. Will test and rereview after rebase.

@johnsaigle
Copy link
Contributor

Tagging this with the 21 release because the fix is basically complete and this problem shouldn't be carried forward into v21.

@zaliqarosli
Copy link
Contributor

@johnsaigle is there a github issue reported from 21.0.0 testing relating to this? i'm going to rebase and change this branch

@zaliqarosli zaliqarosli changed the base branch from bugfix to 21.0-release May 14, 2019 19:10
@zaliqarosli zaliqarosli changed the base branch from 21.0-release to bugfix May 14, 2019 19:11
@zaliqarosli zaliqarosli changed the base branch from bugfix to 21.0-release May 14, 2019 19:11
@zaliqarosli zaliqarosli force-pushed the document-repository-pathing-bug-fix branch 3 times, most recently from 16351ba to c142b41 Compare May 14, 2019 19:20
@zaliqarosli zaliqarosli removed the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label May 14, 2019
@ridz1208 ridz1208 added the Blocked PR awaiting the merge, resolution or rejection of an other Pull Request label May 14, 2019
@ridz1208
Copy link
Collaborator

Blocked to not conflict with the reactification of the module

@ridz1208 ridz1208 added this to the 21.0.0 milestone May 17, 2019
@driusan
Copy link
Collaborator

driusan commented May 27, 2019

The document repo was almost entirely rewritten in #3971 (which unsurprisingly is causing conflicts).. is this still an issue? If so this PR needs to be rebased (or probably more easily just redone on the latest branch.)

@zaliqarosli
Copy link
Contributor

i will fix this PR!

@zaliqarosli zaliqarosli added Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch labels May 27, 2019
@driusan driusan removed the Blocked PR awaiting the merge, resolution or rejection of an other Pull Request label May 27, 2019
@zaliqarosli zaliqarosli force-pushed the document-repository-pathing-bug-fix branch from c142b41 to ca80e11 Compare May 27, 2019 20:24
@zaliqarosli zaliqarosli removed Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix labels May 27, 2019
@zaliqarosli zaliqarosli mentioned this pull request May 27, 2019
Closed
@zaliqarosli
Copy link
Contributor

@johnsaigle please have a look at this guy if you have time!

zaliqarosli added a commit to zaliqarosli/Loris that referenced this pull request May 28, 2019
@zaliqarosli
revert code from aces#4466
bc1c3f1
@johnsaigle
Copy link
Contributor

Looks fine and passed manual tests for uploading; the back-end now properly puts the file where it should go.

Downloading is still broken however but I think the cause is different. IMO this PR should be merged and further work will be done after to figure out what's going wrong.

@zaliqarosli
Copy link
Contributor

@johnsaigle yepp, the broken downloading is a separate issue that isn't taken care of in this PR. please hit approve if all good!

@driusan driusan merged commit 9e8d23c into aces:21.0-release Jun 3, 2019
nicolasbrossard pushed a commit to nicolasbrossard/Loris that referenced this pull request Jun 26, 2019
@Jkat @nicolasbrossard
[Document Repository] Fix duplicate filename in path (aces#4466)
6cc6051 
Got rid of $fullPath usage. Code was checking for a directory named by the file, creating a directory named after the file, and uploading the file inside this new directory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnsaigle johnsaigle johnsaigle approved these changes

Assignees
No one assigned
Labels
Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) Passed Manual Tests PR has undergone proper testing by at least one peer
Projects
None yet
Milestone
21.0.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Jkat @driusan @zaliqarosli @johnsaigle @ridz1208