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

add flip ordering batch process to category manager #11529

Merged
merged 3 commits into from Aug 4, 2017

Conversation

rdeutz
Copy link
Contributor

@rdeutz rdeutz commented Aug 9, 2016

Pull Request for Issue #11103 .

This is not a real fix for the issue, but it is a way to make the B/C break not a big pain. In combination #11475 it allows sites that are using ordering to invert the ordering of articles with one click.

Summary of Changes

It adds a yes/no select to the batch process in the category manager to invert the ordering within categories.

Testing Instructions

  • 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“
  • Is is easier when you limit the view to one category and set leading 0, intro 20, columns 1, links 0
  • check the view and memorise the ordering
  • go to backend, select the category in the category manager and click on batch.
  • select yes under "Reverse the ordering of selected categories"
  • process
  • go to the frontend, the ordering should now inverted

Note: I have tested this with a category with 30000 articles and there was only a small delay.

Documentation Changes Required

Because this is a "fix" for a B/C break we should put at a prominent place in the release notes.

@@ -345,6 +345,7 @@ JLIB_HTML_ADD_TO_THIS_MENU="Add to this menu."
JLIB_HTML_BATCH_ACCESS_LABEL="Set Access Level"
JLIB_HTML_BATCH_ACCESS_LABEL_DESC="Not making a selection will keep the original access levels when processing."
JLIB_HTML_BATCH_COPY="Copy"
JLIB_HTML_BATCH_FLIPORDERING_LABEL="Reverse the ordering of selected categories"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this say reverse the order of articles in the selected categories?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yes, you are right, can you suggest a sentence to make it clear as possible

Copy link
Contributor

Choose a reason for hiding this comment

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

Reverse the ordering of all articles in the selected categories

@brianteeman brianteeman removed the Language Change This is for Translators label Aug 9, 2016
@joomla-cms-bot joomla-cms-bot added the Language Change This is for Translators label Aug 9, 2016
@Sieger66
Copy link
Contributor

Sieger66 commented Aug 9, 2016

This is not a good solution for the issue because:

If you have a site with for example 102 featured aticles and the article-ordering-numbers are for example:

-90
-80
1
2
3
...
99
100

and before your solution the featured articles came in the front-end in the same order.

After your solution and for example 2 new featured articles the order of the articles in front-end is:

102
101
100
99
98
...
3
2
1
-80
-90

First the new 2 articles then artcles in reverse oder and then the sticky-oder-articles in reverse order at last.

Sticky ordering is no longer possible and 102 articles now i must sort.


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

@rdeutz
Copy link
Contributor Author

rdeutz commented Aug 9, 2016

@Sieger66 This isn't for the featured article ordering it works only for the category ordering.

@Sieger66
Copy link
Contributor

Sieger66 commented Aug 9, 2016

But the issue is also by the featured articles!


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

@rdeutz
Copy link
Contributor Author

rdeutz commented Aug 9, 2016

yes, but let us solve one problem first an not mix problems, if we agree that here is working for categories we an easily add a flip ordering for featured articles. That's the reason I am writing test instruction to make clear what is this PR about.

@ggppdk
Copy link
Contributor

ggppdk commented Aug 9, 2016

aaaahhhh, how did i forget about the negative "sticky" ordering numbers ?

anyway since your reversing code uses 1 query per records

Split it in 2 parts

  • reverse positive ones
  • reverse negative ones moving them to -2147483648

-1 becomes -2147483648
-2 becomes -2147483647
-3 becomes -2147483646

the above is good for 32 bit / 64 bit / 128 bit , etc

Then the reverse ordering

  • will always typecast the "signed" ordering column in the listing queries, to unsigned for almost zero performance cost

and it works

you can continue to have sticky orderings by adding the new article that you want to be sticky at MAX(of negative orderings) + 1

Then we can enhance the drag and drop ordering to make use of it,
and we have back a very nice feature (sticky ordering),
that drag and drop ordering has removed

@csthomas
Copy link
Contributor

csthomas commented Aug 9, 2016

IMHO sticky ordering, means special featured article can be wrapped by a module.
But I do not know if we can remove "sticky ordering" from articles and featured.

About performance.

Please test below queries on big tables when column ordering has an index key.

ALTER TABLE `#__content_frontpage` ADD INDEX ( `ordering` ) ;

Then compare two queries, you can use descibe to see how mysql use that.

This query does not use index key.

SELECT SQL_NO_CACHE *
FROM `#__content_frontpage`
ORDER BY CAST( ordering AS UNSIGNED ) DESC

vs

This query use index key and it is fast.

SELECT SQL_NO_CACHE *
FROM `#__content_frontpage`
ORDER BY ordering DESC

@ggppdk
Copy link
Contributor

ggppdk commented Aug 9, 2016

a union can be used to avoid the typecast

@Sieger66
Copy link
Contributor

Sieger66 commented Aug 9, 2016

@rdeutz :
Next problem with this solution:
If any website have a modul for example:

Extensions -> Modules -> New -> Articles - Category (This module displays a list of articles from one or more categories.) -> Ordering Options ->
Article Field to Order By is "Article Order" and
Ordering Direction is "Ascending"

then this Modul is list the articles in reverse direction with this solution.

Self Problem with other Extensions who use the ordering-numbers of articles.


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

@csthomas
Copy link
Contributor

Yes this break B/C. Modules on J3.5.1 and lower works OK, but in J3.6 this is a mess.

IMHO for users the best way will be

  1. Add button "Reverse Ordering" in categories (from this PR)
  • but reverse ordering should be use with where(state >= 0)
  1. And merge PR Fix the ordering of articles and featured #11139.

After that we can back to MAX+1 for ordering in J4.


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

@rdeutz
Copy link
Contributor Author

rdeutz commented Aug 10, 2016

more and more I think about this, it seems to me that reverting the original PR that made the B/C break and then look at the performance problem is the way to go

@ggppdk
Copy link
Contributor

ggppdk commented Aug 10, 2016

Fixing the JTable reorder is something to be done anyway

  • now it does 1 query per record,

a PR had been proposed

  • but it was good only for mysql
  • that PR made the assumption that the articles already had a unique order number (thus it did not fix them if they did not have unique)

I proposed another PR should work in all dbs , addressing the performance issue as it does 1 DB update per 2000 or per 10000 records

  • it works in all DBs (may need some patching for ms sql)
  • it forces unique orderings as before

only it continues to have some medium memory requirements as existing re-order has ... if you try to re-order e.g. 20,000 records (but anyway large websites have memory limit 5x that what will be need, e.g. needed 40-50 MBs and they will have 256 MBs)

#11184
(just i need to rebase it and remove irrelevant changes from article model)

we could fix the JTable reorder performance and revert the change in using last order for new articles

  • also another reason to fix , JTable reorder performance is that it messing up reorder in large menus, due to race conditions in updating the table

@csthomas
Copy link
Contributor

IMHO revert #8576 and add performance should be equal to PR #11139.

A few people complain about large ordering numbers
but it could be changed in templates to display more friendly values in the future.

Sticky ordering will still work. It is full B/C and performance good.

@brianteeman
Copy link
Contributor

@rdeutz I think thats the only way

On 10 August 2016 at 09:12, Georgios Papadakis notifications@github.com
wrote:

Fixing the JTable reorder is something to be done anyway

  • now it does 1 query per record,

a PR had been proposed

  • but it was good only for mysql
  • that PR made the assumption that the articles already had a unique
    order number (thus it did not fix them if they did not have unique)

I proposed another PR should work in all dbs , addressing the performance
issue as it does 1 DB update per 2000 or per 10000 records

  • it works in all DBs
  • it forces unique orderings as before

only it continues to have some medium memory requirements as existing
re-order has ...
if you try to re-order e.g. 20,000 records

#11184 #11184
(just i need to rebase it and remove irrelevant changes from article model)

we could fix the JTable reorder performance and revert the change in using
last order for new articles

  • also another reason to fix , JTable reorder performance is that it
    messing up reorder in large menu items, due to race conditions in updating
    the table


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#11529 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABPH8VoN7YAA_xxXF_B8lGriPEuivysQks5qeYfjgaJpZM4Jf_G1
.

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

@ggppdk
Copy link
Contributor

ggppdk commented Aug 10, 2016

In any case, JTable reorder performance needs to be addressed
regardless of what is decided on the new article orderings

about using large ordering numbers PR #11139 (it reverts #8576)
is not a bad option either (i have tested it)

@csthomas
Copy link
Contributor

For reorder performance #11184 is also good, but I will have to test transaction like:

BEGIN

UPDATE ....
UPDATE ....
UPDATE ....

COMMIT

@ggppdk
Copy link
Contributor

ggppdk commented Aug 10, 2016

Even if you use transaction
why use multiple updates ?

e.g. 10000 updates instead of 4 updates ?

@csthomas
Copy link
Contributor

Yes the big minus of my above is that php will have to send a lots of separated queries to sql server.

Then I vote to apply PRs #11139 + #11184 + #11529 (with sql where state >= 0)

#11529 may be helpful for users who start joomla from 3.6.

@rdeutz
Copy link
Contributor Author

rdeutz commented Aug 10, 2016

I have sent an email to the PLT list to vote on reverting #8576, I will let you know the result of the vote.

@alikon
Copy link
Contributor

alikon commented Aug 11, 2016

i 'll respectfully wait for PLT decision,
when we need to deal with perfomance, most of the times, is a trade off choice

@gwsdesk
Copy link

gwsdesk commented Aug 17, 2016

I hope wisdom prevails and that we indeed revert #8576 and sucessively look into the performance. Again as stated before:

IMHO under no circumstances can we ask people to manually re-arranged thousands of articles. I think it is wrong to "quickly" patch this issue with a solution that is not covering all options and is not well discussed nor using proper coding as I read the comments correct #11139. Once again I propose to PLT to revert this to J3.5.1 methods and after that we work on a proper solution that covers all ordering (featured/articles/categories/modules/ect

@csthomas
Copy link
Contributor

csthomas commented Aug 17, 2016

With #11139 you do not need to do anything like manually re-arrange. Everything is automatically.
Only minus is large numbers in ordering column (which J3.6 does not display now).

Style of codding mainly has been corrected.

I have problem with explaining every detail in English.
but may be you can understand my explanation at https://github.com/joomla/joomla-cms/issues
/11103#issuecomment-235849244

@gwsdesk
Copy link

gwsdesk commented Aug 24, 2016

@rdeutz Any feedback on ?

I have sent an email to the PLT list to vote on reverting #8576, I will let you know the result of the vote.

@rdeutz
Copy link
Contributor Author

rdeutz commented Aug 24, 2016

@gwsdesk it is reverted

@gwsdesk
Copy link

gwsdesk commented Aug 24, 2016

Thanks the Lord ;-)

On 8/24/2016 1:33 PM, Robert Deutz wrote:

@gwsdesk https://github.com/gwsdesk it is reverted


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11529 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHzLNr_i21-hW4vz_dd6CgRnVtKFZSGZks5qi-XGgaJpZM4Jf_G1.

@brianteeman
Copy link
Contributor

@rdeutz as the offending PR has been reverted can this be closed?


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

@rdeutz
Copy link
Contributor Author

rdeutz commented Sep 6, 2016

The function is not bad at all so the question is will we have a possibility to flip the ordering in one go. It might to used not so often, but I have to fix the ordering on one site when I update to 3.6.3 and maybe other people need to do the same. Let us look if people test it and when it gets enough tests then it can go in.

@gwsdesk
Copy link

gwsdesk commented Sep 6, 2016

Robert
I missed one setting. Works like a charm indeed! (see proof http://gwsdev2.work/en/test)

@gwsdesk
Copy link

gwsdesk commented Sep 6, 2016

I have tested this item ✅ successfully on fc4f2ed

I missed one setting. Works like a charm indeed! (see proof http://gwsdev2.work/en/test)


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

@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 thinks this makes sense as a function, it would need test to go in. But if nobody has really an interest it can be closed

@ghost
Copy link

ghost commented May 28, 2017

@rdeutz Questions for "Test Instructions":

Set the the secondary ordering in Articles options to „Ordering Reverse“

didn't found this Setting, guess you mean (but theres no "Ordering Reverse"):

bildschirmfoto 2017-05-28 um 08 06 25

Set the the secondary ordering for the menu to „Use Global“

Global is set on "Ordering", Menu shows for Global:

bildschirmfoto 2017-05-28 um 08 14 06

@carcam
Copy link

carcam commented Jun 3, 2017

I cannot follow the testing instructions:

Set the the secondary ordering in Articles options to „Ordering Reverse“
Is this in the category view menu item or in the com_content options? After applying the patch "Ordering reverse" is not showing in the dropdown. What am I missing @rdeutz ?


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

@Sieger66
Copy link
Contributor

@franz-wohlkoenig and @carcam

System -> Global Configuration -> Articles -> Shared -> Article Order -> Ordering Reverse


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

@ghost
Copy link

ghost commented Jul 20, 2017

I have tested this item ✅ successfully on fc4f2ed


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

@ghost
Copy link

ghost commented Jul 20, 2017

RTC after two successful tests in favor of resolving conflicting Files.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 20, 2017
@rdeutz rdeutz added this to the Joomla 3.8.0 milestone Jul 21, 2017
@rdeutz rdeutz self-assigned this Jul 21, 2017
@mbabker
Copy link
Contributor

mbabker commented Jul 25, 2017

@rdeutz This needs the merge conflict resolved then we can merge.

@rdeutz
Copy link
Contributor Author

rdeutz commented Jul 27, 2017

Solved merge conflicts, maybe someone can re-test @franz-wohlkoenig @gwsdesk @carcam

@mbabker mbabker merged commit 3effc45 into joomla:staging Aug 4, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 4, 2017
@rdeutz rdeutz deleted the add_reverse_ordering_batch branch February 20, 2020 22:33
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.

None yet

10 participants