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

Reverse ordering #11475

Merged
merged 7 commits into from
Jul 6, 2017
Merged

Reverse ordering #11475

merged 7 commits into from
Jul 6, 2017

Conversation

rdeutz
Copy link
Contributor

@rdeutz rdeutz commented Aug 5, 2016

Executive summary

This adds a additional ordering option to the category ordering.

Background information

A while ago we changed the default for the ordering field, before that change a new article got „0“ as ordering so that article was the first one in a blog view when ordered by ordering. The change was that a new article now gets max(ordering) as default value and that will show an article as the last when ordered by ordering. The change was good because we had some performance problems when the ordering value was updated on a site with many articles. So no complaining about the change itself. The problem now is that you have to reorder articles because your article is now the last one in the category. If you do not have this manual step then you need to switch the ordering and use reverse ordering.

Backwards compatibility

Because it is a new option nothing should change

Translation impact

Two new language tags added.

Core changes

Addition to ContentHelperQuery::orderbySecondary

Testing instruction

  • Apply patch
  • You need a category view with articles
  • Set the the secondary ordering in Articles options to „Ordering Reverse“
  • Set the the secondary ordering for the menu to „Use Global“
  • check the view and compare with the backend list of articles, use in both occasions the same ordering.

Should be the same ordering in front and backend

  • you can also change the menu item ordering and play around with the combinations

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-staging labels Aug 5, 2016
@brianteeman
Copy link
Contributor

Corrected typo in the title ;)

@brianteeman brianteeman changed the title Reserve ordering Reverse ordering Aug 5, 2016
@rdeutz
Copy link
Contributor Author

rdeutz commented Aug 5, 2016

@brianteeman starting to hate a word :-)

@ggppdk
Copy link
Contributor

ggppdk commented Aug 5, 2016

Have you seen this PR ?
#11139

  • it fixes B/C break
  • it maintains the objective of avoiding JTable::reorder() from being triggered and running on all records

@ggppdk
Copy link
Contributor

ggppdk commented Aug 5, 2016

but arguably this PR is independent of #11139

@rdeutz
Copy link
Contributor Author

rdeutz commented Aug 5, 2016

@ggppdk yeah you are right this here has nothing to do with #11139, further more I have my problems to see the what #11139 is trying to achieve

@ggppdk
Copy link
Contributor

ggppdk commented Aug 5, 2016

what #11139 is trying to achieve

2 objectives

  • fix B/C break and create new articles and featured ordering on top (with smallest ordering)
  • avoid JTable::reorder() from running on new article creation

the JTable::reorder() running on new article creation and executing hundrends or thousands or queries and this was the reason to create new article with highest ordering

but the slow new article saving is only 1 of the problems JTable::reorder causes:
see here 1 more here: #4303

@rdeutz
Copy link
Contributor Author

rdeutz commented Aug 5, 2016

@ggppdk thanks for the summery, but I don't think it makes sense to fix a B/C break after a lot of versions. Someone could argue to change it back now is also a B/C break. I will look into the issue at hand

@ggppdk
Copy link
Contributor

ggppdk commented Aug 5, 2016

but I don't think it makes sense to fix a B/C break after a lot of versions. Someone could argue to change it back now is also a B/C break

Please forgive me for annoying you, but you are wrong

The B/C break is here and it effects 100% of the records in all web-sites that want to use order by ordering number for listing articles at frontend

  • it would have been true if people were slowing trying to change the order of the items (i say slowly because in some cases you have thousands of records) !

but they do not try to make them reverse (instead some try to use creation date, some other suffer dragging from the bottom up through many records)

Why they have not tried to make them reverse ?
Because the B/C break has made the ordering number of new articles be last cannot be used (yet)
because revese ordering in the listing queries does not exist,

  • you are trying to add it now

so noone is yet trying to use reverse ordering, they will start after you merge this and after it is released
and then you argument will start to have an effect with time

and lets say that we merge this , and people want to use it, they will have to manually re-order hundrends or thousands of articles and then use it

@ggppdk
Copy link
Contributor

ggppdk commented Aug 5, 2016

and lets say that we merge this , and people want to use it, they will have to manually re-order hundrends or thousands of articles and then use it

continuing the above

Lets say that i have 200 records, now order normally,

  • now for every new record i (the user) drag it up 200 positions to place it in correct place

Now lets say i want to avoid the above
and take advantage of

  • new article having biggest / last order number
  • and of the new reverse ordering that you want to add

how am i going to reverse these records ?
i am gone drag and drop 200 records over 200/2 = 100 positions on average ?

what if i have 2000 records ?

@ggppdk
Copy link
Contributor

ggppdk commented Aug 5, 2016

and then if you use ordering numbers in more than 1 place,

  • you need to do the same for every category that uses it

at minimum we would need a button to make the "Revese all records" task automatic

then we can keep the B/C break

  • and we can demand that all users that now make use of ordering numbers ...

to update their site configuration to use the new reverse ordering and reverse all their existing records via the new button

@rdeutz
Copy link
Contributor Author

rdeutz commented Aug 5, 2016

But this affects only new items, if I got it right then the change was that new item got max(ordering) as value and not 0 as it was before, correct me if I am wrong

@ggppdk
Copy link
Contributor

ggppdk commented Aug 5, 2016

But this affects only new items, if I got it right then the change was that new item got max(ordering) as value and not 0 as it was before, correct me if I am wrong

correct,

so now sites that continue to use ordering numbers

  • will make backend manager pagination e.g. 200 records
  • and then drag up the record to the top

@ggppdk
Copy link
Contributor

ggppdk commented Aug 5, 2016

and then drag up the record to the top

it is not too much of a problem for sites with e.g. 200 items in featured view, and if you do not create articles often,

so people have adjusted to doing this extra work, it takes less than a minute to do if you have a few hundrend records

this PR is a solution,
but it should also be with a new button to reverse all existing records at once

@rdeutz
Copy link
Contributor Author

rdeutz commented Aug 5, 2016

Let me test the other issue, we are having a discussion at the wrong spot, this PR here is a simple add of an option to the sorting. Tester will run away when they see all the back and force discussion :-)

@csthomas
Copy link
Contributor

csthomas commented Aug 5, 2016

Joomla 3.6.0 broke B/C for article/featured ordering column.

@rdeutz I also created a similar PR #11134 to yours but without enthusiasm from others then I closed it. To understand #11139 please read the comment from #11103 (comment)

@ggppdk
Copy link
Contributor

ggppdk commented Aug 5, 2016

@rdeutz

about benefits of PR #11139
see my comment :
#11139 (comment)

@alikon
Copy link
Contributor

alikon commented Aug 11, 2016

I have tested this item ✅ successfully on


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

@ghost
Copy link

ghost commented Jan 10, 2017

I have tested this item ✅ successfully on 2cd6afb


but: String like JGLOBAL_VOTES, JGLOBAL_FIELD_GROUPS or show like Global Option (Hide) isn't shown.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/11475.

@jeckodevelopment
Copy link
Member

@rdeutz can you look at the conflicts please?

@mbabker
Copy link
Contributor

mbabker commented May 21, 2017

What's the status here?

@rdeutz
Copy link
Contributor Author

rdeutz commented May 22, 2017

I still think this makes sense, will fix the conflicts later

@rdeutz rdeutz closed this May 22, 2017
@rdeutz rdeutz reopened this May 22, 2017
@rdeutz rdeutz self-assigned this May 22, 2017
@@ -478,6 +479,7 @@ JGLOBAL_ORDER_DESCENDING="Descending"
JGLOBAL_ORDER_DIRECTION_LABEL="Direction"
JGLOBAL_ORDER_DIRECTION_DESC="Sort order. Descending is highest to lowest. Ascending is lowest to highest."
JGLOBAL_ORDERING="Article Order"
JGLOBAL_REVERSE_ORDERING="Article Reverse Order"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alphabetize/reorder string.

@rdeutz
Copy link
Contributor Author

rdeutz commented Jul 6, 2017

@Quy thanks, fixed

@mbabker mbabker merged commit 1661862 into joomla:staging Jul 6, 2017
@mbabker mbabker added this to the Joomla 3.7.4 milestone Jul 6, 2017
@rdeutz rdeutz deleted the reserve_ordering branch February 20, 2020 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants