[com_modules] Add page (menuitem) filter #9987

andrepereiradasilva · 2016-04-19T12:14:58Z

Pull Request for Improvement.

Summary of Changes

One filter, IMO, is missing in com_modules: the page filter.
I think this filter is very handy since it can show all modules for a particular menu item.

Testing Instructions

Apply patch
Go to Extensions -> Modules
Create custom modules witl "All", "No pages", "All except selected" and "Only selected" menu assigments
Now test the new filter
Test also the filter when adding "+Module" in articles
- in backend the filter should appear
- in frontend the filter shouldn't appear

Note: The filter query needs to be tested in postgresql too (it's a query with several subqueries).

Observations

There actually an inconsistency in admin modules list: the "Pages" column exist and shouldn't.
Note that inconsistency already exists because the column is already there.

So i guess a PR to correct that should also remove this filter from the admin modules list.

brianteeman · 2016-04-19T12:17:21Z

administrator/language/en-GB/en-GB.com_modules.ini

@@ -124,6 +124,7 @@ COM_MODULES_OPTION_POSITION_TEMPLATE_DEFINED="Template"
 COM_MODULES_OPTION_POSITION_USER_DEFINED="User"
 COM_MODULES_OPTION_SELECT_CLIENT="- Select Type -"
 COM_MODULES_OPTION_SELECT_MODULE="- Select Type -"
+COM_MODULES_OPTION_SELECT_PAGE="- Select Page -"


Need to check but dont we usually say Menu Item?

yes but in this case the column name is "Pages" that's why i used this.

Blast this is tricky. The title of the option is Menu Assignment and then the values are all Page xxx

yes - confusing indeed - I am happy to go with your suggestion for now

ok, so it can be tested as it is

brianteeman · 2016-04-19T12:39:50Z

Just as with position it is possible to set pages to ::none::
So there needs to be a way to select ::none:: from the filter

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9987.}

andrepereiradasilva · 2016-04-19T12:40:41Z

ok, you're right, makes sense. will check that.

andrepereiradasilva · 2016-04-19T12:57:41Z

ok done. We have an option to select the modules not assigned to any pages now too.

brianteeman · 2016-04-19T15:01:41Z

I have tested this item ✅ successfully on fe3b973

All good.

Just think it would be better if instead of "No pages" it would be the same as with positions and be ":: None ::"

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9987.}

andrepereiradasilva · 2016-04-19T15:06:00Z

Just think it would be better if instead of "No pages" it would be the same as with positions and be ":: None ::"

IMO that could be confusing. Because that is a term used in the positions.

We could use "None" (instead of "No pages") like in the column when you have a module not assigned to any menu if you prefer. Please tell me what you prefer, i can easily chnage it.

brianteeman · 2016-04-19T15:13:15Z

I will leave it to someone else to decide which option

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9987.}

andrepereiradasilva · 2016-04-19T15:15:28Z

@brianteeman BTW did you test on postgresql ?

brianteeman · 2016-04-19T15:16:36Z

Not yet - i forgot - testing that for you now

brianteeman · 2016-04-19T15:19:42Z

As far as I can see it works well in postgres

MDXBilal12 · 2016-04-19T16:16:52Z

I have tested this item ✅ successfully on fe3b973

All good.

Just think it would be better if instead of "No pages" it would be the same as with positions and be ":: None ::"

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9987.}

MDXBilal12 · 2016-04-19T16:18:36Z

All good.

Just think it would be better if instead of "No pages" it would be the same as with positions and be ":: None ::"

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9987.}

brianteeman · 2016-04-20T08:20:06Z

@andrepereiradasilva if you make the change to ::none:: I will set this RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9987.}

joomla-cms-bot · 2016-04-20T11:35:43Z

This PR has received new commits.

CC: @brianteeman, @MDXBilal12

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9987.}

andrepereiradasilva · 2016-04-20T11:36:31Z

@brianteeman done. See 28183f5

brianteeman · 2016-04-20T11:44:09Z

Not at my desk but looking at the code doesn't that produce ::::none::::
On 20 Apr 2016 12:36 pm, "andrepereiradasilva" notifications@github.com
wrote:

@brianteeman https://github.com/brianteeman done. See 28183f5
28183f5

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#9987 (comment)

andrepereiradasilva · 2016-04-20T11:58:30Z

look again

brianteeman · 2016-04-20T12:28:49Z

I have tested this item ✅ successfully on 28183f5

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9987.}

brianteeman · 2016-04-20T12:29:15Z

As I said I was checking from my phone ;) All good

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9987.}

brianteeman · 2016-04-20T12:29:41Z

Setting RTC as the only change since the last tests was a language string

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9987.}

brianteeman · 2016-05-24T09:07:58Z

I have tested this item ✅ successfully on 7e37a19

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9987.}

brianteeman · 2016-05-24T09:08:17Z

RTC - cool new feature

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9987.}

roland-d · 2016-05-24T10:09:20Z

administrator/components/com_modules/models/modules.php

+			// If user selected the modules not assigned to any page (menu item).
+			if ((int) $menuItemId === -1)
+			{
+				$query->having('MIN(mm.menuid) IS NULL');


The mm.menuid should be enclosed in a quoteName statement.

roland-d · 2016-05-24T10:13:07Z

Taking off RTC for now until code fixes are implemented.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9987.}

joomla-cms-bot · 2016-05-24T18:31:57Z

This PR has received new commits.

CC: @brianteeman, @grhcj, @MDXBilal12

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9987.}

andrepereiradasilva · 2016-05-24T18:32:36Z

done @roland-d

andrepereiradasilva · 2016-05-25T10:12:51Z

@roland-d can we have the RTc back on this one. Only did the changes you requested.

roland-d · 2016-05-25T11:42:47Z

@andrepereiradasilva I am having a conflict:

error: patch failed: layouts/joomla/searchtools/default/filters.php:11
error: layouts/joomla/searchtools/default/filters.php: patch does not apply

Would you mind checking if there is a conflict with current staging?

andrepereiradasilva · 2016-05-25T11:49:23Z

strange let me update the branch

joomla-cms-bot · 2016-05-25T11:49:27Z

This PR has received new commits.

CC: @brianteeman, @grhcj, @MDXBilal12

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9987.}

andrepereiradasilva · 2016-05-25T11:49:59Z

ok @roland-d try now.

roland-d · 2016-05-25T16:24:44Z

@andrepereiradasilva The fatal error is gone but I have a whitespace error now :)

<stdin>:108: trailing whitespace.
                                        (' . $subQuery1 . ') = 0
warning: 1 line adds whitespace errors.

In addition I have a failure in my test, see this screenshot:

The menu is called About Joomla and I think we should keep the same name rather than an internal name. What do you think?

joomla-cms-bot · 2016-05-25T17:05:04Z

This PR has received new commits.

CC: @brianteeman, @grhcj, @MDXBilal12

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9987.}

andrepereiradasilva · 2016-05-25T17:05:34Z

you're more efficient than travis! whitespace removed

andrepereiradasilva · 2016-05-25T17:07:49Z

@roland-d

The menu is called About Joomla and I think we should keep the same name rather than an internal name. What do you think?

I think so. But thats's all across joomla menu field selector (in all views that use it).
Nothing to do with my PR. That is for another PR.

andrepereiradasilva · 2016-05-25T17:08:29Z

I will make a PR to change that in all menu select fields

andrepereiradasilva · 2016-05-25T17:53:22Z

@roland-d new PR for that #10625 please test

brianteeman · 2016-05-25T17:59:31Z

If we do that everywhere then best to resolve that in a separate email

Bakual · 2016-05-25T18:27:35Z

This should be tested in other databases than MySQL (PostgreSQL or MSSQL). Because we may have to add more fields to the select statement due to the added having clause.
MySQL allows to do stuff here which PostgreSQL doesn't allow.

roland-d · 2016-05-25T18:37:47Z

Thanks everybody

andrepereiradasilva added 3 commits April 19, 2016 12:33

Add new language string to select the page

1190160

add the new page (menuitem) filter

7f0a535

Add the new filter to modules model

ae6b1fe

joomla-cms-bot added Language Change This is for Translators PR-staging labels Apr 19, 2016

brianteeman reviewed Apr 19, 2016
View reviewed changes

correct comment

829cf75

andrepereiradasilva added 2 commits April 19, 2016 13:56

add no pages option to xml

b59cadb

process "No pages" option in the modules model

fe3b973

Add ":: None ::" language variable

28183f5

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 20, 2016

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 24, 2016

roland-d reviewed May 24, 2016
View reviewed changes

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 24, 2016

quotes and cast as requested by @roland-d

c8eef45

Merge branch 'staging' into com_modules-menuitem-filter

35d4293

remove whitespace

eb6febd

andrepereiradasilva mentioned this pull request May 25, 2016

[menuitem field] Use the menu title instead of the menu type #10625

Merged

roland-d merged commit 5e52989 into joomla:staging May 25, 2016

andrepereiradasilva deleted the com_modules-menuitem-filter branch May 25, 2016 19:46

[com_modules] Add page (menuitem) filter #9987

[com_modules] Add page (menuitem) filter #9987

Conversation

andrepereiradasilva commented Apr 19, 2016 • edited by joomla-cms-bot

Summary of Changes

Testing Instructions

Observations

brianteeman Apr 19, 2016

Choose a reason for hiding this comment

andrepereiradasilva Apr 19, 2016

Choose a reason for hiding this comment

brianteeman Apr 19, 2016

Choose a reason for hiding this comment

brianteeman Apr 19, 2016

Choose a reason for hiding this comment

andrepereiradasilva Apr 19, 2016

Choose a reason for hiding this comment

brianteeman commented Apr 19, 2016

andrepereiradasilva commented Apr 19, 2016 • edited

andrepereiradasilva commented Apr 19, 2016

brianteeman commented Apr 19, 2016

andrepereiradasilva commented Apr 19, 2016

brianteeman commented Apr 19, 2016

andrepereiradasilva commented Apr 19, 2016

brianteeman commented Apr 19, 2016

brianteeman commented Apr 19, 2016

MDXBilal12 commented Apr 19, 2016

MDXBilal12 commented Apr 19, 2016

brianteeman commented Apr 20, 2016

joomla-cms-bot commented Apr 20, 2016

andrepereiradasilva commented Apr 20, 2016

brianteeman commented Apr 20, 2016

andrepereiradasilva commented Apr 20, 2016

brianteeman commented Apr 20, 2016

brianteeman commented Apr 20, 2016

brianteeman commented Apr 20, 2016

brianteeman commented May 24, 2016

brianteeman commented May 24, 2016

roland-d May 24, 2016

Choose a reason for hiding this comment

roland-d commented May 24, 2016

joomla-cms-bot commented May 24, 2016

andrepereiradasilva commented May 24, 2016

andrepereiradasilva commented May 25, 2016

roland-d commented May 25, 2016

andrepereiradasilva commented May 25, 2016

joomla-cms-bot commented May 25, 2016

andrepereiradasilva commented May 25, 2016

roland-d commented May 25, 2016

joomla-cms-bot commented May 25, 2016

andrepereiradasilva commented May 25, 2016

andrepereiradasilva commented May 25, 2016

andrepereiradasilva commented May 25, 2016

andrepereiradasilva commented May 25, 2016

brianteeman commented May 25, 2016 via email

Bakual commented May 25, 2016

roland-d commented May 25, 2016

andrepereiradasilva commented Apr 19, 2016 •

edited by joomla-cms-bot

andrepereiradasilva commented Apr 19, 2016 •

edited