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

redo of flo's tag branch and merge #17668

Merged
merged 6 commits into from Aug 23, 2017
Merged

redo of flo's tag branch and merge #17668

merged 6 commits into from Aug 23, 2017

Conversation

@JannikMichel
Copy link
Contributor

@JannikMichel JannikMichel commented Aug 22, 2017

Pull Request for Issue #5474 .

Summary of Changes

I try to finish the great work from @jrseliga and @florian1995.
@florian1995 fixed the problem that when you deselect for example all tags the list won't refresh.

The following test instructions are also from @jrseliga.

Testing Instructions

Administrator

Articles

  1. Create 3 articles:
    - Article 1 - Category 1 - Tag 1 - Author 1 - Public
    - Article 2 - Category 2 - Tag 1 & Tag 2 - Author 2 - Registered
    - Article 3 - Category 2 - Tag 2 - Author 1 - Super User
  2. View Articles
    - Use Search Tools

Just with a simple structure like this, there is a large number of potential test cases. Please refer to the New behavior section for an overview of the filtering logic

Featured Articles

  1. View Featured Articles
    Use Search Tools

Categories

  1. View Categories
    -Use Search Tools
    Developed@icampus
@Schmidie64
Copy link

@Schmidie64 Schmidie64 commented Aug 22, 2017

I have tested this item successfully on cedb5ee

@icampus
Everything works how it should be.


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

@eXsiLe95
Copy link
Contributor

@eXsiLe95 eXsiLe95 commented Aug 22, 2017

I have tested this item successfully on cedb5ee

Tested:

Testing

  1. Created multiple articles (like described)
  2. Went to article overview
  3. Used article filters

Before patch

Only one option per filter can be selected. Multiple filters are useable, but not multiple options on one filter.

With patch

Multiple options on a single filter can be selected. Works as described and expected.

Suggestions

You can not see what field matches a filter. Therefore, the initial value could be displayed instead of dummy text "Type or select some options". Maybe, you should use CSS placeholder instead of HTML value, too.

Tested @icampus


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

@@ -99,26 +99,38 @@ protected function populateState($ordering = 'a.id', $direction = 'desc')
$search = $this->getUserStateFromRequest($this->context . '.filter.search', 'filter_search');
$this->setState('filter.search', $search);

This comment has been minimized.

@Quy

Quy Aug 22, 2017
Contributor

Remove blank line.

$this->setState('filter.access', $access);

$authorId = $app->getUserStateFromRequest($this->context . '.filter.author_id', 'filter_author_id');
$this->setState('filter.author_id', $authorId);

This comment has been minimized.

@Quy

Quy Aug 22, 2017
Contributor

Remove blank line.

@@ -21,7 +21,7 @@ class ContentModelArticles extends JModelList
/**
* Constructor.
*
* @param array $config An optional associative array of configuration settings.
* @param array $config An optional associative array of configuration settings.

This comment has been minimized.

@Quy

Quy Aug 22, 2017
Contributor

Per Joomla's Coding Manual:
The minimum whitespace between any text elements, such as tags, variable types, variable names and tag descriptions, is two real spaces.

@@ -22,34 +22,37 @@
<field
name="category_id"
type="category"
multiple="true"
class="multipleCategories"

This comment has been minimized.

@Quy

Quy Aug 22, 2017
Contributor

The four attributes name, type, label and description should be written in this order and at the top of the element definition.

@@ -22,34 +22,37 @@
<field
name="category_id"
type="category"
multiple="true"
class="multipleCategories"
label="JOPTION_FILTER_CATEGORY"
description="JOPTION_FILTER_CATEGORY_DESC"
extension="com_content"
onchange="this.form.submit();"
published="0,1,2"
>

This comment has been minimized.

@Quy

Quy Aug 22, 2017
Contributor

Please read the coding style for XML when element is empty: https://developer.joomla.org/coding-standards/xml.html

>
<option value="">JOPTION_SELECT_AUTHOR</option>
</field>
>

This comment has been minimized.

@Quy

Quy Aug 22, 2017
Contributor

Fix tab and coding style.

label="JOPTION_FILTER_TAG"
description="JOPTION_FILTER_TAG_DESC"
mode="nested"
onchange="this.form.submit();"
>
<option value="">JOPTION_SELECT_TAG</option>

This comment has been minimized.

@Quy

Quy Aug 22, 2017
Contributor

Fix coding style.

label="COM_CONTENT_FILTER_PUBLISHED"
description="COM_CONTENT_FILTER_PUBLISHED_DESC"
onchange="this.form.submit();"
>

This comment has been minimized.

@Quy

Quy Aug 22, 2017
Contributor

Fix tab.

type="category"
multiple="true"
class="multipleCategories"
label="JOPTION_FILTER_CATEGORY"

This comment has been minimized.

@Quy

Quy Aug 22, 2017
Contributor

Fix order.

@@ -15,7 +15,7 @@
label="COM_CONTENT_FILTER_PUBLISHED"
description="COM_CONTENT_FILTER_PUBLISHED_DESC"
onchange="this.form.submit();"
>
>

This comment has been minimized.

@Quy

Quy Aug 22, 2017
Contributor

Use tabs and not spaces. Same issue in the other places.

@eXsiLe95
Copy link
Contributor

@eXsiLe95 eXsiLe95 commented Aug 23, 2017

I have tested this item successfully on 52d7b39

## Tested:

Testing

  1. Created multiple articles (like described)
  2. Went to article overview
  3. Used article filters

Before patch

Only one option per filter can be selected. Multiple filters are useable, but not multiple options on one filter.

With patch

Multiple options on a single filter can be selected. Works as described and expected.

Notes

Since the update, every field has its own placeholder. Good job!

Tested @icampus


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

@DrDreave
Copy link
Contributor

@DrDreave DrDreave commented Aug 23, 2017

I have tested this item successfully on 52d7b39

Operating System

  • Joomla! 3.8.0-beta3-dev
  • PHP 5.6.2
  • MySQLi 5.5.38
  • Apache/2.2.29 (Unix)

Steps

  • Open article administration
  • Use search tools

Test before patch

  • Single filter selection
    Single filter selection

Test after patch

  • Multi filter selection for category, access, author tag
    Multi filter selection

Tested @icampus


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

@ghost
Copy link

@ghost ghost commented Aug 23, 2017

RTC after two successful tests.

@mbabker mbabker added this to the Joomla 3.8.0 milestone Aug 23, 2017
@mbabker mbabker merged commit 8fe81e3 into joomla:staging Aug 23, 2017
5 checks passed
5 checks passed
@joomla-cms-bot
JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
@infograf768
Copy link
Member

@infograf768 infograf768 commented Aug 23, 2017

This has broken multilang frontend!

@infograf768
Copy link
Member

@infograf768 infograf768 commented Aug 23, 2017

Not only multilang. any menu item displaying a category does not display any article.
There are no articles in this category. If subcategories display on this page, they may contain articles.

@infograf768
Copy link
Member

@infograf768 infograf768 commented Aug 24, 2017

Issue:
#17694

@infograf768
Copy link
Member

@infograf768 infograf768 commented Aug 24, 2017

Patch here:
#17697 corrects this issue.

Please test.

@infograf768
Copy link
Member

@infograf768 infograf768 commented Sep 2, 2017

This contains a useless string
See #17835

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

8 participants