open-contracting / ocdskit

A suite of command-line tools for working with OCDS data
https://ocdskit.readthedocs.io
BSD 3-Clause "New" or "Revised" License
17 stars 6 forks source link

Update to v1.1 #78

Closed yolile closed 5 years ago

yolile commented 5 years ago

https://github.com/open-contracting/kingfisher/issues/256

closes #77

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.4%) to 98.144% when pulling 12b6d161a1f07ae728a2e0867bf6b0861a4939c6 on update_to_v1.1 into fc56bfeec216b92ab3595f99d886054be8c5c0d0 on master.

jpmckinney commented 5 years ago

If you run flake8 locally, you won't have to wait for Travis :)

jpmckinney commented 5 years ago

I know the except Exception: pass blocks were present in the original scripts, but we shouldn't ignore all exceptions like that. There are also a variety of other code and style issues, repetitions, etc. from Tim's original version.

Can you:

  1. Fix the issues, so that the code is closer to the quality of our other code
  2. Change it so that it takes packages from standard input, instead of reading a directory
  3. Put test fixtures as files in tests/fixtures instead of using very long strings
  4. Rename the command to upgrade, and have it take a required argument which should be two OCDS versions as a colon-separated list, e.g. 1.0:1.1. In future, we'd add support for upgrading between other versions.
yolile commented 5 years ago

@jpmckinney I made the requested changes, but the tests are failing due to a pytest version issue

jpmckinney commented 5 years ago

Thanks! I fixed the build. I'll review PR shortly.

jpmckinney commented 5 years ago

@duncandewhurst Do you have an idea why the old code considered two organizations whose only difference was a contact point to be different organizations? I don't see why that should be the case.


Note: Versioned releases within a record package are not upgraded.

Note: The upgrade can lead to information loss if the same organization has a different address, contactPoint, or additionalIdentifiers across occurrences. The code warns in such cases.


Moved upgrade functions to new file, and:

duncandewhurst commented 5 years ago

Do you have an idea why the old code considered two organizations whose only difference was a contact point to be different organizations? I don't see why that should be the case.

My guess is that this is due to the guidance in organization section of the release reference, which states:

If a contracting process represents a contract arranged by the department or branch of a larger organization, the legal entity (usually the registered organization) should be described in the identifier section, with details of the branch or department given in the name, address and contact point as relevant.

Presumably to address the case where the procuring entity and the buyer are different departments of the same legal entity.

The guidance on constructing the party id is in line with this, though not reflected in the code (since there isn't a department id field in OCDS):

The ID used for cross-referencing to this party from other sections of the release. This field may be built with the following structure {identifier.scheme}-{identifier.id}(-{department-identifier}).

jpmckinney commented 5 years ago

Thanks, @duncandewhurst ! I've changed the code to also use address and contactPoint to identify the organization, if present.

@yolile Please review and merge if you see no issues.