-
-
Notifications
You must be signed in to change notification settings - Fork 214
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
[265] Add file upload to allow mocking of file operations #266
Conversation
Codecov Report
@@ Coverage Diff @@
## master #266 +/- ##
==========================================
+ Coverage 90.22% 90.45% +0.23%
==========================================
Files 104 105 +1
Lines 5380 5500 +120
==========================================
+ Hits 4854 4975 +121
+ Misses 526 525 -1
Continue to review full report at Codecov.
|
Thanks for this idea + implementation. I'll refactor the code in this branch to match the solution logic. |
@JackCreativeCrew |
Yeah thanks, looks good. Mine wasn't using the already provided functions, sorry I only started using this ~3 days ago. The rest of the code looks good though. The only functional change I'd suggest is returning the path of the created/updated file when put/posting, reason being I'm testing files going through a rest API and I'd like to access them after they've been uploaded etc from another program which would require the path. EDIT: Also I can't approve this because it's mine but I'm ok to merge if you are. |
…gin with, otherwise the file upload fails with 404 (can't find the folder to upload to).
Minor change to create the __admin/mapping folder if it doesn't exist otherwise the file upload fails. I'd imagine this is a problem in other places too (scenarios etc)? |
Actually I was just thinking about that scenario. Also your remark bout returning the path. And actually, I think this is also a security risk to allow sub-folders or different paths like So I think I will add logic to the code to take the filename only and change path into filename in the interface |
Yeah, ok, I hadn't considered that. Something else I've come across, the file is being written as everything in the multipart body which isn't just the file. I've stuffed around trying to get this to write just the file but as yet failed.
at the head and
at the tail. At the moment I'm only using one file i.e. that file, is there any way to only write the first piece of content and just chop the rest? |
First I will fix the unit-tests |
Second |
I must be doing something wrong on my end then. Thanks for checking that out. |
If you have tested all scenarios in this branch, and are confident that this code is correct, I can merge this PR and create a new NuGet. |
I haven't had time today to review/check my stuff against this today sorry. I'll get back to you by the end of tomorrow Sydney time though. |
…urning it as a body.
Ok I've added a HEAD endpoint as well and a test for both file found and not found. It requires no body as the HEAD request can't have a body so the structure is a bit different. The code is now all working for me and if you accept those changes, I'm ok with merging it. |
Head is a good idea. I did some small updates on code + tests, and I added this call also to the Also removed an unused method from "MappingConverter.cs" ? |
Yeah, I was using that for debugging which admin endpoints were available, must've not gotten deleted before the commit. |
I'll merge to master now. |
I've added endpoints for file upload/download/deletion (PUT/GET/DELETE) to allow for testing using files. The code is pretty basic but fulfills my requirements at the moment and has test coverage of happy paths.