Skip to content
This repository has been archived by the owner on Aug 10, 2023. It is now read-only.

Make allowed file endings configurable #271

Merged
merged 5 commits into from Apr 4, 2019
Merged

Conversation

PhilippMatthes
Copy link
Collaborator

Fixes: #205

@PhilippMatthes PhilippMatthes added the enhancement New feature or request label Mar 18, 2019
@PhilippMatthes PhilippMatthes self-assigned this Mar 18, 2019
Copy link
Contributor

@martinmo martinmo left a comment

Choose a reason for hiding this comment

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

Thanks @PhilippMatthes, this is a good starting point. Please address my comments and also provide unit tests. Then it should be good to go.

inloop/solutions/validators.py Outdated Show resolved Hide resolved
inloop/solutions/validators.py Show resolved Hide resolved
inloop/solutions/validators.py Outdated Show resolved Hide resolved
inloop/solutions/validators.py Outdated Show resolved Hide resolved
inloop/solutions/views.py Outdated Show resolved Hide resolved
inloop/solutions/views.py Outdated Show resolved Hide resolved
@PhilippMatthes PhilippMatthes dismissed martinmo’s stale review March 19, 2019 14:35

Martin's suggestions were taken into account and added to this branch.

Copy link
Contributor

@martinmo martinmo left a comment

Choose a reason for hiding this comment

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

Looks better, still some points to address. Also, we need to think about case-sensitive vs. case-insensitive. Neither the constance help text nor the test cases mention (or test, respectively) case insensitivity.

inloop/solutions/validators.py Outdated Show resolved Hide resolved
inloop/solutions/validators.py Outdated Show resolved Hide resolved
inloop/solutions/validators.py Outdated Show resolved Hide resolved
inloop/solutions/validators.py Outdated Show resolved Hide resolved
@PhilippMatthes
Copy link
Collaborator Author

@martinmo I addressed your recommendations and added another test. Please look into it again. Thanks!

@PhilippMatthes PhilippMatthes dismissed martinmo’s stale review March 20, 2019 07:50

This review is outdated.

Copy link
Contributor

@martinmo martinmo left a comment

Choose a reason for hiding this comment

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

I've added comments of the third review round. There are still some very minor issues.

Concerning the term "file ending", I would prefer to use "filename extension" instead. File ending is not accurate and could also refer to the last character of the contents of a file. Wikipedia also refers to that name (https://en.wikipedia.org/wiki/Filename_extension).

inloop/solutions/validators.py Show resolved Hide resolved
inloop/solutions/constance.py Outdated Show resolved Hide resolved
inloop/solutions/validators.py Outdated Show resolved Hide resolved
@PhilippMatthes PhilippMatthes dismissed martinmo’s stale review March 21, 2019 09:09

Suggestions were taken into account.

@PhilippMatthes PhilippMatthes requested review from martinmo and removed request for martinmo March 21, 2019 09:09
@martinmo martinmo force-pushed the 205-file-upload-pattern branch 2 times, most recently from f9be5ae to 017ca95 Compare March 21, 2019 15:38
Copy link
Contributor

@martinmo martinmo left a comment

Choose a reason for hiding this comment

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

The validation fails with an AttributeError if I use the manual upload. See comments for the reason. I suggest to adapt the validation function to take a list of filenames as an argument. Use upload.keys() when it is a dict. Use a list comprehension when it is a list of objects.

Update: in the second case, it's of course not just a list of filenames, but a list of file objects instead.

inloop/solutions/views.py Show resolved Hide resolved
inloop/solutions/views.py Show resolved Hide resolved
@PhilippMatthes
Copy link
Collaborator Author

@martinmo you are right. Didn't think about that one.

@PhilippMatthes
Copy link
Collaborator Author

I'll fix this tomorrow morning. Sorry for the inconveniences.

@PhilippMatthes PhilippMatthes dismissed martinmo’s stale review March 22, 2019 08:16

Suggestions were taken into account.

@martinmo martinmo merged commit a30e2fe into master Apr 4, 2019
martinmo added a commit that referenced this pull request Apr 4, 2019
@martinmo martinmo deleted the 205-file-upload-pattern branch April 4, 2019 11:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants