Skip to content

changes to line 430 to accept number and string#18

Open
shikharjaiswal67 wants to merge 7 commits into
singer-io:masterfrom
shikharjaiswal67:tap-bc-orders
Open

changes to line 430 to accept number and string#18
shikharjaiswal67 wants to merge 7 commits into
singer-io:masterfrom
shikharjaiswal67:tap-bc-orders

Conversation

@shikharjaiswal67

Copy link
Copy Markdown

Description of change

product.product_options.value is defined as a string in the BigCommerce v2 API documentation. This request is related to issue #17 where value's in the BigCommerce source are strings and the insert fails for these orders.

The BigCommerce API docs which indicate the product_options.value is a string [(https://developer.bigcommerce.com/api-reference/3b4dfef625708-list-order-products)]

Screenshot for BigCommerce API doc
https://drive.google.com/file/d/1G4Y9JX7bcdVsbml-MzB8hHrUEx_NpZPL/view?usp=sharing

Manual QA steps

Verify string and integer product_options.value are being passed through the Tap

Risks

Rollback steps

  • revert this branch

@singer-bot

Copy link
Copy Markdown

Hi @shikharjaiswal67, thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes.

@singer-bot

Copy link
Copy Markdown

You did it @shikharjaiswal67!

Thank you for signing the Singer Contribution License Agreement.

@shikharjaiswal67 shikharjaiswal67 changed the title changes to line 430 to accept int and string changes to line 430 to accept number and string May 12, 2022
@dgrass414

Copy link
Copy Markdown

I am seeing this same issue with Stitch, how would I go about getting this fix utilized in our integration?

@shikharjaiswal67

shikharjaiswal67 commented Dec 14, 2022

Copy link
Copy Markdown
Author

I am seeing this same issue with Stitch, how would I go about getting this fix utilized in our integration?

probably send a message in their slack channel or to the repo owner directly.

@shikharjaiswal67

Copy link
Copy Markdown
Author

@luandy64
can you please merge this asap. we updated our bigcommerece integration in stitch and it reverted the changes on our integration. We are again getting errors in stitch logs and data is missing.

Comment thread tap_bigcommerce/schemas/orders.json Outdated
},
"value": {
"$ref": "type-number.json"
"type": ["number", "string", "null"]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @shikharjaiswal67,

I was wondering about the necessity of maintaining two separate datatypes. The documentation specifically mentions the datatype as string. Should we consider eliminating the number datatype altogether?

Having both datatypes may lead to the creation of two columns in the destination table, each with a different datatype (number and string). What are your thoughts on this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

hi @sgandhi1311 ,

Agreed with you as there is no need for both datatypes. I will create update the PR and let you know.

@sgandhi1311

Copy link
Copy Markdown
Member

I can merge it into the master branch; however, I've included a comment that needs attention before finalizing the merge.

@shikharjaiswal67

shikharjaiswal67 commented Jan 2, 2024

Copy link
Copy Markdown
Author

@sgandhi1311 - i have made the necessary changes. can you please review and approve it. Let me know for any issues. thank you.

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.

4 participants