Closed sahil-ondc closed 7 months ago
@sharmasahil0331 All comments have been addressed except for the common payment fields, since reference buyer app doesn't send these fields. If buyer app sends them then it's included in our response.
/message/order/payment must have required property '@ondc/org/settlement_basis'
/message/order/payment must have required property '@ondc/org/settlement_window'
/message/order/payment must have required property '@ondc/org/withholding_amount'
Please check https://github.com/ONDC-Official/v1.2.0-logs/pull/115
search_inc_refresh
on_search_full_catalog_refresh
@BasanthVerma
search_inc_refresh
Pasted the wrong JSON log will add the right JSON
on_search_full_catalog_refresh
@sharmasahil0331
Will update search_inc_refresh on confirmation.
@sharmasahil0331 @bluecypher
search_inc_refresh feedback was corrected in #177
@BasanthVerma Reported issues are still not fixed.
@sharmasahil0331 @bluecypher, we've submitted Group B with all 5 flows in #280
on_search_full_catalog_refresh
on_search_inc_catalog_refresh
@BasanthVerma
https://docs.google.com/document/d/1brvcltG_DagZ3kGr1ZZQk4hG4tze3zvcxmGV4NMTzr8/edit
https://docs.google.com/document/d/1brvcltG_DagZ3kGr1ZZQk4hG4tze3zvcxmGV4NMTzr8/edit
message/order/tags should have property quote_trail as per the API contract
https://docs.google.com/document/d/1brvcltG_DagZ3kGr1ZZQk4hG4tze3zvcxmGV4NMTzr8/edit
@BasanthVerma
Hi @sahil-ondc @bluecypher , we've resubmitted Group B 1-5 flows and also added flow 6.
Please review: https://github.com/ONDC-Official/v1.2.0-logs/pull/606
@BasanthVerma
on_status
/message/order/fulfillments/0/end/instruction - Please check the fulfillments/0/end/instruction/code in our log. We are using code 3, and according to the contract foot note 1145, short_desc is required for code 1 & 2 only .
on_search_full_catalog_refresh & on_search_inc_catalog_refresh - We will fix them
Please check our comment regarding on_status, and is it good if we submit only updated Flow 1 logs again? @sahil-ondc @bluecypher
@BasanthVerma,
Yes, both are fine.
@bluecypher Updated flow 1 logs, please check.
@BasanthVerma
@sahil-ondc
I don't see our final commit here which shows the actual submission. 95c5386 and 69c0738 are our internal commits on the working branch of our fork. There can be n number of commits here. The final commit involves a merge from the working branch to our fork's master (which is missing in your screenshot). Then we make the actual submission by sending a PR from our fork's master to this repo's master.
648df20 is the submission commit and no commits/changes were made after this.
Please check the full history.
@bluecypher
@BasanthVerma
@sahil-ondc @bluecypher resubmitted flow 1 new logs as per the above feedback here https://github.com/ONDC-Official/v1.2.0-logs/pull/768
Regarding the last point in on_search_full - I understand that some of products had "na" under nutritional info, additives info etc so they appear as placeholder values but they're not. They were manually entered by one of our team members when creating those test products. We don't have placeholder values for any of such fields. However, although contract doesn't specify min length for such strings, we will be enforcing a min length of maybe 64 characters.
/on_search
/on_search (inc)
/on_select
/on_init
/on_confirm
/on_status
/on_select (OOS)
/on_cancel
/on_update
/on_status
@BasanthVerma
placeholder images of SKUs should not be provided in the catalog; ensure that all images are loading properly, as some are not currently loading
: These image links are from CAAS database and the CAAS image bucket appears to be down. Some images still might not load in our new submission, we are planning to remove all external image URLs and use our CDN in the coming weeks (before going to prod).
are all the scenarios of incremental pull supported?
: All scenarios except offers increment pull is supported. We've included provider and location incremental update cases in the new submission.
@bluecypher resubmitted all 6 flows with fixes.
@BasanthVerma
on_search_full_catalog_refresh
Looks like very old test data from our catalog, we'll remove them.
we'll remove them.
init
address.building should be more than 3 chars - this is from buyer app
on_confirm
/message/order/tags/bpp_terms should have 'provider_tax_number' - We're MSN, in contract its says mandatory for ISN. Please confirm if it's still made necessary by buyer apps, we can add it.
on_status -Couldn't find any of these issues in the most of the on_status that I just checked. Please share the file path where these issues were found.
@sahil-ondc @bluecypher Please check and confirm if submitting only new on_search_full_catalog_refresh log will suffice
provider_tax_number is mandatory for all the SNPs
@BasanthVerma
@bluecypher can you check my other comments too?
/on_select:
/on_confirm:
@BasanthVerma - pls resubmit for flows 1 & 5 (expect above issues for flow 2 to be verifiable in flow 5);
@BLR-0118 please review our PR #949 , this includes the updates as per the feedback:
@BasanthVerma
on_search_full_catalog - fixing on_search_inc_catalog - this appears to be correct. message_id's are unique in the submitted logs for catalog. But in store and provider it's not unique, fixing it.
@sahil-ondc
@BLR-0118 @bluecypher @sahil-ondc we've resubmitted flow 1 with feedback fixes
/on_search
/on_search (inc)
/on_init
/on_status (OFD)
/on_cancel
/on_status (RTO-Delivered)
@BasanthVerma
on_search
range can be utilised instead of frequency and times in case of fixed timings - The logs already appear to have range, along with frequency & times. Are you suggesting we only use range and remove frequency & times?
description of variant grouping should not point to a particular variant; eg: 200g is mentioned in the description of variant_group "vg2" - Contract example has "Variant Group 1". Is this something that is displayed to the end user on buyer apps? What is the recommended value to be used for the description? For ex: Surf excel 100 gm and 200 gm variants - can the description be "Quantity Measure based variant" or should it say "Surf excel" alone?
on_cancel
In our case it's off-network self-fulfilled logistics where the merchant would be making the delivery themselves. Usually the merchant will try to contact the buyer before going out for delivery to check their availability. In practice they might reach out more than 2-3 times before canceling (additionally our ops will also attempt to contact in case the merchant couldn't reach the buyer). So should we leave this to 2 or set it to 3?
@sandeepshahi @BLR-0118
@BasanthVerma
/message/catalog/bpp/descriptor/tags/bpp_terms should not have property ‘collect_payment’ as part of phase 1
Removing this
timming should be provided in provider level tags as per the API contract
Adding this as per the full contract
Categories had some issues and are removed in the current logs, are those issues fixed (on_search)
No categories are not removed. They’re fixed and are they’re part of the log that is submitted. (Provider ID nynGrPkGHzKUiR64S3Mw)
There are no additional charges. quote_trail and quote values are adding up in the log submitted. Original Order delivery fee and the convenience fee with respect to the delivery fee is left in quote, rest are refunded in quote_trail. What are we missing here?
@sahil-ondc
@sahil-ondc it's the convenience fee w.r.t delivery fee that is remaining in quote, more details below:
Convenience fee / misc = (SUM of items + Delivery fee ) 3% convenience fee 18% GST
misc in on_confirm = (75+228+44.65) 0.03 1.18 = 12.31 as shown in quote
revised misc for RTO on_cancel = ( 0+0+44.65) 0.03 1.18 = 1.58 as shown in quote (Rest of the misc 10.73 with respect to items is is refunded in quote_trail)
ACK
@sahil-ondc fixed and resubmitted latest logs for on_search full catalog
/on_status APIs:
/on_cancel:
@BasanthVerma - request to pls resubmit for flow 1 & 5
@BasanthVerma - removed the comment on rto states, it's available in /on_cancel & /on_status
@BLR-0118 Fixed and resubmitted flow 1 and 5. Please review #1282
@BasanthVerma
on_select: If you are referring to the price of item 78dc6dba-f96f-4e0f-a832-def925fa41b6, it exists in incremental item on_search
on_init: Looks like new addition in on_init based on the comment in the contract, we'll add it and resubmit flow 5.
@sahil-ondc
ACK.
full_catalog_on_search:
incremental search:
@BasanthVerma - clearing logs for v1.2.0 (RET10), subject to the above being fixed;
@BasanthVerma - logs cleared for RET13;
on_search_full_catalog_refresh
search_inc_refresh
on_search_inc_refresh
on_init
on_confirm
on_status_delivered
common
@BasanthVerma