openownership / lib-cove-bods

Check that your data complies with the Beneficial Ownership Data Standard (BODS) using our install our data review library to analyse files via your command line interface
https://datareview.openownership.org/
Other
1 stars 0 forks source link

Additional check: Person Statement politicalExposure/status must exist and be isPep/unknown in certain conditions #87

Closed kd-ods closed 1 month ago

kd-ods commented 2 years ago

Check: If politicalExposure/details array is non-empty then politicalExposure/status should not be isNotPep.

Error message: "politicalExposure.status does not exist or has the value 'isNotPep'. Information has been provided under politicalExposure.details so politicalExposure.status should be 'isPep' or 'unknown'."

odscjames commented 2 years ago

Check: If either politicalExposure/details array .....

The word "either" here is I think a mistake?

kd-ods commented 2 years ago

Yes - I'll correct that typo in the original post.

odscjames commented 2 years ago

This is for 0.3.

Do we need a version of this for 0.2 or can we just ignore? Something like: If politicalExposure/details array has any entries with no missing info then politicalExposure/status should be true

kd-ods commented 2 years ago

Do we need a version of this for 0.2....?

Yes. The json pointers and property names will need to be aligned to 0.2 tho.

odscjames commented 2 years ago

For 0.2, I meant :smile: : If pepStatusDetails array has any entries with no missing info (ie the missingInfoReason field ) then hasPepStatus should be true.

And we don't touch 0.1

odscjames commented 2 years ago

In doing this, I also realised we could have another check for 0.3+:

has_pep_details_without_missing_info_but_incorrect_pep_status - if there are any details without missingInfoReason then status should be "isPep"

?

(Make separate issue if so pls)

kd-ods commented 2 years ago

if there are any details without missingInfoReason then status should be "isPep"

I think, let's not go that far atm.

BUT, back to the original check for this issue: I've realised that there ws an implicit bit of the check that needs to be explicit....

Check: If politicalExposure/details array is non-empty then politicalExposure/status should exist and should not be isNotPep.

Blueskies00 commented 2 years ago

Tests https://docs.google.com/spreadsheets/d/1rrJNBXTWZNGEXlbS3wpbt8g7xE4JPrnoF6HG6NFq9Fs/edit#gid=1522424564

  1. PEP status = isPep AND completed PEP fields. Expected result: No errors. Actual result: No errors. Comments: Test: PASSED ACTIONS: None.

  2. politicalExposure/status = missing AND missingInfoReason completed. Expected result: Error - PEP status missing. Actual result: Validation error - "politicalExposure/status is missing but required. Check that the field is included and correctly spelled." Additional check - "This Person Statement has some PEP details but their PEP status has been declared as 'isNotPep'." Additional check - "This Person Statement has some PEP details with missing info but their status has not been declared as 'unknown'." Comments: The first additional check is incorrect in it's message - the PEP status has not been declared at all. There's also some duplication of messaging here, though arguably the validation error and the second check re missing info are complementary. Test: FAILED ACTIONS: @odscjames Can you review the first additional check and why it's appeared in this instance - is it just a wording thing? Also, is it possible to ensure that the two additional checks don't overlap in instances like this?

  3. politicalExposure/status = missing AND various pep details completed. Expected result: Error - PEP status missing. Actual result: Validation error - "politicalExposure/status is missing but required. Check that the field is included and correctly spelled." Additional check - "This Person Statement has some PEP details but their PEP status has been declared as 'isNotPep'." Comments: The first additional check is incorrect in it's message - the PEP status has not been declared at all. Test: FAILED ACTIONS: @odscjames Can we amend the wording for the additional check when encountering a missing value to something like...

    "This Person Statement has some PEP details but their PEP status has not been declared".

Alternatively, we could just have the Validation check for the missing status - @kd-ods what do you think?

  1. politicalExposure/status = isNotPep AND various pep details completed. Expected result: Error - PEP status must be isPep. Actual result: Additional check - "This Person Statement has some PEP details but their PEP status has been declared as 'isNotPep'. " Comments: Test: PASSED ACTIONS: None.

  2. politicalExposure/status = Unknown AND various pep details completed. Expected result: No errors. Actual result: No errors. Comments: This is the result expected based on the comments above considering whether or not to include this check and deciding not to for now. However, I've included it for posterity. Test: PASSED ACTIONS: @kd-ods Maybe just worth thinking about this one again for future revisions.

  3. PEP status = isPep AND all other pep fields empty. Expected result: No errors. Actual result: No errors. Comments: As expected as subsequent fields aren't required by the schema. Test: PASSED ACTIONS: None.

  4. politicalExposure/status = isPep AND missingInfoReason completed. Expected result: Error - wrong PEP status. Actual result: Additional check - "This Person Statement has some PEP details with missing info but their status has not been declared as 'unknown'." Comments: Test: PASSED ACTIONS: None.

  5. PEP status = TRUE (invalid entry) AND completed PEP fields. Expected result: Error - value isn't from codelist. Actual result: Validation error - "politicalExposure/status contains an unrecognised value. Check the related codelist for allowed code values." Comments: Test: PASSED ACTIONS: None.

kd-ods commented 2 years ago

On (3) - I agree that the additional check message could be better. How about just changing it to the following (rather than creating a special case):

"This Person Statement has some PEP details but their PEP status is missing or has been declared as 'isNotPep'."

odscjames commented 2 years ago

Ok, so 2 issues here:

1)

The check is correct but error message is wrong. Currently we have the same alert raised for isNotPep or None - a check with type: has_pep_details_but_incorrect_pep_status

So if we could edit the message to:

"This Person Statement has some PEP details but their PEP status is missing or has been declared as 'isNotPep'."

As suggested.

2)

The overlap - if no status set but details with missing info comes up, then we only want the:

has_pep_details_with_missing_info_but_incorrect_pep_status / This Person Statement has some PEP details with missing info but their status has not been declared as 'unknown'.

alert, and NOT also the:

has_pep_details_but_incorrect_pep_status / "This Person Statement has some PEP details but their PEP status is missing or has been declared as 'isNotPep'."

Both of these are fine as ok changes

kd-ods commented 2 years ago
  1. Yes, update the error message as suggested.
  2. Actually, I would suggest just using the second one: has_pep_details_but_incorrect_pep_status (with the updated message, as suggested)
odscjames commented 2 years ago

Call - if both come up for same data, we'll just show has_pep_details_with_missing_info_but_incorrect_pep_status / This Person Statement has some PEP details with missing info but their status has not been declared as 'unknown'. ( so not what @kd-ods said in last comment)

And @kd-ods will rewrite text for that error.

kd-ods commented 2 years ago

For has_pep_details_with_missing_info_but_incorrect_pep_status please change:

"This Person Statement has some PEP details with missing info but their status has not been declared as 'unknown'."

to:

"This Person Statement has a missingInfoReason for PEP status details, so PEP status should be declared as 'unknown'."

Blueskies00 commented 2 years ago

Reran the failed tests. The messages have been updated and duplication removed. All good!