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 of bug I introduced that skipped validation error reporting #175

Merged
merged 3 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,14 @@ Submit4DN
Change Log
----------

4.0.2
=====

`PR 175: error reporting bug fix <https://github.com/4dn-dcic/Submit4DN/pull/175>`_

* bug fix of bug that did not properly report validation errors
* added a test for case where an alias is passed to error_report function

4.0.1
=====

Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[tool.poetry]
name = "Submit4DN"
version = "4.0.1"
version = "4.0.2"
description = "Utility package for submitting data to the 4DN Data Portal"
authors = ["4DN-DCIC Team <[email protected]>"]
license = "MIT"
Expand Down
57 changes: 48 additions & 9 deletions tests/test_import_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -280,14 +280,15 @@ def test_combine_set_expsets_with_existing():


def test_error_report(connection_mock):
# There are x errors, x of them are legit, need to be checked against the all aliases list, and excluded
# There are 6 errors in the err_dict, 5 of them are legit, 1 is checked against the all aliases list, and excluded
err_dict = {
"title": "Unprocessable Entity",
"status": "error",
"errors": [
{"location": "body",
"description": "Test for error with no name"},
{"name": "Schema: ", "location": "body",
{# This one should be excluded from report as this alias is in the sheet alias list
"name": "Schema: ", "location": "body",
"description": "Unable to resolve link: siyuan-wang-lab:region_1MB_TAD_1"},
{"name": "Schema: ", "location": "body",
"description": "Unable to resolve link: siyuan-wang-lab:region_5MB_TAD_2"},
Expand All @@ -307,14 +308,52 @@ def test_error_report(connection_mock):
rep = imp.error_report(err_dict, "Vendor", ['dcic:insituhicagar', 'siyuan-wang-lab:region_1MB_TAD_1'], connection_mock)
message = '''
ERROR vendor Test for error with no name
ERROR vendor Field 'Schema: ': Unable to resolve link: siyuan-wang-lab:region_5MB_TAD_2
ERROR vendor Field 'Schema: genome_location.1': 'siyuan-wang-lab:region_5MB_TAD_2' not found
ERROR vendor Field 'age': 'at' is not of type 'number'
ERROR vendor Field 'sex': 'green' is not one of ['male', 'female', 'unknown', 'mixed']
ERROR vendor Field 'Schema: ': Unable to resolve link: siyuan-wang-lab:region_5MB_TAD_2
ERROR vendor Field 'Schema: genome_location.1': 'siyuan-wang-lab:region_5MB_TAD_2' not found
ERROR vendor Field 'age': 'at' is not of type 'number'
ERROR vendor Field 'sex': 'green' is not one of ['male', 'female', 'unknown', 'mixed']
'''
assert rep.strip() == message.strip()


def test_error_with_alias_param_report(connection_mock):
# There are 6 errors in the err_dict, 5 of them are legit, 1 is checked against the all aliases list, and excluded
err_dict = {
"title": "Unprocessable Entity",
"status": "error",
"errors": [
{"location": "body",
"description": "Test for error with no name"},
{# This one should be excluded from report as this alias is in the sheet alias list
"name": "Schema: ", "location": "body",
"description": "Unable to resolve link: siyuan-wang-lab:region_1MB_TAD_1"},
{"name": "Schema: ", "location": "body",
"description": "Unable to resolve link: siyuan-wang-lab:region_5MB_TAD_2"},
{"location": "body", "name": "Schema: genome_location.1",
"description": "'siyuan-wang-lab:region_5MB_TAD_2' not found"},
{"name": "protocol_documents",
"description": "'dcic:insituhicagar' not found", "location": "body"},
{"name": "age",
"description": "'at' is not of type 'number'", "location": "body"},
{"name": "sex",
"description": "'green' is not one of ['male', 'female', 'unknown', 'mixed']", "location": "body"}
],
"code": 422,
"@type": ["ValidationFailure", "Error"],
"description": "Failed validation"
}
rep = imp.error_report(err_dict, "Vendor", ['dcic:insituhicagar', 'siyuan-wang-lab:region_1MB_TAD_1'], connection_mock, error_id='dcic:insituhicagar')
message = '''
ERROR vendor Test for error with no name
ERROR vendor dcic:insituhicagar Field 'Schema: ': Unable to resolve link: siyuan-wang-lab:region_5MB_TAD_2
ERROR vendor dcic:insituhicagar Field 'Schema: genome_location.1': 'siyuan-wang-lab:region_5MB_TAD_2' not found
ERROR vendor dcic:insituhicagar Field 'age': 'at' is not of type 'number'
ERROR vendor dcic:insituhicagar Field 'sex': 'green' is not one of ['male', 'female', 'unknown', 'mixed']
'''
assert rep.strip() == message.strip()



def test_error_conflict_report(connection_mock):
# There is one conflict error
err_dict = {"title": "Conflict",
Expand Down Expand Up @@ -485,7 +524,7 @@ def test_workbook_reader_update_new_file_fastq_post_and_file_upload(capsys, mock
all_aliases, dict_load, dict_rep, dict_set, True, [])
args = imp.ff_utils.post_metadata.call_args
out = capsys.readouterr()[0]
outlist = [i.strip() for i in out.split('\n') if i is not ""]
outlist = [i.strip() for i in out.split('\n') if i != ""]
post_json_arg = args[0][0]
assert post_json_arg['md5sum'] == '8f8cc612e5b2d25c52b1d29017e38f2b'
assert message0 == outlist[0]
Expand Down Expand Up @@ -533,7 +572,7 @@ def test_workbook_reader_patch_file_meta_and_file_upload(capsys, mocker, connect
assert updated_post['@graph'][0]['upload_credentials'] == 'new_creds'
# check for output message
out = capsys.readouterr()[0]
outlist = [i.strip() for i in out.split('\n') if i is not ""]
outlist = [i.strip() for i in out.split('\n') if i != ""]
assert message0 == outlist[0]
assert message1 == outlist[1]

Expand Down Expand Up @@ -681,7 +720,7 @@ def test_order_sorter(capsys):
message1 = '''WARNING! Check the sheet names and the reference list "sheet_order"'''
assert ordered_list == imp.order_sorter(test_list)
out = capsys.readouterr()[0]
outlist = [i.strip() for i in out.split('\n') if i is not ""]
outlist = [i.strip() for i in out.split('\n') if i != ""]
import sys
if (sys.version_info > (3, 0)):
assert message0 in outlist[0]
Expand Down
8 changes: 3 additions & 5 deletions wranglertools/import_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -793,9 +793,7 @@ def error_report(error_dic, sheet, all_aliases, connection, error_id=''):
nf_txt = 'not found'
not_found = None
alias_bit = None
if error_id:
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this was the bit that I saw was causing the issue - removing this should do the trick.

alias_bit = error_id
elif utrl_txt in error_description:
if utrl_txt in error_description:
alias_bit = error_description.replace(utrl_txt, '')
elif error_description.endswith(nf_txt):
alias_bit = error_description.replace(nf_txt, '').replace("'", '')
Expand All @@ -805,8 +803,8 @@ def error_report(error_dic, sheet, all_aliases, connection, error_id=''):
if not_found and not_found in all_aliases:
continue
error_field = err['name']
report.append("{sheet:<30}Field '{er}': {des}"
.format(er=error_field, des=error_description, sheet="ERROR " + sheet.lower()))
report.append("{sheet:<30}{eid} Field '{er}': {des}"
.format(er=error_field, des=error_description, eid=error_id, sheet="ERROR " + sheet.lower()))
# if there is a an access forbidden error
elif error_dic.get('title') == 'Forbidden':
error_description = error_dic['description']
Expand Down
Loading