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

Example drt main #735

Merged
merged 9 commits into from
Dec 20, 2024
Merged

Example drt main #735

merged 9 commits into from
Dec 20, 2024

Conversation

kathryn-ods
Copy link
Contributor

@kathryn-ods kathryn-ods commented Nov 21, 2024

Overview

What does this pull request do?
Makes minor fixes to the examples files, identified by the DRT

How can a reviewer test or examine your changes?
Run a few through the DRT to check they're fine

Who is best placed to review it?

Closes #738

Translations

  • New or edited strings appearing as a result of this PR will be picked up for translation
  • I've notified the translation coordinator of any new strings that will need
    translating. See the Handbook

Documentation & Release

swapping 2 letter iso country for 3 digit

swapping 2 letter iso country for 3 digit

fixing shares and componentRecords

fixing midindent

removing duplicate statement Id

fixing midindent

fixing shares

fixing component records
@kathryn-ods kathryn-ods requested a review from kd-ods November 21, 2024 13:44
@kathryn-ods
Copy link
Contributor Author

@kd-ods in addition to the fixes here all examples had the error

beneficialOwnershipOrControl expected to be 'true' in at least one Relationship statement with a person as an interested party. If this dataset contains beneficial owners, check that beneficialOwnershipOrControl is correctly used.

Do we need to fix this?

@kathryn-ods
Copy link
Contributor Author

I'm not sure why the docs linkcheck keeps breaking - I haven't made any docs changes so it shouldn't be. I think there's some sort of issue with the way it interacts with the ISO site.

@kd-ods
Copy link
Collaborator

kd-ods commented Nov 22, 2024

@kd-ods in addition to the fixes here all examples had the error

beneficialOwnershipOrControl expected to be 'true' in at least one Relationship statement with a person as an interested party. If this dataset contains beneficial owners, check that beneficialOwnershipOrControl is correctly used.

Do we need to fix this?

I don't think we need to change the example files.

(It's a bit of an anomalous type of error message... since it's not an error, as such. It's a prompt for users of the tool to check that they are correctly using that field IF their dataset contains beneficial owners. There are definitely improvements to the user interface of the Data Review Tool which could make that clearer.)

entity didn't have hasPublicListing true
@kd-ods
Copy link
Collaborator

kd-ods commented Nov 28, 2024

The commit above fixes an issue spotted here.

@kathryn-ods
Copy link
Contributor Author

@kd-ods I think this still needs merging in? The link check is breaking for some reason other than the docs itself

@kd-ods
Copy link
Collaborator

kd-ods commented Dec 19, 2024

It looks like the particular link that is failing the check is https://www.iso20022.org/market-identifier-codes - it's apparently timing out. However the link does not seem to be broken if checked by hand.

I suspect that bots checking links are not welcomed in iso-land.

Do you think we should wait until the data review tool is released, do a final run of the data through it, before merging @kathryn-ods ?

@kathryn-ods
Copy link
Contributor Author

It looks like the particular link that is failing the check is https://www.iso20022.org/market-identifier-codes - it's apparently timing out. However the link does not seem to be broken if checked by hand.

I asked @rhiaro to look at this and they said

looks like there's something wrong with the ssl cert handshake and it times out. Not sure how to fix it off the top of my head. I think it's an issue with openssl on ubuntu .. I haven't looked into exactly what is running the tests on github actions, but there's probably a version upgrade somewhere in there that would resolve it

Do you think we should wait until the data review tool is released, do a final run of the data through it, before merging

I don't have strong feelings either way - what do you think?

Copy link
Collaborator

@kd-ods kd-ods left a comment

Choose a reason for hiding this comment

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

A couple of the files have discrepancies in the statementDates which need to be fixed.

examples/multiple-indirect-ownership.json Show resolved Hide resolved
examples/multiple-tax-residencies.json Show resolved Hide resolved
making statementDate consistent
making dates consistent
@kd-ods kd-ods self-requested a review December 20, 2024 10:47
@kathryn-ods kathryn-ods merged commit 42a17bc into main Dec 20, 2024
4 checks passed
@kathryn-ods kathryn-ods deleted the example_drt_main branch December 20, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] - Example files need to be corrected on main and 0.4 release
2 participants