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

Why is product PublishedAt NullValueHandling as Include? #367

Closed
moxplod opened this issue May 29, 2019 · 11 comments
Closed

Why is product PublishedAt NullValueHandling as Include? #367

moxplod opened this issue May 29, 2019 · 11 comments
Labels
object updating Bugs and strange behavior around updating objects, primarly related to #284 question

Comments

@moxplod
Copy link

moxplod commented May 29, 2019

[JsonProperty("published_at", DefaultValueHandling = DefaultValueHandling.Include, NullValueHandling = NullValueHandling.Include)]

NullValueHandling = NullValueHandling.Include

Means it's being included in JSON blob as null even when all I am trying to do is update the product title or any other field.

Which unintentionally unpublishes the product on any product update.

Is there a reason for this as this is the only property in the entire library that has that attribute?

@nozzlegear
Copy link
Owner

Unfortunately this is one of those damned if you do, damned if you don't kind of things. The NullValueHandling is set to include because it would be impossible to unpublish a product at all without it. If we were to set the NullValueHandling to ignore, the package wouldn't serialize or send the null value and then you can't unpublish the product. I'm open to any suggestions on how to better handle this, but for the time being, being able to send null values for PublishedAt is intentional.

@dkershner6
Copy link
Contributor

I am wondering if we should also enforce returning that field on ProductService.Get/List, so that no one could pull a product, change some stuff, then send back the product and have it pause.

@dkershner6
Copy link
Contributor

I guess that wouldn't have helped this particular scenario though...no ideal solution unfortunately.

@clement911
Copy link
Collaborator

It is a tricky one.
The ListFilter has a fields property to include fields that should be included in the response. I wonder if we could use a similar mechanism on request.

@moxplod
Copy link
Author

moxplod commented Jun 8, 2019

My recommendation would be to use the separate publish and unpublish product methods that you already have written in the product service if you need to unpublish a product. So the developer knows they need to be explicit when doing that.

With the way, it is in the SDK right now with any updates I need to make to the product it unintentionally unpublishes the product. Which ended up causing a significant bug for me and had a lot of my users complaint.

@clement911
Copy link
Collaborator

Not a bad idea but ShopifySharp is a lightweight library and is intentionally designed to be a very thin layer over Shopify's REST api.
Adding additional logic like a publish/unpublish would be a departure from this goal.

@moxplod
Copy link
Author

moxplod commented Jun 11, 2019

Those methods already exist in this SDK library

@clement911
Copy link
Collaborator

Oh I had totally missed that!

@dkershner6
Copy link
Contributor

I believe those used to be the only way, and we switched to also using nulls due to a request.

This is just one of those things that is 6 of one, half dozen of another.

@clement911
Copy link
Collaborator

Related issue: #284

@nozzlegear nozzlegear added the object updating Bugs and strange behavior around updating objects, primarly related to #284 label Jun 13, 2019
@SeanoNET
Copy link
Contributor

I encountered this today, i resolved it by checking the PublishedAt in Shopify before updating the given product. This is not ideal as it requires an extra API call before updating the product.

I do like @moxplod idea of removing the ability to set PublishedAt in ProductService.UpdateAsync() and instead call ProductService.UnpublishAsync() to unpublish a product.

My recommendation would be to use the separate publish and unpublish product methods that you already have written in the product service if you need to unpublish a product. So the developer knows they need to be explicit when doing that.

If i am not missing anything, a possible solution could be to add a [JsonIgnore] to published_at when serializing the product update request and use published = true instead in ProductService.UnpublishAsync()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
object updating Bugs and strange behavior around updating objects, primarly related to #284 question
Projects
None yet
Development

No branches or pull requests

5 participants