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

feat!: Add WITHDRAWN state to Requisition #212

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

SanjayVas
Copy link
Member

@SanjayVas SanjayVas commented Aug 9, 2024

This change should generally be safe for DataProvider integrators:

  • The common ListRequisitions request includes a filter for Requisition.State.UNFULFILLED
  • Barring the above, any code that deals with protobuf enums is expected to gracefully handle unrecognized values.
    • For example, Java generated code includes an UNRECOGNIZED value for every protobuf enum.

@wfa-reviewable
Copy link

This change is Reviewable

@SanjayVas SanjayVas changed the title Add WITHDRAWN state to Requisition. feat: Add WITHDRAWN state to Requisition Aug 9, 2024
@SanjayVas SanjayVas changed the title feat: Add WITHDRAWN state to Requisition feat!: Add WITHDRAWN state to Requisition Aug 9, 2024
Copy link
Contributor

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/requisition.proto line 187 at r1 (raw file):

    // `measurement` will be in the `FAILED` state.
    REFUSED = 3;
    // The `Requisition` has been withdrawn. Terminal state.

should this be

The `Requisition` has been withdrawn by a party other than the `DataProvider`. Terminal State.

Also, this will mean that the measurement will be in the FAILED state, correct?

Copy link
Contributor

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/requisition.proto line 187 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

should this be

The `Requisition` has been withdrawn by a party other than the `DataProvider`. Terminal State.

Also, this will mean that the measurement will be in the FAILED state, correct?

my wording makes an assumption that the operator may want the ability to withdraw. If we don't want to cover that use case for now, we could just comment that this means it was withdrawn by the MC.

Copy link
Member Author

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/requisition.proto line 187 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

my wording makes an assumption that the operator may want the ability to withdraw. If we don't want to cover that use case for now, we could just comment that this means it was withdrawn by the MC.

The existing wording is correct, IMO. The DataProvider cannot withdraw a Requisition themselves, as is apparent by the lack of a WithdrawRequisition method. They can instead refuse.

This intentionally does not enumerate all the conditions that can result in a Requisition being withdrawn. At the moment, this can happen any time the Measurement transitions to FAILED or CANCELLED. This is not limited to MC cancellation, and can include a different EDP refusing their Requisition, a Duchy indicating a failure, etc.

Copy link
Contributor

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)

Copy link
Member Author

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Dismissed @stevenwarejones from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)

@SanjayVas SanjayVas merged commit c35ccb6 into main Aug 14, 2024
13 checks passed
@SanjayVas SanjayVas deleted the sanjayvas-requisition-withdrawn branch August 14, 2024 16:41
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.

3 participants