NPM Resource Changed ? ? Pending

User tests: Successful: Unsuccessful:

avatar joomdonation
joomdonation
16 May 2021

Pull Request for Issue #33861.

Summary of Changes

This PR fixes the issue #33861

Testing Instructions

  1. Confirm the issue described at #33861
  2. Download update package for this PR at https://ci.joomla.org/artifacts/joomla/joomla-cms/4.0-dev/33909/downloads/43761/Joomla_4.0.0-beta8-dev+pr.33909-Development-Update_Package.zip , go to System -> Update -> Joomla, upload this package ad run the update
  3. Then check and confirm that the issue sorted.
avatar joomdonation joomdonation - open - 16 May 2021
avatar joomdonation joomdonation - change - 16 May 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 16 May 2021
Category JavaScript Repository NPM Change
f73790f 16 May 2021 avatar joomdonation CS
avatar joomdonation joomdonation - change - 16 May 2021
Labels Added: NPM Resource Changed ?
34b45a0 16 May 2021 avatar joomdonation CS
avatar joomdonation joomdonation - change - 16 May 2021
The description was changed
avatar joomdonation joomdonation - edited - 16 May 2021
avatar sandramay0905 sandramay0905 - test_item - 16 May 2021 - Tested successfully
avatar sandramay0905
sandramay0905 - comment - 16 May 2021

I have tested this item successfully on 34b45a0


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33909.

avatar Fedik
Fedik - comment - 16 May 2021

you have found bug correctly, but the fix not really ;)

You have to skip "mirror" row, not last row.
The mirrored row is removed in "dragend" event, it would be best to call the request here, however this event also triggered when user cancel sorting, so it a bit tricky.

Just make the loop like this:

for (i = 0, l = rows.length; l > i; i += 1) {
  // Skip a mirror element
  if (rows[i].closest('.gu-mirror')) continue;

  rows[i].value = i + 1;
  result.push(`order[]=${encodeURIComponent(rows[i].value)}`);
  result.push(`cid[]=${encodeURIComponent(inputRows[i].value)}`);
}
avatar joomdonation
joomdonation - comment - 16 May 2021

@Fedik No surprise with my beginner JS level :D. I asked @dgrammatiko before and he confirmed the fix looks OK. I tested and it seems it works, too. Maybe you can help with a proper fix? I will be happy to close this PR

avatar Fedik
Fedik - comment - 16 May 2021

just copy paste the code from my last comment ;)

avatar joomdonation
joomdonation - comment - 16 May 2021

Thanks @Fedik. Tested and it worked, so I committed the change.

avatar Ruud68
Ruud68 - comment - 16 May 2021

Hi Tuan, Although this fixes the issue initially reported here: #33861 testing this a bit further exposes another bug in the logic:
create 6 custom fields, set the view list length to 5 (so only the first 5 are shown).
now when sorting on the first page, the sorting looks okay until you F5 then field number 6 turns out to be not the last but the first...
Should I create a new issue for this?

avatar Fedik
Fedik - comment - 16 May 2021

to fix eslint, add

// Skip a mirror element
// eslint-disable-next-line no-continue
if (rows[i].closest('.gu-mirror')) continue;

this no-continue rule is pointless, I would disable it globally

avatar joomdonation
joomdonation - comment - 16 May 2021

@Ruud68 I'm unsure If my js skill is enough to address that. So just leave this PR here for a while, If I cannot get it sorted, I will close and ask @Fedik or @dgrammatiko for help

One thing I don't understand is why ordering of fields are set to 0 when it is created? If we create 3 fields, I would expect ordering of the fields set to 1, 2, 3..... and so on. For case, I think the last field has ordering set to 0, so after the sorting, it still has ordering 0, while other fields on the first page has ordering set to right value and it causes the error

So maybe something related to field ordering when it is first saved?

a34e44b 16 May 2021 avatar joomdonation CS
avatar Fedik
Fedik - comment - 16 May 2021

So maybe something related to field ordering when it is first saved?

This something in PHP side on save, I suspect.

avatar joomdonation
joomdonation - comment - 16 May 2021

This something in PHP side on save, I suspect.

I think so. But we might also have problem with client side, too. If from second page, we start ordering again with value 0 (or 1) for first row, we would get the issue (need to look at more carefully, just a quick analyst)

avatar Ruud68
Ruud68 - comment - 16 May 2021

makes me wonder what happens when you have a list of e.g. 100K items you sort, would it rewrite all the records ordering fields?
Just a thought as I have a customer who has that amount of sortable items...

avatar joomdonation
joomdonation - comment - 16 May 2021

As I guessed, on new page (second, third page....), we re-start with ordering of items again from 1, so we have a big bug here :( .

avatar Ruud68
Ruud68 - comment - 16 May 2021

And to further thicken the plot: in J3 ordering is using negative numbers, so e.g. when you set field one below field 2 then field one gets ordering of 1 and field 2 gets ordering of -1.
It should also be tested how ordering of fields will handle these when upgrading to 4.0 and then reordering n 4. Will it correctly handle the negative numbers etc. as the logic of ordering has apparently changed.

avatar joomdonation
joomdonation - comment - 16 May 2021

OK. I think this PR solves a valid issue. For re-ordering items logic, I open a separate issue #33917 . @Ruud68 If you have any other concerns, please add it to the issue so that we can look at it later.

avatar Fedik
Fedik - comment - 16 May 2021

we re-start with ordering of items again from 1

hmhm the script just send the final positions to the backend, and backend should recalculate the final order for given ID, depend from element position in request.
In theory.
Not sure how it work now, in backend.

avatar joomdonation
joomdonation - comment - 16 May 2021

Not sure how it work now, in backend

From my quick look, the posted ordering will be saved for each record (if the value for that change), then the ordering if related items will be re-calculated (I haven't looked at that re-calculate logic yet)

So from page 2 and page 3 for example, the item will have ordering again, set to 1, 2, 3......, so it is completely wrong. We need to look at it and correct it in a separate PR

avatar joomdonation joomdonation - change - 16 May 2021
The description was changed
avatar joomdonation joomdonation - edited - 16 May 2021
avatar Fedik Fedik - test_item - 16 May 2021 - Tested successfully
avatar Fedik
Fedik - comment - 16 May 2021

I have tested this item successfully on a34e44b


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33909.

avatar joomdonation
joomdonation - comment - 16 May 2021

@sandramay0905 Could you please re-test this PR ? Just makes sure it works like in your previous test. There is another bug related to re-ordering items but we will address it in a different PR

avatar sandramay0905 sandramay0905 - test_item - 16 May 2021 - Tested successfully
avatar sandramay0905
sandramay0905 - comment - 16 May 2021

I have tested this item successfully on a34e44b


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33909.

avatar joomdonation joomdonation - change - 16 May 2021
Status Pending Ready to Commit
avatar joomdonation
joomdonation - comment - 16 May 2021

Thanks for testing !


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33909.

avatar joomdonation
joomdonation - comment - 16 May 2021

Thanks for testing !


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33909.

avatar Quy Quy - change - 17 May 2021
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-05-17 16:40:35
Closed_By Quy
Labels Added: ?
avatar Quy Quy - close - 17 May 2021
avatar Quy Quy - merge - 17 May 2021
avatar Quy
Quy - comment - 17 May 2021

Thank you!

Add a Comment

Login with GitHub to post a comment