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.

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

fix attachment type checking, allow excel and csv #2077

Merged
merged 7 commits into from Mar 24, 2020
Merged

Conversation

@opqdonut
Copy link
Collaborator

opqdonut commented Mar 24, 2020

fixes #2023

Definition of Done / Review checklist

Reviewability

  • link to issue
  • consider adding screenshots for ease of review

API

  • API is documented and shows up in Swagger UI
  • API is backwards compatible or completely new

Documentation

  • update changelog if necessary

Testing

  • complex logic is unit tested
  • valuable features are integration / browser / acceptance tested automatically

Accessibility

  • all icons have the aria-label attribute
  • conscious decision about where to move focus after an action

Follow-up

  • no critical TODOs left to implement
opqdonut added 5 commits Mar 24, 2020
Previously, the frontend had a whitelist of allowed extensions, while
the backend had a whitelist of allowed mime types. These two lists had
to be manually kept in sync, but didn't add any safety since we were
just checking the mime type reported by the client.

Now there is an explicit whitelist of allowed extensions that's shared
between backend and frontend.

Additionally, this fixes uploading excel attachments, and allows .csv
and .tsv attachments
@opqdonut opqdonut force-pushed the allow-excel-2023 branch from 7129d24 to db38525 Mar 24, 2020
@@ -331,6 +331,7 @@
:events "Events"
:failed "Failed"
:has-accepted-licenses "Terms of use accepted."
:invalid-attachment "Attachment not acceptable"

This comment has been minimized.

Copy link
@Macroz

Macroz Mar 24, 2020

Collaborator

Would be nice to list here the allowed types.

Also isn't it more like "Attachment type not acceptable"? Also there is now attachment being acceptable and allowed file extensions. How about unifying and using "Acceptable file types" and "Attachment type not acceptable".

@@ -271,7 +273,20 @@
{:id id
:type :button
:on-click (fn [e] (.click (.getElementById js/document upload-id)))}
(text :t.form/upload)]]))
(text :t.form/upload)]
[:a.application-search-tips.btn.btn-link.collapsed

This comment has been minimized.

Copy link
@Macroz

Macroz Mar 24, 2020

Collaborator

Doesn't seem like the content is focused after opening. Comparing to the search info.

opqdonut added 2 commits Mar 24, 2020
- more consistent terminology
- mention allowed file types
also, fix copypasted classes
@opqdonut opqdonut mentioned this pull request Mar 24, 2020
4 of 9 tasks complete
@Macroz
Macroz approved these changes Mar 24, 2020
@Macroz Macroz merged commit 78f6a8d into master Mar 24, 2020
6 checks passed
6 checks passed
WIP Ready for review
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: doo Your tests passed on CircleCI!
Details
ci/circleci: ok Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: without-db Your tests passed on CircleCI!
Details
@Macroz Macroz deleted the allow-excel-2023 branch Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.