? Success

User tests: Successful: Unsuccessful:

avatar infograf768
infograf768
13 Jan 2016

This should solve #8876
See there for tests instructions.

Basically, this PR first checks for multiple Home pages in the same menu before other checks.

Please
@andrepereiradasilva @dgt41 @fontanil

Please test. ?

avatar infograf768 infograf768 - open - 13 Jan 2016
avatar infograf768 infograf768 - change - 13 Jan 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 13 Jan 2016
Labels Added: ?
avatar infograf768 infograf768 - change - 13 Jan 2016
Category Administration Components Multilanguage
avatar andrepereiradasilva
andrepereiradasilva - comment - 13 Jan 2016

@infograf768 it does not unset the default home now, but the message "No menu item set to home." is still there and it shouldn't.

avatar andrepereiradasilva
andrepereiradasilva - comment - 13 Jan 2016

The message can also be improved "A menu should contain only one Default home."

Something like this?:
"You can't set this menu item as default home page for xxxxx language(s) because there is already a menu item assigned as default home page for xxxxxxx language(s) in this menu.
A menu can only have a menu item assigned as default homepage.
If you want to change the default home page for this menu to xxxxx language(s), unset the current default home page in this menu first."

avatar infograf768
infograf768 - comment - 13 Jan 2016

The message you should get is
"A menu should contain only one Default home."
and
"No menu item set to Home"
screen shot 2016-01-13 at 15 26 36
which is exactly what we want when using the icon.

avatar infograf768
infograf768 - comment - 13 Jan 2016

And whern editing such a menu item:

Warning

Save failed with the following error: A menu should contain only one Default home.

screen shot 2016-01-13 at 15 28 49

avatar andrepereiradasilva
andrepereiradasilva - comment - 13 Jan 2016

@infograf768 why "No menu item set to Home"?
There is a menu item set to home (in other language) in that menu... only not the one you tried to set.

avatar infograf768
infograf768 - comment - 14 Jan 2016

The Message is the result of the action, not a statement of the presence or not of a Home in the Menu.
It is independent from this patch as we get exactly the same message before the patch when trying to set a Home for another Content Language than the one which has already a Home in that Menu.
The PR just corrects the issue when trying to set a Home when the menu item is set to ALL languages and the existing Home is a Content Language.
Also, the explanation of the failure is in the Error/Warning.
The strings are and have been there since 1.6.

COM_MENUS_ITEMS_SET_HOME_0="No menu item set to home."
COM_MENUS_ITEMS_SET_HOME_1="1 menu item successfully set to home."
COM_MENUS_ITEMS_SET_HOME_MORE="%d menu items successfully set to home."
COM_MENUS_ITEMS_UNSET_HOME="1 menu item successfully unset to home."

from this code:
https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_menus/controllers/items.php#L141-L157

It is similar to other plural messages in J!.
For example:

COM_USERS_N_USERS_ACTIVATED_0="No user activated."
COM_CONTENT_N_ITEMS_CHECKED_IN_0="No article successfully checked in."

Although there are users activated or articles already checked in. Etc.

If you have another better idea for COM_MENUS_ITEMS_SET_HOME_0, please propose.
@brianteeman What do you think?

In any case, as the PR is OK, we can separate the discussion about the value of the string from this.
Please set the Test as successful.

avatar rdeutz rdeutz - test_item - 14 Jan 2016 - Tested successfully
avatar rdeutz
rdeutz - comment - 14 Jan 2016

I have tested this item :white_check_mark: successfully on 3cc2166

Confirmed the behaviour, patch works as expected.

What I don't like is that the check is in the store function, might be better in the check function. As developer I would expect when I call the check function that all logical problems are checked and an error in the store function is more something really serious like db connection lost. Might be nitpicking.


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

avatar andrepereiradasilva andrepereiradasilva - test_item - 14 Jan 2016 - Tested successfully
avatar andrepereiradasilva
andrepereiradasilva - comment - 14 Jan 2016

I have tested this item :white_check_mark: successfully on 3cc2166

@infograf768 true your right. This PR is ok. It corrects the problem. Sorry for hijacking it with the message thing.

Just to conclude the messages discussion. I know it's there from a long time, my point was: from usability point, a user receiving one RED and one GREEN message at the same time is like "what?????". The result of the action is ok (GREEN) or not (RED)?


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

avatar infograf768 infograf768 - change - 14 Jan 2016
Status Pending Ready to Commit
avatar infograf768
infograf768 - comment - 14 Jan 2016

RTC for 3.5.0. Thanks for testing!


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

avatar joomla-cms-bot joomla-cms-bot - change - 14 Jan 2016
Labels Added: ?
avatar fontanil fontanil - test_item - 14 Jan 2016 - Tested successfully
avatar fontanil
fontanil - comment - 14 Jan 2016

I have tested this item :white_check_mark: successfully on 3cc2166

Tested on today's staging: works perfectly. Thanks JM!


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

avatar infograf768
infograf768 - comment - 14 Jan 2016

@andrepereiradasilva
Concerning the colour of the Message. What could be done —in a separate PR— is this:

                if (count($cid) == 0)
                {
                    $this->setMessage(JText::plural($ntext, count($cid)), 'warning');
                }
                else
                {
                    $this->setMessage(JText::plural($ntext, count($cid)));
                }

We would get:

screen shot 2016-01-14 at 12 33 54

I guess it could be part of a more general PR where all similar instances would be modified.

avatar infograf768
infograf768 - comment - 14 Jan 2016

Or

                if (count($cid) == 0)
                {
                    $this->setMessage($model->getError());
                }
                else
                {
                    $this->setMessage(JText::plural($ntext, count($cid)));
                }

In order to only get the error.

avatar andrepereiradasilva
andrepereiradasilva - comment - 14 Jan 2016

+1 for second option! One error message is enough.

avatar infograf768
infograf768 - comment - 14 Jan 2016

Will try to prepare another PR covering all cases in core. Not urgent.

avatar andrepereiradasilva
andrepereiradasilva - comment - 14 Jan 2016

ok thanks @infograf768

avatar roland-d roland-d - change - 16 Jan 2016
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2016-01-16 08:12:33
Closed_By roland-d
avatar roland-d roland-d - close - 16 Jan 2016
avatar joomla-cms-bot joomla-cms-bot - close - 16 Jan 2016
avatar joomla-cms-bot joomla-cms-bot - change - 16 Jan 2016
Labels Removed: ?
avatar janet75
janet75 - comment - 5 Feb 2020

Can create a 'warning (simple in html like pop-up mice menu to notice in back admin panel)' when attempting in a multi-languages site to setting 'default' in a new Home page in a new menu 'before save (no close) it' with a specific language, avoiding below error(s) (did not studied code, yet)?


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

Add a Comment

Login with GitHub to post a comment