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

[4.0] Fixing children when changing menu of menu item #25205

Merged
merged 4 commits into from
Jun 23, 2019

Conversation

Hackwar
Copy link
Member

@Hackwar Hackwar commented Jun 14, 2019

Pull Request for Issue #7579.

Summary of Changes

When a menu item has child-menu items and its menu is being changed, the children should change their menu as well. Right now this is not the case.

Testing Instructions

  1. Be sure that you have 2 menus.
  2. Create a menu item in one menu and another item in the same menu as a child of the first.
  3. Edit the first menu item and change the menu that this item belongs to.

Expected result

The menu item and all its children are changed to the new menu and remain in their former structure.

Actual result

The first menu item is changed over, the children stay in the original menu.

This fixes a really old issue (#7579), so please help out to test this and fix this long term issue.

@ghost
Copy link

ghost commented Jun 15, 2019

@RoterNagel @justinherrin please test.

@richard67
Copy link
Member

I have tested this item ✅ successfully on f53c3a9


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

@uglyeoin
Copy link
Contributor

Once I get a J4 install up and running I will test this

@uglyeoin
Copy link
Contributor

uglyeoin commented Jun 18, 2019

I have tested this and I have some questions.

Scenario:

Menu 1.
Parent 1

  • Child 1
    -- Child 3

Menu 2
Parent 2

Batch move Child 1 to Parent 2. Works.
Go into Child 1 and move to Parent 1 using sidebar menu (not batch). Leaves child 2 behind but they are still part of parent 2.
Move Child 1 from Parent 1 BACK to Parent 2. Child 2 now becomes a child of Child 1 again.

This is confusing and I'm not sure it's the right action. Can someone else please sanity check my work?

@richard67
Copy link
Member

@uglyeoin

Can someone else please sanity check my work?

Well, first of all your menu strtucture show "Child 3" and not "Child 2", but in the text below you write about "child 2" and not "child 3" => inconsistent => Gives a bad impression about sanity and carefulness.

@richard67
Copy link
Member

@uglyeoin Beside the inconsistency mentioned above, it seems you misinterpreted the purpose of this PR.

You write something about "Batch copy .. works". But that is not subject of this PR.

This PR deals with moving a menu item and its children from one menu to another by editing the menu item properties and there changing the menu to which the menu item belongs to.

It does not deal with changing the parent menu item for that menu item, neither in batch nor in editing menu items.

@Hackwar Correct me if I'm wrong.

@uglyeoin
Copy link
Contributor

Sorry my bad, I haven't added the patch. Rushing too much. I will try again

@richard67
Copy link
Member

No problem.

@richard67
Copy link
Member

richard67 commented Jun 18, 2019

@uglyeoin If you want to test on current 4.0-dev branch and so pulled the latest changes, made a composer install and an npm install, and then want to start to set up the site and see only an empty page: Is a know issue, PR is in progress: See #25261 . Edit: PR #25261 is merged, so after pulling latest changes and running composer install and npm install , installation should work.

@richard67
Copy link
Member

richard67 commented Jun 18, 2019

I have tested this item ✅ successfully on 58146b6

Tested again after recent commit. Still works.

Easy to test with Patchtester Component and with Blog Sample Data installed.

With the PR applied, changed menu for "Working on Your Site" from "Author Menu" to "Main Menu Blog" in the menu item edit view and then "Save & Close".

Result: Menu items for "Main Menu Blog" are shown in the menu items list. The menu item and the 2 children "Site Settings" and "Template Settings" have been moved to that menu.

(Without the PR, the child items were left alone and not moved with the parent).

Note that there is the same issue when in the menu item edit view changing the parent menu item of a menu item which has children. This is not subject of this PR here but should be corrected with another, future PR.


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

@alikon
Copy link
Contributor

alikon commented Jun 18, 2019

I have tested this item ✅ successfully on 58146b6


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

@ghost
Copy link

ghost commented Jun 18, 2019

Status "Ready To Commit".

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 18, 2019
@richard67
Copy link
Member

richard67 commented Jun 18, 2019

@Hackwar Question: Should the lft and rgt values also be changed when moving a menu item with children from one to the other menu like with this PR? And when moving a menu item with children from 1 to another parent menu item as I wrote above to be done for another PR? Or will lft and rgt be updated when saving the changed menu item? I hope that will be the case.

@Hackwar
Copy link
Member Author

Hackwar commented Jun 18, 2019

No, it doesn't have to change the lft and rgt values. Further issues should be fixed in other PRs.

@richard67
Copy link
Member

@Hackwar Ok. I will try to make a PR for the parent menu item changing thing in the same way as you did this here for the menu changing.

@richard67
Copy link
Member

@uglyeoin I can confirm your finding, also with this PR aplied. But I think it is a separate issue. I had it with and also without this PR applied. Do you want to open a new issue for it?

@uglyeoin
Copy link
Contributor

@richard67 that's weird. But a bit lucky that I discovered it. I have created a new issue here #25270

@roland-d roland-d added this to the Joomla 4.0 milestone Jun 23, 2019
@roland-d roland-d merged commit 6e9eefd into joomla:4.0-dev Jun 23, 2019
@roland-d
Copy link
Contributor

Thank you.

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 23, 2019
@Hackwar Hackwar deleted the patch-8 branch October 23, 2020 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants