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

Fix backend articles manager not listing articles in sub-categories when categories filter is active #18179

Merged
merged 7 commits into from Oct 9, 2017

Conversation

@ggppdk
Copy link
Contributor

@ggppdk ggppdk commented Oct 1, 2017

Pull Request for Issue #18178

Summary of Changes

Listing articles in sub-categories when categories filter is active is no longer working
This quite a big annoyance for people that need it,

  • also issue may go unnoticed by people that need

Bug was introduced when category filter in articles manager was converted to multi-value

  • multi value category filtering code does not support listing sub-category articles (yet)

This PR fixes

  • the very common case of filtering by a single category
  • but also fixes the case of selecting more than 1 category in the filter
  • also the (max) level filter is applied per filtered category

Testing Instructions

In a Joomla installation that e.g. has testing articles
filter by category "Joomla" and you will notice that only 11 articles are shown , those that belong to category "Joomla"

also please test with

  • 2 categories without (max) level filter
  • 2 categories with (max) level filter

Expected result

Those that belong to subcategories should also be listed (when max level filter is not set)

Actual result

Those that belong to subcategories are not listed (despite max level filter not set)

Documentation Changes Required

None

…hen categories filter has only 1 category
@alikon
Copy link
Contributor

@alikon alikon commented Oct 1, 2017

i don't think this fix is correct
cause now we can select based upon multiple categories selection
so
if i choose to select multiple categories only, for me means i don't want articles from subcategories
but only articles from the selected categories

@degobbis
Copy link
Contributor

@degobbis degobbis commented Oct 1, 2017

Even if this is perhaps just a side effect of the multi-select funtion, so I find that now better so. I have previously missed seeing only the articles of a category without subcategories. If you want to see the articles of the subcategories too, then better by selecting it additional.


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

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 1, 2017

@alikon

The fix is very correct in terms that it restores the old and expected behaviour
(look at the code please)

  • when a single category is selected then we make use of category and the max-levels filter
  • when multi-categories are selected we only list in the selected categories

it is very bad UX to select a category and not be able list articles in sub-categories

In my own extension i have implemented also listing records in sub-categories even when selecting multiple categories (combining the max-level filter)

@degobbis
Copy link
Contributor

@degobbis degobbis commented Oct 1, 2017

To restore the old behavior, this patch is correct.
But I think it would be more logical to control the sub-categories with Max-Level when I want to see them. The selection of the category should only display those that were selected.
So I think the bug is that Max-Level does not change the display of the subcategories.


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

@alikon
Copy link
Contributor

@alikon alikon commented Oct 1, 2017

@ggppdk in your own extensions you are free to do whatever you want,
in the core i think you are free to propose whatever you want, but we are all free to discuss proposal and even to have different opinion

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 1, 2017

@degobbis

What you describe about multiple categories,
is trade-off that was accepted when category filter was made multi-value

Doing what you ask is possible
by creating multiple lft and rgt limiting clauses that are glued with OR

but e.g. @alikon believes otherwise

only the specific categories should be listed and
e.g. the max-levels filter should be hidden or a message should be shown to the user once per login session that articles are listed only in the specific (multiple-selected) categories

also the max-levels filter in single category starts at the level of the category

  • in the case of multiple categories it should propably start at the top, meaning it may be a little confusing to the user

  • let's fix this nasty UX bug for single category

  • and not add something that in my opinion would be a new feature

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 1, 2017

@alikon

This is bug fix of previous / expected behaviour,
please forget me mentioning my extension
I mentioned it only to show that i have a little experience on the topic

Also i do not care so much for this PR,
we can just leave it broken and live with it
you can ask me to close it and i will do

@alikon
Copy link
Contributor

@alikon alikon commented Oct 1, 2017

@ggppdk
i 'll always never ask you to close a pr
"simply because i don't like it"
i've just expressed how i've understand that new feature/behaviour #17668
maybe should have been better to discuss more and more deeply that feature/behaviour (#17668) before to merge it

@brianteeman
Copy link
Member

@brianteeman brianteeman commented Oct 1, 2017

We have a UX team - why not let them make the decision?

@alikon
Copy link
Contributor

@alikon alikon commented Oct 1, 2017

👍

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 1, 2017

Sure,
my most important notes and questions are in this answer:
#18179 (comment)

how to behave with single category selected ?

  • handle it differently than selecting multiple categories

how to behave with multiple category selected ?

  • hide the max level filter ?
  • show a warning message once per session that articles in sub-categories are not shown ?
@infograf768
Copy link
Member

@infograf768 infograf768 commented Oct 2, 2017

show a warning message once per session that articles in sub-categories are not shown ?

Not really in favour of that.
Suggestion if possible: a new filter to display sub-categories articles or not for all cases (single and multiple categories selected.

@Bakual
Copy link
Contributor

@Bakual Bakual commented Oct 2, 2017

Imho items from subcategories have to be shown when there is no level filter set. That is the old behavior and thus the expected one. Doesn't matter if one category is selected or multiple. The behavior has to be the same.
If you only want items from the selected categories, you set the level filter to 1. That's the point of that filter.

@degobbis
Copy link
Contributor

@degobbis degobbis commented Oct 2, 2017

In my opinion, with the multi-selective of the categories, the level filter field has become obsolete.
I can now choose my subcategories more flexibly than before.
What makes sense now would be a selection field for "show all subcategories".

For me personally this bug is welcome. Finally, ONLY the items appearing in the selected category are displayed.
This has always annoyed me before, still have to adjust the level for this.


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

@hrefferh
Copy link

@hrefferh hrefferh commented Oct 2, 2017

I have tested this item successfully on 92d7844


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

@pickeboe
Copy link

@pickeboe pickeboe commented Oct 2, 2017

We discovered this bug too. Today a client and myself was wondering about the new "feature" or the bug. My client called me and asked me, where to find his articles, listed in sub categories of a higher category. I said him, he should use the filter field "max level filter". He did. But couldn't find the expected articles. Why? I tested it, and must agree to him: this filter has broken! I agree with Bakuals posting.

Irrespective of the multi category filter mode, the "max level filter" should work like before. This is very important!


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

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 2, 2017

Ok fixed previous behaviour for multiple categories too

now we will have J3.7.x behaviour in case of filtering by multiple categories

Fixed the max level filter is usage too,
by making it to be based on the deepest category

@ggppdk ggppdk changed the title Fix backend articles manager not listing articles in sub-categories when categories filter has only 1 category Fix backend articles manager not listing articles in sub-categories when categories filter is active Oct 2, 2017

$baselevel = 1;
$subcat_items_where = array();
foreach($categoryId as $filter_catid)

This comment has been minimized.

@Quy

Quy Oct 2, 2017
Contributor

Add a space after foreach.

This comment has been minimized.

@ggppdk

ggppdk Oct 2, 2017
Author Contributor

thanks for the review

: $baselevel;
$subcat_items_where[] = '(c.lft >= ' . (int) $categoryTable->lft . ' AND c.rgt <= ' . (int) $categoryTable->rgt . ')';
}
$query->where(implode($subcat_items_where, ' OR '));

This comment has been minimized.

@Quy

Quy Oct 2, 2017
Contributor

The parameters are reversed. It should be: $query->where(implode(' OR ', $subcat_items_where));

This comment has been minimized.

@ggppdk

ggppdk Oct 2, 2017
Author Contributor

implode accepts them in this order too (historical reasons),
but yes better to place them as official function signature

@infograf768
Copy link
Member

@infograf768 infograf768 commented Oct 2, 2017

My test:
I have 2 root categories, each of them having a sub category.

  1. If I filter by these 2 root categories and do not define a level for Select Max Levels
    articles belonging to the root and children categories DO display fine

  2. If I set Select Max Levels to 1
    I then loose the subcategory articles from the second category in the order they show in the filter. Here the sub category of Categoria (it-it) (it-IT).
    But the articles form the first root categorie(fr-FR) subcategory DO display.

screen shot 2017-10-02 at 17 09 59

Note: I can't change the order of the root categories in the filter. I guess the order depends on their id.

  1. I need to set Select Max Levels to 2 to get back the display of the articles belonging to the subcategory of Categoria (it-it) (it-IT)
@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 2, 2017

@Quy , @infograf768

thanks for review, testing
i made a fix for depth level filter, and CS
please recheck / retest

@infograf768
Copy link
Member

@infograf768 infograf768 commented Oct 3, 2017

I have tested this item successfully on d1abe63


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

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 3, 2017

(the trade-off is that the max-level filter is now based on the deepest filtered category)

the alternative is to create 1 join to the categories DB table per category selected in the category filter
-- this way we will be able to distiguish the level per category selected

if you would want this, i could do

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 3, 2017

Another option to make this consistent is always use depth from top

@hrefferh
Copy link

@hrefferh hrefferh commented Oct 3, 2017

@ggppdk

You maybe right.
But for me it is confusing that when I filter root + sub1sub1 [ level: 2 ] to get results of sub2sub1 and sub3sub1 too!?

root
  |          
  |          
  |sub1       |sub2     |sub3
     |        |         |
     |        |         |
     |sub1    |sub1     |sub1
@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 3, 2017

yes i agree it can be confusing,

re-thinking of it , i think , maybe it is possible to do without extra joins
checking it

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 3, 2017

@hrefferh , @infograf768

done , now the (max) level filter is applied to every category individually
please retest

@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Oct 3, 2017

Also i found a bug in categories manager linking to article manager with wrong level filter, i will make a different PR

@hrefferh
Copy link

@hrefferh hrefferh commented Oct 3, 2017

I have tested this item successfully on fc7803e

Bingo!

root + sub1sub1 [ level: 2 ] => 11 hits

sub1sub1 [ level: 2 ] => 23 hits

root + sub1sub1 [ level: 2 ] && sub1sub1 [ level: 2 ] => 34 hits - identical to single filtering

GREAT! THANK YOU AGAIN FOR YOUR EFFORTS!!!


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

@ghost
Copy link

@ghost ghost commented Oct 3, 2017

@infograf768 can you please retest?

@infograf768
Copy link
Member

@infograf768 infograf768 commented Oct 4, 2017

I have tested this item successfully on fc7803e


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

@infograf768
Copy link
Member

@infograf768 infograf768 commented Oct 4, 2017

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC label Oct 4, 2017
@infograf768 infograf768 added this to the Joomla 3.8.2 milestone Oct 4, 2017
{
$categoryId = ArrayHelper::toInteger($categoryId);
$query->where('a.catid IN (' . implode(',', ArrayHelper::toInteger($categoryId)) . ')');
$categoryTable = JTable::getInstance('Category', 'JTable');
$subcat_items_where = array();

This comment has been minimized.

@mbabker

mbabker Oct 5, 2017
Contributor

Variable should be camel case.

This comment has been minimized.

@ggppdk

ggppdk Oct 5, 2017
Author Contributor

Done

@infograf768
Copy link
Member

@infograf768 infograf768 commented Oct 8, 2017

@mbabker
I guess this one can now get in.

@xinelope

This comment has been minimized.

Copy link

@xinelope xinelope commented on 92d7844 Oct 9, 2017

Jepp, it works, thank you 👍

@mbabker mbabker merged commit 7116be3 into joomla:staging Oct 9, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
@joomla-cms-bot joomla-cms-bot removed the RTC label Oct 9, 2017
@bobr666
Copy link

@bobr666 bobr666 commented Nov 30, 2017

I thought this fix was included in Joomla 3.8.2 but the feature still doesn't seem to be fixed


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

@zero-24
Copy link
Member

@zero-24 zero-24 commented Nov 30, 2017

@bobr666 can you please open a new issue with that thing that don't work for you (please explain with as mach as possible detail) so we can keep track on open issues. Thanks!
https://github.com/joomla/joomla-cms/issues/new

@sandstorm871
Copy link

@sandstorm871 sandstorm871 commented Mar 28, 2018

I only noticed this on a site we just updated to 3.8.6 and seem the problem still persists?
I have a category blog link set to include subcategories - set to all, but only the articles from the parent category show.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18179.
@ggppdk
Copy link
Contributor Author

@ggppdk ggppdk commented Mar 28, 2018

@sandstorm871

This was a fix for the DB model of the backend articles manager,
it is not related to filtering of the frontend views

@sandstorm871
Copy link

@sandstorm871 sandstorm871 commented Mar 28, 2018

Sorry, I misread this, I'll test further & start a new issue, as think this is a bug.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet