Phillip/test fileupload #55
Conversation
Codecov Report
|
ldb
left a comment
ldb
left a comment
There was a problem hiding this comment.
Is it possible that this PR contains changes from previous branches that were since merged with master? If yes, can you please rebase to remove that excess code? Will continue the review then.
| ### Registration | ||
| - `/api/register` Register a new user. | ||
|
|
||
| ```json |
http/courseEntryHandler_test.go
Outdated
| } | ||
|
|
||
| service.StoreCourseEntryFilesFn = func(files [][]byte, id string, date time.Time) (error, []string) { | ||
| if len(files) == 0 { |
There was a problem hiding this comment.
Maybe use a switch construct here?
| privateChain := Chain(protected, Logger(a.Logger), CORS, NewAuthMiddleware(a.UserService)) | ||
| publicChain := Chain(public, Logger(a.Logger), CORS) | ||
| staticChain := Chain(http.FileServer(http.Dir(a.Static)), Logger(a.Logger), CORS) | ||
| filesChain := Chain(http.StripPrefix("/files", http.FileServer(http.Dir(a.Files))), Logger(a.Logger), CORS) |
There was a problem hiding this comment.
Shouldn't it be Chain(http.FileServer(http.Dir(a.Files))), http.StripPrefix("/files")...? The first argument of Chain is the final handler.
There was a problem hiding this comment.
http.StripPrefix takes in a string that will be removed from the beginning from the handlers path
So it needs to take in both arguments
phiphi282
force-pushed
the
phillip/test_fileupload
branch
from
4e85910 to
a407051
Compare
| } | ||
|
|
||
| type Uploader interface { | ||
| UploadFile(file []byte, course string, filename string) (error, string) |
There was a problem hiding this comment.
error should be the last value returned.
| } | ||
| } | ||
|
|
||
| type Uploader interface { |
There was a problem hiding this comment.
What is this Interface used for? Wasn't the file uploaded to this server?
There was a problem hiding this comment.
The interface is used for mocking the upload function in our tests
| return nil, entry | ||
| } | ||
|
|
||
| func (cES CourseEntryService) StoreCourseEntryFiles(files [][]byte, id string, date time.Time) (error, []string) { |
upload/upload.go
Outdated
| type Uploader struct{} | ||
|
|
||
| func (u *Uploader) UploadFile(file []byte, course string, filename string) (error, string) { | ||
| // check content type |
http/courseEntryHandler.go
Outdated
| } | ||
|
|
||
| pURLs, err := url.URLifyStrings(request.Pictures) | ||
| err, paths := a.CourseEntryService.StoreCourseEntryFiles(request.Pictures, id, request.Date) |
There was a problem hiding this comment.
This seems unusual to me. Aren't files usually uploaded using multipart/form-data? Also, what kind of data is the file content here? Is is Base64 encoded? I did not see any decoding in this PR. As a last thought, there is no limit on file size.
There was a problem hiding this comment.
the json decoder decodes the base64 encoded bytes-file automagically.
The filesize is not limited yet.
courseEntry.go
Outdated
|
|
||
| type CourseEntryService interface { | ||
| StoreCourseEntry(entry *CourseEntry, cfu CourseFindUpdater) (err error, courseEntry *CourseEntry) | ||
| StoreCourseEntryFiles(files [][]byte, id string, date time.Time) (err error, paths []string) |
There was a problem hiding this comment.
What do you think about passing a []io.Reader here instead of [][]byte? That would make any storing / uploading etc way more flexible.
added file upload
it works
I would prefer to not make tests for upload.go for now