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

adding amountNet and amountGross to value, updating value definition #1519

Merged
merged 8 commits into from
Oct 17, 2023

Conversation

odscjen
Copy link
Contributor

@odscjen odscjen commented May 18, 2022

closes #817

@odscjen odscjen added this to the 1.2.0 milestone May 18, 2022
@odscjen odscjen requested a review from mrshll1001 May 18, 2022 09:34
Copy link
Contributor

@mrshll1001 mrshll1001 left a comment

Choose a reason for hiding this comment

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

One very minor change in the form of a typo, otherwise what I'd expect based on the issue,.

docs/history/changelog.md Outdated Show resolved Hide resolved
@odscjen odscjen requested a review from mrshll1001 May 18, 2022 10:18
Copy link
Contributor

@mrshll1001 mrshll1001 left a comment

Choose a reason for hiding this comment

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

This looks fine, typo amended.

@odscjen odscjen requested a review from jpmckinney May 18, 2022 13:05
Copy link
Member

@jpmckinney jpmckinney left a comment

Choose a reason for hiding this comment

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

We can omit "as a number" as this is repetitive with the type validation keyword.

I know "The amount as stated in the tender notice / award notice / contract notice document as a number" is the text I suggested in #817 (comment), but Value objects can appear in many different places, and the numbers might not originate in these specific documents in those cases. The number might also originate from an electronic system, such that there is no "document" per say.

Unfortunately, as an alternative, I don't think we can use turns of phrase like "original" / "unadjusted" / "unmodified" amount or similar, as we do allow the amount of Value objects to change over time, and it might just confuse readers as to the semantics.

Maybe something like "The amount as entered into a system or as published in a document."

This PR should also be supported by changes to the Markdown page for the Value object, to explain how the three fields ought to be used/interpreted (e.g. there are three fields so that, when it is unknown whether taxes are included or not, it can nonetheless be entered into amount – please be more eloquent than me :)).

The Markdown page should encourage publishers to describe their semantics for amount, e.g. a publish that has access to the amountNet and amountGross can have a policy of entering the amountNet in the amount. (We can perhaps also encourage this approach of using amount for amountNet when all are available, for consistency across datasets.)

@odscjen odscjen requested a review from jpmckinney August 1, 2022 10:07
docs/schema/reference.md Outdated Show resolved Hide resolved
odscjen and others added 2 commits August 3, 2022 09:51
@odscjen odscjen requested a review from jpmckinney August 3, 2022 08:54
@odscjen
Copy link
Contributor Author

odscjen commented Sep 11, 2023

@jpmckinney I think all the requested changes to this PR were made so just bumping back for your final review

@jpmckinney jpmckinney merged commit 2bbe21a into 1.2-dev Oct 17, 2023
10 checks passed
@jpmckinney jpmckinney deleted the 817-value-amount branch October 17, 2023 04:49
@jpmckinney
Copy link
Member

jpmckinney commented Oct 17, 2023

Not sure why I left this PR open so long. I think I had paused my work on OCDS 1.2 around the time the updates were made.

This new description of amount resolves some of the ambiguities that I was maybe unsure about (e.g. how to interpret amount depending on which other fields were available).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Update Value.amount description to suggest/recommend tax-free amounts
3 participants