Skip to content
This repository has been archived by the owner on Jan 27, 2022. It is now read-only.

Bug fix to address work order receipt flow in work order sync mode execution #440

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Ram-srini
Copy link
Contributor

@Ram-srini Ram-srini commented May 5, 2020

To fix issue - #412

Signed-off-by: Ramakrishna Srinivasamurthy [email protected]

@manojgop
Copy link
Contributor

manojgop commented May 6, 2020

@Ram-srini Could you update the commit message with details of the bug in work order receipt flow in sync mode.

if receipt_entry is None:
receipt_entry = {
"params": {
"receiptUpdates": []
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any param like "receiptUpdates" in the spec ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. This is implementation specific, need to store the receipt updates in that tag.

@@ -82,12 +82,22 @@ def WorkOrderReceiptCreate(self, **params):
)
else:
wo_receipt = self.kv_helper.get("wo-receipts", wo_id)
if wo_receipt is None:
wo_receipt = json.loads(wo_receipt)
if wo_receipt and "workOrderId" not in wo_receipt["params"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Under what scenario will workOrderId be not part of wo_receipt ? My understanding is receipt is for a specific work order. So a receipt does not exists without a work order

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WorkOrderId is exists if work order receipt is created. Refer the git issues for description #412

Copy link
Contributor

@rranjan3 rranjan3 left a comment

Choose a reason for hiding this comment

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

Please add some link to defect filed or some description otherwise to keep record of what was breaking and how it was fxed in the commit message. Also try compressing the commit title.

updated_receipt))
logger.info("Receipt for the workorder id %s is updated to %s",
wo_id, wo_receipt)
# If receipt is not created yet, add tag "receiptUpdates" to
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor typo in function comment @292 wo_json_ >>> json response
@298 - I do not follow the description there

self.private_key
)

# If it is first update to receipt
Copy link
Contributor

Choose a reason for hiding this comment

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

it is first update >>> is not the first update

"Receipt for the workorder id %s is completed " +
"and no further updates are allowed",
wo_id)
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Can there be contradictory receipt updates coupled with COMPLETED ? Most probably no. Otherwise we might still want to check whats the latest one and not return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

always check the latest status of receipt, if it is COMPLETED then there won't be further updates to it.

Address the git issue - hyperledger-archives#412

Signed-off-by: Ramakrishna Srinivasamurthy <[email protected]>
Since listener config is hard-coded with container service name
and it is failing in standlone mode.
Added command line option to listener to read zmq_url and defact value
in config file is localhost and command line argument takes precedence.

Signed-off-by: Ramakrishna Srinivasamurthy <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants