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

Collapse option not working for the the BO category tree #23625

Merged
merged 5 commits into from
Apr 27, 2021
Merged

Collapse option not working for the the BO category tree #23625

merged 5 commits into from
Apr 27, 2021

Conversation

crezzur
Copy link
Contributor

@crezzur crezzur commented Mar 12, 2021

Questions Answers
Branch? 1.7.7.x
Description? Resolve issue "collapse" option not working for the the BO category tree (old theme) Affected pages: Faceted search, other pages and modules which use the old theme tree version
Type? bug fix
Category? BO
BC breaks?
Deprecations?
Fixed ticket? Fixes #23558.
How to test? Displays an error when closing a (sub)category folder in the category tree.
Possible impacts? category tree in module Faceted search, other pages and modules which uses the old theme category tree.

This change is Reviewable

@crezzur crezzur requested a review from a team as a code owner March 12, 2021 15:07
@prestonBot
Copy link
Collaborator

Hello @crezzur!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@prestonBot prestonBot added the Bug fix Type: Bug fix label Mar 12, 2021
@prestonBot prestonBot added the 1.7.7.x Branch label Mar 12, 2021
@PierreRambaud PierreRambaud removed the 1.7.7.x Branch label Mar 12, 2021
@prestonBot prestonBot added the develop Branch label Mar 12, 2021
@PierreRambaud PierreRambaud changed the title Update tree.js Collapse option not working for the the BO category tree Mar 15, 2021
@NeOMakinG
Copy link

I asked a question only on one this, but every $(this) scope looks strange to me

NeOMakinG
NeOMakinG previously approved these changes Mar 15, 2021
Copy link

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

Tested it and every changed line was using wrong scoped reference, LGTM to me ;)

PierreRambaud
PierreRambaud previously approved these changes Mar 16, 2021
@PierreRambaud PierreRambaud added the Waiting for QA Status: action required, waiting for test feedback label Mar 16, 2021
@khouloudbelguith khouloudbelguith self-assigned this Mar 17, 2021
@khouloudbelguith
Copy link
Contributor

Hi @crezzur,

Thanks for your fix.
This PR fixes a regression in PS1.7.7, you need to target the 1.7.7.x branch.

Thank you!

@khouloudbelguith khouloudbelguith added the Waiting for author Status: action required, waiting for author feedback label Mar 17, 2021
@Progi1984
Copy link
Contributor

@crezzur No your are targeting develop branch and not the 1.7.7.x branch

@prestonBot prestonBot added the 1.7.7.x Branch label Mar 17, 2021
@Progi1984
Copy link
Contributor

@crezzur I think you misunderstood me : your PR targets the develop branch :
image

@NeOMakinG NeOMakinG changed the base branch from develop to 1.7.7.x March 17, 2021 14:36
@NeOMakinG NeOMakinG dismissed stale reviews from PierreRambaud and themself March 17, 2021 14:36

The base branch was changed.

@NeOMakinG NeOMakinG changed the base branch from 1.7.7.x to develop March 17, 2021 14:36
@NeOMakinG
Copy link

@crezzur I think you need to change the target branch to 1.7.7.x, and then rebase your branch on the 1.7.7.x branch, if the rebase is too hard, we can just open a new PR or cherry pick the commit on a new PR

@crezzur crezzur changed the base branch from develop to 1.7.7.x March 17, 2021 14:44
@NeOMakinG
Copy link

@NeOMakinG Did i do it correct now?

You just changed the base branch, you need to rebase it into 1.7.7.x now

@khouloudbelguith khouloudbelguith added Waiting for dev Status: action required, waiting for tech feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Mar 17, 2021
@crezzur crezzur changed the base branch from 1.7.7.x to develop March 17, 2021 15:18
@NeOMakinG NeOMakinG added Waiting for QA Status: action required, waiting for test feedback and removed Waiting for author Status: action required, waiting for author feedback Waiting for dev Status: action required, waiting for tech feedback labels Apr 26, 2021
Progi1984
Progi1984 previously approved these changes Apr 26, 2021
@khouloudbelguith
Copy link
Contributor

Hi @crezzur,

The first issue is fixed but the second problem still exists.
Steps to reproduce the issue:

  1. Go to BO > Shop Parameters > General page
  2. Enable Multistore context
  3. Go to BO > Advanced Parameters > Multistore page
  4. Click on "Add new shop"
  5. In the "Associated categories" section, click on Collapse All
  6. Click on Check All
  7. Click on Expand All
  8. Try to close a folder
  9. See error: Cannot be closed

I attached a screen record
https://drive.google.com/file/d/1XfrDGN6BgR_LmCS8ALnpO0fDeUEVR7VO/view?usp=sharing

Thank you!

@khouloudbelguith khouloudbelguith added Waiting for author Status: action required, waiting for author feedback Waiting for dev Status: action required, waiting for tech feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Apr 26, 2021
@crezzur
Copy link
Contributor Author

crezzur commented Apr 26, 2021

@khouloudbelguith
The error is showing instantly when you click on the expand all button. The reason it is displayed in the Add shop treeview only is because this treeview has the search option activated.

The error is caused by the function call organizeTree(); (tree.js line 218).
Looking into this function it looks like its a dead part of code.

inside the function organizeTree() a call to the function getCategoryById() is made. Looking into the function getCategoryById() the HTML attribute name="id_parent" is searched.

Searching into the whole folder STORE\BACKFOFFICE\themes\default\ and more specific STORE\BACKFOFFICE\themes\default\template\helpers\tree\ for the HTML attribute name="id_parent" returns negative.

concluding this i am guessing the functions organizeTree() and getCategoryById() are leftovers from something what is already removed. If you come to the same conclusion i will add a pull request to remove those functions.

@khouloudbelguith
Copy link
Contributor

Hi @crezzur,

Thanks for your feedback.
So this issue of creating a shop will be fixed in another PR?

Thank you!

The error is showing instantly when you click on the expand all button. The reason it is displayed in the Add shop treeview page only is because this treeview has the search option activated.

The error is caused by the function call organizeTree(); (tree.js line 218).
Looking into this function it looks like its a dead part of code.

inside the function organizeTree() a call to the function getCategoryById() is made. Looking into the function getCategoryById() the HTML attribute name="id_parent" is searched.

Searching into the whole folder STORE\BACKFOFFICE\themes\default\ and more specific STORE\BACKFOFFICE\themes\default\template\helpers\tree\ for the HTML attribute name="id_parent" returns negative.

concluding this i am guessing the functions organizeTree() and getCategoryById() are leftovers from something what is already removed.
@crezzur crezzur dismissed stale reviews from Progi1984, NeOMakinG, and sowbiba via 5814396 April 26, 2021 10:54
@crezzur
Copy link
Contributor Author

crezzur commented Apr 26, 2021

@khouloudbelguith added the changes to this pull request.

@sowbiba sowbiba added Waiting for QA Status: action required, waiting for test feedback and removed Waiting for author Status: action required, waiting for author feedback Waiting for dev Status: action required, waiting for tech feedback labels Apr 27, 2021
@khouloudbelguith
Copy link
Contributor

@khouloudbelguith khouloudbelguith added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Apr 27, 2021
@prestonBot
Copy link
Collaborator

QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge.

@khouloudbelguith khouloudbelguith added this to the 1.7.7.4 milestone Apr 27, 2021
@Progi1984 Progi1984 merged commit 8ec76bc into PrestaShop:1.7.7.x Apr 27, 2021
@prestonBot
Copy link
Collaborator

Contribution merged, congratulations!

Would you mind answering our quick 1-minute survey? We would love to hear about your experience so far, it will help us improve our process for the community involved, like you. ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.7.7.x Branch Bug fix Type: Bug fix QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Expand all" button not working in the back office
9 participants