Authorize uuid for update_positions on ResourceController #4992
Conversation
github-actions
bot
added
the
changelog:solidus_backend
|
Sorry seems the CI have not the function gen_random_uuid() on postgres for instance so this PR can be closed if uuid is not in your vision at short terms ;) Have a nice day ;) |
|
It seems like Postgres needs to load What if we keep the change without the UUIDs test you added? Do you see any cons for those who are not using UUIDs? |
|
@kennyadsl thanks for answer true fact for the breaking change. Indeed good idea to keep the code even if the spec don't show the need of change but in this particular case it is ok in my opinion. And I see no cons due to the fact that "already in place" specs are green on my computer. So ok for me for people who are not using UUIDs. I will modify this PR. Thanks |
julienanne
force-pushed
the
improve-update-positions-compatibility-with-uuid
branch
2 times, most recently
from
5667c49 to
7fea311
Compare
|
@kennyadsl it seems that I have doing something wrong (CI is red) but I don't know what, can you help me please ? |
|
Should be fixed when we merge #5010, I will ask you for a rebase when done, thanks! |
|
@julienanne please rebase now! |
julienanne
force-pushed
the
improve-update-positions-compatibility-with-uuid
branch
from
7fea311 to
cae6e5e
Compare
|
@kennyadsl seems better thanks ;) |
|
@solidusio/core-team thoughts? |
| @@ -82,7 +82,7 @@ def update_positions | |||
| records = model_class.where(id: positions.keys).to_a | |||
|
|
|||
| positions.each do |id, index| | |||
| records.find { |r| r.id == id.to_i }&.set_list_position(index) | |||
| records.find { |r| r.id.to_s == id }&.set_list_position(index) | |||
There was a problem hiding this comment.
Non-blocking, but given we're not testing it, adding a code comment here would be good.
There was a problem hiding this comment.
I write a com, good idea indeed when two implementations make the spec green and we want one implementation more than the other.
But my English is not accurate so feel free to suggest something more accurate if needed ;)
Thanks ;)
julienanne
force-pushed
the
improve-update-positions-compatibility-with-uuid
branch
from
cae6e5e to
5832d47
Compare
|
No spec red on my local machine, flaky test ? |
|
@julienanne thanks for your contribution! 🎉 |
|
@kennyadsl Do you need backports for v3.3, v3.2 and v3.1 or you think it is not necessary ? Have a nice day |
|
The backport would have been automatic if I remembered to add the proper labels to the PR, I'm sorry! Let me see if we can trigger the workflow anyway after the PR is merged. |
|
@waiting-for-dev suggested to add the labels even if the PR is merged, and it should work. Let me try! |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
|
Backport done @julienanne! 3.1 is not supported anymore following our policy here, though. |
|
Thanks |
Summary
In the backend section, from Ref 248b39f we can't sort object list when we use uuid for the object's id.
We need to allow records save for the case of uuid.
This change need to be backported until the version 3.1.0 if I'm right ;)
Hope it can help.
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: