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

Rule "studyid_not_in_content" should gracefully pass when unable to read incoming file #103

Closed
2 of 3 tasks
jujaga opened this issue Mar 7, 2019 · 5 comments
Closed
2 of 3 tasks
Assignees
Labels
bug Something isn't working verification Verify the bug has been fixed
Milestone

Comments

@jujaga
Copy link
Member

jujaga commented Mar 7, 2019

OCWA Issue

User Story

As a researcher,

I want the "studyid_not_in_content" rule to not fail on files which do obviously do not have study ids

so that I can submit files such as images and other binary content.

Test Case

ENV

  • DEV
  • TEST
  • PROD

TESTCASE

  • Add any image file

EXPECTED

  • The studyid_not_in_content rule should pass

ACTUAL

  • The studyid_not_in_content rule fails

Notes

The rule needs to be rewritten to gracefully fail upon failure to decode instead of hard failing.

@jujaga jujaga added the bug Something isn't working label Mar 7, 2019
@jujaga jujaga added this to the Sprint 51 milestone Mar 7, 2019
@jujaga jujaga self-assigned this Mar 7, 2019
jujaga added a commit that referenced this issue Mar 7, 2019
Change validation rule names #94

Signed-off-by: Jeremy Ho <jujaga@gmail.com>
@jujaga
Copy link
Member Author

jujaga commented Mar 7, 2019

Rule was updated to print(not bool(re.compile(b'[\\w]{1}[\\d]{9}').search(${file.content}))) which handles binary content better and satisfies this issue.

@jujaga jujaga mentioned this issue Mar 8, 2019
8 tasks
@pripley123
Copy link
Collaborator

tried creating a new request in the test environment and uploaded a jpg. Saved and closed. Viewed the request. Still flagged for study ids

@pripley123
Copy link
Collaborator

I've tried a couple of more jpg - these are passing ... so I need to narrow this down a bit

@jujaga
Copy link
Member Author

jujaga commented Mar 11, 2019

Certain image files have an explicit string inside the binary which will trip the [\w]{1}[\d]{9} regex rule as it only looks for either

  1. a letter followed by exactly 9 digits or
  2. exactly 10 digits

The implemented rule is behaving as expected; however until we are provided better requirements that do not match on so many potential values, some images are inevitably going to fail.

@pripley123
Copy link
Collaborator

Thanks for documenting the reason for images failing. Agreed it is expected behaviour for now. Since the regex used to search for study ids is configurable, this seems fine for MVP

@pripley123 pripley123 added the verification Verify the bug has been fixed label Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working verification Verify the bug has been fixed
Projects
None yet
Development

No branches or pull requests

3 participants