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

Update OrderItem.php #5

Merged
merged 1 commit into from
Jan 8, 2020
Merged

Update OrderItem.php #5

merged 1 commit into from
Jan 8, 2020

Conversation

gulaandrij
Copy link

fixes the wrong setting weight of the ordered item

@gulaandrij
Copy link
Author

ping @zack6849

@zack6849
Copy link
Owner

zack6849 commented Jan 8, 2020

Could you explain why this is "wrong"?
I'm not the original author of this code, this is a fork, but I'm not sure I understand why this is better or why the original is wrong, there's not even a "weight" property on the OrderItem Object, so why would you set one?

If you're trying to bring this in line with the same functionality of the Order model, that'd make sense, but then we should be declaring all of those as properties of the Order Item, no?

The base model class looks (to me) like when you construct it with attributes, it assigns them to a property anyways

Is the issue that you're using order item but trying to update the weight, and the update isn't reflected in the magic property it declares, or something of that nature?

Thanks,

@zack6849 zack6849 added the needs clarification needs clarification on why it's necessary label Jan 8, 2020
@gulaandrij
Copy link
Author

the main problem that 'weight' is set to the attribute array and not send to API

before fix
image

after fix
image

@gulaandrij
Copy link
Author

also as you can see here https://www.shipstation.com/developer-api/#/reference/model-orderitem Order Item object has 'weight' attribute

@zack6849 zack6849 merged commit f0af609 into zack6849:master Jan 8, 2020
@gulaandrij gulaandrij deleted the patch-1 branch January 8, 2020 17:20
@zack6849
Copy link
Owner

zack6849 commented Jan 8, 2020

I also went ahead and refactored all the other attribute arrays as properties, could you confirm this is working as expected? If so, i'll tag a new release

Thanks @gulaandrij !

@gulaandrij
Copy link
Author

tnx, I'll check

@zack6849 zack6849 removed the needs clarification needs clarification on why it's necessary label Jan 9, 2020
@zack6849
Copy link
Owner

zack6849 commented Jan 9, 2020

@gulaandrij did you ever figure out if the code in master fixed your problems?

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.

2 participants