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

[New Feature] Implementing filtering admin modules per admin languages (including custom menu modules) #17215

Merged
merged 12 commits into from Jul 27, 2017

Conversation

infograf768
Copy link
Member

@infograf768 infograf768 commented Jul 23, 2017

Pull Request for Issue #17155

Summary of Changes

Allow administrator menu modules to be tagged and displayed per available administrator languages.
Added Warning when no menu modules are tagged to All languages and some administrator menu modules are missing for an administrator language in order to not suddenly lose all admin menu modules when switching backend language.
This warning will only display when the new Menus parameter option is set to yes.
Added a new adminfilter file for modules.

Testing Instructions

Patch staging.
Set the new parameter in Menus Options.

screen shot 2017-07-21 at 15 05 30

I have created 3 custom admin menus. French, English and one for ALL

screen shot 2017-07-21 at 15 07 12

I have assigned each of these menus to an admin menu module and tagged that module to an administrator language.

screen shot 2017-07-23 at 20 20 44

Tagging to a language is only possible for admin menu modules.
screen shot 2017-07-23 at 20 22 44

screen shot 2017-07-23 at 20 24 21

If there are no Published menu module tagged to ALL languages and some menu modules tagged to an administrator language are missing, we get a warning.
On my site I have 6 administrator languages and only 2 have a custom admin menu (French and English).

In the modules manager page
screen shot 2017-07-23 at 20 26 45

and also in the menus/menu items manager pages
screen shot 2017-07-23 at 20 28 12

As formerly, if any of the custom admin menus has a compulsory item missing , we get the Warning.
Both Warnings may be combined.
screen shot 2017-07-23 at 20 30 33

If the Menus Options Filtering parameter is set to No, the language field and columns are not displayed in the modules manager.

Notes

To be sure I get the Warning, I added the code to both menus and modules controller display() method. A better solution is welcome.

The purpose of this patch is to deal ONLY with administrator menu modules.
It can easily be upgraded to deal with all admin modules by modifying the conditional which is now set as a parameter in com_menus.
If set as a Global parameter, or Modules parameter, or system plugin in the future (or right now) the method JLanguageMultilang::isAdminEnabled() will just have to be modified.

Documentation Changes Required

Complete custom admin menus docs.

@izharaazmi
@RCheesley
All improvements welcome.

'position' => JText::_('COM_MODULES_HEADING_POSITION'),
'name' => JText::_('COM_MODULES_HEADING_MODULE'),
'a.access' => JText::_('JGRID_HEADING_ACCESS'),
'a.language' => JText::_('JGRID_HEADING_LANGUAGE'),
Copy link
Contributor

Choose a reason for hiding this comment

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

alignment

'position' => JText::_('COM_MODULES_HEADING_POSITION'),
'name' => JText::_('COM_MODULES_HEADING_MODULE'),
'a.access' => JText::_('JGRID_HEADING_ACCESS'),
'a.language' => JText::_('JGRID_HEADING_LANGUAGE'),
Copy link
Contributor

Choose a reason for hiding this comment

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

alignment

@@ -18,6 +18,9 @@ COM_MENUS_BATCH_OPTIONS="Batch process the selected menu items"
COM_MENUS_BATCH_TIP="If a menu or parent is selected for move/copy, any actions selected will be applied to the copied or moved menu items. Otherwise, all actions are applied to the selected menu items."
COM_MENUS_CHANGE_MENUITEM="Select or Change Menu Item"
COM_MENUS_CONFIGURATION="Menus: Options"
COM_MENUS_CUSTOM_ADMIN_MENU_LABEL="Custom Administrator Menus"
COM_MENUS_CUSTOM_ADMIN_LANG_FILTER_DESC="Let's filter custom administrator menus per language assigned to mod_menu modules."
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what you are intending to say here - do you really mean "let us..."

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant "allows filtering". will change.

@@ -18,6 +18,9 @@ COM_MENUS_BATCH_OPTIONS="Batch process the selected menu items"
COM_MENUS_BATCH_TIP="If a menu or parent is selected for move/copy, any actions selected will be applied to the copied or moved menu items. Otherwise, all actions are applied to the selected menu items."
COM_MENUS_CHANGE_MENUITEM="Select or Change Menu Item"
COM_MENUS_CONFIGURATION="Menus: Options"
COM_MENUS_CUSTOM_ADMIN_MENU_LABEL="Custom Administrator Menus"
Copy link
Contributor

Choose a reason for hiding this comment

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

Alpha order.

@infograf768
Copy link
Member Author

After some thought, I think we should create the parameter in the Modules Options and not the Menus options. In that case, we may not limit it to mod_menu and make it a totally new feature.

What do you think?


$totalModules = (int) $db->loadResult();

if ($totalModules != $adminLanguages)
Copy link
Contributor

Choose a reason for hiding this comment

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

@infograf768 Installed 3 languages - en, fr, de and having module counts - en = 1, fr = 0, de = 2 will pass this test but its not what we want. Is that correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you do not have any custom admin menu with module set to ALL, then we should change this conditional to
if ($totalModules < $adminLanguages)
it would solve the issue and display the warning I guess.
Can you confirm?

Copy link
Member Author

Choose a reason for hiding this comment

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

@izharaazmi
Can you confirm?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, no. This is indeed not enough
We need to improve this by fetching each admin language and make sure we have at least one module assigned to each.
Let's work on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think group by and IN (<language list>) should solve the issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

@izharaazmi
Without taking these out of the controllers for now, can you propose here a full snippet to replace the existing one? I am not good a groupby stuff ;)

we have now:

		// Check custom administrator menu modules
		if (JLanguageMultilang::isAdminEnabled())
		{
			// Check if we have any mod_menu module set to All languages
			$db = JFactory::getDbo();
			$query = $db->getQuery(true)
				->select('COUNT(*)')
				->from($db->qn('#__modules'))
				->where($db->qn('module') . ' = ' . $db->quote('mod_menu'))
				->where($db->qn('published') . ' = 1')
				->where($db->qn('client_id') . ' = 1')
				->where($db->qn('language') . ' = ' . $db->quote('*'));
			$db->setQuery($query);

			$modulesAll = (int) $db->loadResult();

			// If none, check that we have a mod_menu module for each admin language
			if ($modulesAll == 0)
			{
				$adminLanguages = count(JLanguageHelper::getInstalledLanguages(1));

				$db = JFactory::getDbo();
				$query = $db->getQuery(true)
					->select('COUNT(*)')
					->from($db->qn('#__modules'))
					->where($db->qn('module') . ' = ' . $db->quote('mod_menu'))
					->where($db->qn('published') . ' = 1')
					->where($db->qn('client_id') . ' = 1')
					->where($db->qn('language') . ' != ' . $db->quote('*'));
				$db->setQuery($query);

				$totalModules = (int) $db->loadResult();

				if ($totalModules != $adminLanguages)
				{
					$msg = JText::_('JMENU_MULTILANG_WARNING');
					JFactory::getApplication()->enqueueMessage($msg, 'warning');
				}
			}
		}

@@ -541,6 +541,15 @@ public function getForm($data = array(), $loadData = true)
if ($clientId == 1)
{
$form = $this->loadForm('com_modules.module.admin', 'moduleadmin', array('control' => 'jform', 'load_data' => $loadData), true);

// Display language field to filter admin custom menus per language
if ($module == 'mod_menu' && JLanguageMultilang::isAdminEnabled())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we allow language filtering for all modules both client? I thing you have done all the work already and we can quickly expand the scope here.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was my question above:
#17215 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right. That would be a great thing.

<?php elseif ($item->language == '*'):?>
<?php echo JText::alt('JALL', 'language'); ?>
<?php else:?>
<?php echo $this->escape($item->language); ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

IIIRC, we are displaying language flags somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should NOT display language flags because this is independant of the Content Languages and can be used on monolanguage sites where Content Languages are not necessary.
The language field is fetching administrator languages NOT content languages.

@@ -76,6 +76,47 @@ public function display($cachable = false, $urlparams = false)
// Load the submenu.
ModulesHelper::addSubmenu($this->input->get('view', 'modules'));

// Check custom administrator menu modules
if (JLanguageMultilang::isAdminEnabled())
Copy link
Contributor

Choose a reason for hiding this comment

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

I think its not good, not enough to test this condition in module manager and menu manager alone. This check should be something that occurs always. Would it be any good if we do this in postinstall messages?

Copy link
Member Author

Choose a reason for hiding this comment

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

Postinstall messages may show only once and then dismissed.
The ideal solution would be in the index.php of the admin template or, better, when we load the template if we want to the proper check all the time.

@izharaazmi
Copy link
Contributor

izharaazmi commented Jul 24, 2017

@infograf768 This should work for the check part.

if (JLanguageMultilang::isAdminEnabled())
{
	$languages = JLanguageHelper::getInstalledLanguages(1, true);
	$langCodes = array();

	foreach ($languages as $language)
	{
		$langCodes[$language->metadata['tag']] = $language->name;
	}

	$db    = JFactory::getDbo();
	$query = $db->getQuery(true);

	$query->select($db->qn('m.language'))
		->from($db->qn('#__modules', 'm'))
		->where($db->qn('m.module') . ' = ' . $db->quote('mod_menu'))
		->where($db->qn('m.published') . ' = 1')
		->where($db->qn('m.client_id') . ' = 1')
		->group($db->qn('m.language'));

	$mLanguages = $db->setQuery($query)->loadColumn();

	// Check if we have a mod_menu module set to All languages or a mod_menu module for each admin language.
	if (!in_array('*', $mLanguages) && count($langMissing = array_diff(array_keys($langCodes), $mLanguages)))
	{
		$app         = JFactory::getApplication();
		$langMissing = array_intersect_key($langCodes, array_flip($langMissing));

		$app->enqueueMessage(JText::sprintf('JMENU_MULTILANG_WARNING_MISSING_MODULES', implode(', ', $langMissing)), 'warning');
	}
}
JMENU_MULTILANG_WARNING_MISSING_MODULES="The administrator menu module for <strong>%s</strong> does not exist. Create a custom administrator menu and module for each administrator language or publish a menu module set to All languages."

@infograf768
Copy link
Member Author

@izharaazmi
Thanks. Committed after just changing name to nativeName.
This gives:
screen shot 2017-07-24 at 11 37 01

@infograf768
Copy link
Member Author

@mbabker @Bakual

Shall we set the parameter in the Modules Options instead of Menus Options?
In that case, shall we implement language filtering for all admin modules instead of only mod_menu?

@brianteeman
Copy link
Contributor

Language filtering for admin modules would be good. This has come up before as a feature request

@brianteeman
Copy link
Contributor

Just wondering if we even need an option. Would I r not be enough to check if the admin menu exists for the selected admin language

@RCheesley
Copy link

RCheesley commented Jul 24, 2017 via email

@mbabker mbabker added this to the Joomla 3.8.0 milestone Jul 24, 2017
@mbabker mbabker added this to Testing/Review in [3.8] General Jul 24, 2017
@infograf768
Copy link
Member Author

I will modify the PR to get the multilingual option for all modules.
Tomorrow or after tomorrow. Target 3.8 anyway . 😄

@izharaazmi
Copy link
Contributor

@infograf768 I'd rather try getting this into 3.8 first, and we can discuss these later 😄 👍

@izharaazmi
Copy link
Contributor

@infograf768 My tests are ok, just added few comments please reply to them.

@infograf768
Copy link
Member Author

@izharaazmi

the new method may find a use (once modified) for other stuff than modules.

Correct. Therefore this check should not be bound to com_modules params anyway, I guess?

"Should not be bound ONLY to..." .
This is why I wrote above "once modified":

I may have though that the new method may find a use (once modified) for other stuff than modules.

The code for example for specific enabled plugins or user groups or whatever would be "added" to the one checking the admin modules params.

if "something" || "somethingelse" || "anothercondition" || "the modules params"
{
     $enabled = true;
}

if judged necessary, we may then add a new method in JModuleHelper and use it in JLanguageMultilang::isAdminEnabled() to keep things in a handy place.

We could do that right now, but we are in a hurry to get this in 3.8.0
😃

If this is satisfying, please add test in issues.joomla.org

@infograf768
Copy link
Member Author

did not get these, weird.
Looking

@infograf768
Copy link
Member Author

Done, if I understood correctly

@izharaazmi
Copy link
Contributor

I have tested this item ✅ successfully on 31abf2d

Filters working for backend modules and new configuration parameter enables/disables filtering feature correctly.


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

@izharaazmi
Copy link
Contributor

I guess this counts as 2 tests as the changes were not affecting code. Right?

*
* @since 3.8.0
*/
public static function isAdminEnabled()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this method does not fit here. This check is specific to modules so this should be not in global CMS\Language\Multilanguage context.

Copy link
Member Author

Choose a reason for hiding this comment

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

already replied

*
* @return string
*
* @see JFormFieldAdminLanguage
Copy link
Contributor

Choose a reason for hiding this comment

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

Could not find JFormFieldAdminLanguage. Also could not find any usage of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

i already deleted that. was copy paste

/**
* Utility class working with administrator language select lists
*
* @since 1.6
Copy link
Contributor

Choose a reason for hiding this comment

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

You are adding this class in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we need a correct batch language method for admin modules, i.e. dealing with administrator languages and not content languages.

@izharaazmi
Copy link
Contributor

Is this RTC?

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 3.8.0 milestone Jul 27, 2017
@ghost
Copy link

ghost commented Jul 27, 2017

RTC after two successful tests.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 27, 2017
@wojsmol
Copy link
Contributor

wojsmol commented Jul 27, 2017

@infograf768 I see RTC but if enything is neadet plese ping me.

@mbabker mbabker added this to the Joomla 3.8.0 milestone Jul 27, 2017
@mbabker mbabker merged commit 1bcfb4a into joomla:staging Jul 27, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 27, 2017
@mbabker mbabker moved this from Testing/Review to Completed in [3.8] General Jul 27, 2017
@infograf768 infograf768 deleted the multilang_custom_adminmenus branch July 28, 2017 06:23
@infograf768
Copy link
Member Author

@izharaazmi @mbabker

After a good night sleep, I think @izharaazmi was right. 😃
If an admin language filtering may be necessary in the future for other stuff than the modules, we better move the conditional method into JModuleHelper.

This would mean adding a similar method for each element concerned in back-end with the possibility to enable or not such admin lang filtering by checking a specific parameter.

Admins will then be able to choose, element by element, what they want to be filtered or not.

Now that this PR is merged, it is quite easy to make a new PR changing that aspect of it.
It is not as urgent as the new feature.
Would you like me to prepare that now?

@izharaazmi
Copy link
Contributor

izharaazmi commented Jul 28, 2017 via email

@infograf768
Copy link
Member Author

LOL
Let's wait for @mbabker feedback

@mbabker
Copy link
Contributor

mbabker commented Jul 28, 2017

You guys will use this more often than me, so whatever makes sense to you roll with it.

@infograf768
Copy link
Member Author

@izharaazmi
you do or i do?

@izharaazmi
Copy link
Contributor

izharaazmi commented Jul 28, 2017 via email

@izharaazmi
Copy link
Contributor

See #17314

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Language Change This is for Translators
Projects
No open projects
[3.8] General
Completed
Development

Successfully merging this pull request may close these issues.

None yet

10 participants