-
Notifications
You must be signed in to change notification settings - Fork 12
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
834 additional iati checks #856
Conversation
…penag_tag.feature
a0eb69c
to
fbb540a
Compare
1ca23c2
to
0e4e129
Compare
@@ -11,7 +11,7 @@ | |||
from cove.lib.exceptions import CoveInputDataError, UnrecognisedFileTypeXML | |||
|
|||
|
|||
def common_checks_context_iati(context, upload_dir, data_file, file_type, api=False): | |||
def common_checks_context_iati(context, upload_dir, data_file, file_type, api=False, openag=False, orgids=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think orgids should probably be on by default - I think we want to see it in the web interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we'd like it in the web interface, but I don't want the additional overhead (UI design, etc) to slow down anything else in this sprint. So, if it's easy/cheap/free to add to the web UI, amazing, but if there's any real work involved, let's leave it and prioritise it against other work in non-sprint or our next IATI sprint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't be too difficult to add to the web interface, but I'd rather get this merged and leave that for another sprint or for the non-sprint board. If we agree on that, then I think it should be off by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, makes sense.
Given an IATI activity | ||
Then at least one `location` element is expected | ||
|
||
Scenario Outline: location.location-id must be present |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not location/location-id
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, the scenario outline will be the name of a generated file (with /
giving problems in Unix)
for xpath in get_xobjects(context.xml, context.xpath_expression): | ||
attr_id = xpath.attrib.get(attribute, '') | ||
for prefix in ORGIDS_PREFIXES: | ||
if re.match('^%s' % prefix, attr_id): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since prefixes aren't regexes, would attr_id.startswith(prefix)
be clearer here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I just forgot that startswith
exists
|
||
@then('every `{xpath_expression1}` must include `{xpath_expression2}` element') | ||
@register_ruleset_errors(['openag']) | ||
def step_openag_location_id_expected(context, xpath_expression1, xpath_expression2): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this function name be more generic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
4122967
to
d6a2a45
Compare
d6a2a45
to
815fc80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed that this branch leaves the web interface failing for a lot of files, e.g. https://raw.githubusercontent.com/IATI/IATI-Extra-Documentation/version-2.02/en/activity-standard/activity-standard-example-annotated.xml
Looks to be a JSON decode error associated with rulesets. I'd be happy to have a go fixing this tomorrow.
@Bjwebb that would be great. Some context. The problem is that file that it tries to read does not translate to JSON, it contains the following: _Argument 'element' has incorrect type (expected lxml.etree._Element, got lxml.etree.ElementUnicodeResult) This is the rule it tries to apply (in https://github.com/OpenDataServices/cove/blob/834-additional-iati-checks/cove_iati/rulesets/iati_standard_v2_ruleset/mandatory_elements.feature):
And processed by this step function https://github.com/OpenDataServices/cove/blob/834-additional-iati-checks/cove_iati/rulesets/iati_standard_v2_ruleset/steps/standard_ruleset_step_definitions.py#L86 Caveat: I have amended the latest commit on this branch, please discard your local branch if you have one already and re-pull the branch. |
@edugomez Since you're around tomorrow, it would be good if you could check my changes before we merge this. |
elif hasattr(child_xobj, 'is_text') and child_xobj.is_text: | ||
return '{}/text()'.format(tree.getpath(child_xobj.getparent())) | ||
else: | ||
return tree.getpath(child_xobj) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
#834