[sync-actions]: support empty orderHint about nodejs HOT 10 CLOSED

I just had a look at the code.
When you create a category without orderHint, a random orderHint will be assigned.

val concreteOrderHint: String = orderHint.getOrElse(Random.orderHint())

I find this choice quite strange.
I don't know the reason why it is implemented like this.
The change was done in 2013 by somebody who is working at CT anymore.

for me:

• either orderHint is required. Then we should not be able to set an empty string.
• or orderHint is optional. Then we should have a setOrderHint with an optional orderHint. Setting a defined empty string should still be refused.

In general, we refuse empty strings because theirs semantic are not clear.

from nodejs.

I'm cleaning up my old issues, so feel free to reopen if this is still relevant.

Hope you're all having a lovely Sunday at Commercetools :)

from nodejs.

🤔 this needs to be clarified a bit more with the API team. In the doc it says that the orderHint is optional, but the update action requires it.

It seems to me that the update action is "broken".
If the field is optional, the action should be setOrderHint, which means you can unset it by passing { action: 'setOrderHint' } without a value.
If the field is required (so the update action is correct), then you shouldn't be able to "unset" it and setting it as an empty string is kind of wrong.

from nodejs.

Why would you want to build an action with an empty string? If it is required, I would assume it needs a non-empty string, regardless of if it's supported by the API?

from nodejs.

Thanks for the quick reply @wizzy25.

Why would you want to build an action with an empty string?

To set the orderHint to an empty string. The API supports it, so the MC and SDK should support it as well, unless it is unintended behaviour (in which case it should be fixed in the API too).

In our MC case, when the user deletes the text in the orderHint input and saves the draft, we want to save the empty string. The current implementation generates an invalid update action which leads to the API throwing an error.

If it is required, I would assume it needs a non-empty string, regardless of if it's supported by the API?

As empty strings seem to be valid order hints that assumption seems wrong. I do not have the full context here though.

from nodejs.

@wizzy25 This is blocking one of our user stories. In case you are sure we don't want to accept empty strings as orderHint's, can you get confirmation from a PO?

We also accept duplicates and randomly generate orderHints when creating a category for the first time. This leads me to believe that empty strings are valid order hints as well.

from nodejs.

Okay... I don't have access to that repo

from nodejs.

/cc @OlegIlyenko @yanns 👆

from nodejs.

I asked about this in the Platform Support channel yesterday and Tobias talked to a few people about this personally. Stefan talked to Jens apparently.

The answer we got was that the docs are wrong and orderHint is not optional for a category. Once created it can not be removed.

The reason I assumed empty strings to be acceptable values is because it would ensure that this category is the first category when sorting naturally.

I am fine with either direction we take here. But we need it implemented so we can finish the user story.

I find this choice quite strange.

Yes, that is indeed quite strange. When using the MC to create a category it would end up in a random place in the list as soon as we start showing that list sorted by orderHint. For us, the desired UX would be to have the category added to the bottom of the list (talked to Jenn about this).

from nodejs.

Seems like we should sit together and figure this one out properly.

Problems with current approach:

• New categories not added to bottom of list, but randomly inserted
• Do we support empty strings or not (API supports it, node-sdk generates invalid udpate actions)
• Assuming it uses natural sorting: When sorting naturally and trying to sort ['-1', '1', '0', '0.0', '0.0000', '-0.01'] it ends up as ["-0.01", "-1", "0", "0.0", "0.0000", "1"] which is unexpected if you only use numbers (-0.01 coming before -1)
• Trying to place one category between two others is super hard if the two other categories use strings a and b. We basically need to use aa. Then if trying to place one between a and aa we'd need to use a0 or something.
• The behaviour of the field orderHint in products is widely different from the field orderHint in categories. Consistent behaviour is expected for fields with the same name.

Ideally:

• orderHint behaves just like in products (float from 0 to 1)
  • Resolves empty string issue
  • Resolves "drop a category between two others" issue
• New Categories are added to the bottom (only tricky if the last one is 0.999999999...)

For now we made orderHint a required field in the MC which works around the node-sdk generating invalid update-actions (Actions of type changeXy must always have value even if it is an empty string, as stated in this issue. Otherwise they should be set actions and the field should be optional - as far as I can tell). We also recommend to use only values between 0 and 1 to avoid the unexpected sorting issues.

Once we start the drag-n-drop user story for category reordering we will need a proper solution to this. Otherwise we're not able to calculate the the orderHint for the new position. I see that avoiding breaking changes might be hard here. That's why a meeting is probably the best. Everyone I got input from regarding this had a slightly different opinion here and being the messenger was quite lame :P @tdeekens will probably set up a meeting before we start the drag-n-drop story.

from nodejs.