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

Fix index.php links in admin zone #35231

Merged
merged 1 commit into from Feb 8, 2024
Merged

Conversation

Hlavtox
Copy link
Contributor

@Hlavtox Hlavtox commented Jan 31, 2024

Questions Answers
Branch? develop
Description? Simmilar to #35167, I migrated all usages of hardcoded index.php in admin zone to getAdminLink method. I needed to fixed some things along the way. There are million places with hardcoded parameters behind the calls still, but that would be next step sometime.
Type? refacto
Category? CO
BC breaks? no
Deprecations? no
How to test? See below
UI Tests https://github.com/Hlavtox/ga.tests.ui.pr/actions/runs/7812733796
Fixed issue or discussion?
Related PRs
Sponsor company

How to test

  • Check you can upload carrier image in BO.
  • Check that links to customer and orders in customer threads page in BO work correctly.
  • Check that if you go configure any module, the Translate button in the header works properly.
  • Check that search in the BO on the top works correctly.

@Hlavtox Hlavtox requested a review from a team as a code owner January 31, 2024 13:05
@prestonBot prestonBot added develop Branch Refactoring Type: Refactoring labels Jan 31, 2024
@Hlavtox Hlavtox added this to the 9.0.0 milestone Jan 31, 2024
@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 1, 2024

@Hlavtox Hlavtox added the Waiting for QA Status: action required, waiting for test feedback label Feb 1, 2024
Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

@Hlavtox I think "Fix index.php links in admin zone" is not an accurate description of this PR content 😉

matks
matks previously requested changes Feb 2, 2024
Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

@Hlavtox I think this PR needs to be tested by a QA analyst. And there is too much different things in this PR 🤔 there's refactoring, link changes, variables removal... I think it will be hard for a QA analyst to verify all of this.

I suggest to split the PR in smaller parts, each of them with a clear focus / goal and a clear "How to test". You are modifying a lot of legacy code, that is fragile and prone to errors. Please rely on QA team skills to verify whether or not you have broken something.

@ps-jarvis ps-jarvis added the Waiting for author Status: action required, waiting for author feedback label Feb 2, 2024
@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 6, 2024

@matks I don't know what I should do now, why did you put a block? 😁 I can just edit the PR table and let QA test it ;)

@matks
Copy link
Contributor

matks commented Feb 6, 2024

@Hlavtox I put a stop for the "I tested all sketchy things manually, green tests should be sufficient. :-)" in "How to test" 😉

Yes please modify it and request the help of QA team to make sure nothing is broken.

But I'm still worried: there's refactoring, link changes, variables removal... I think it will be hard for a QA analyst to verify all of this. A lot of things are mixed together.

@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 7, 2024

@matks I changed the PR table to account for it.

Regarding the changes, it's very simple:

  • admin-dev/init.php is used only by the filemanager. In that file, there was a hardcoded link. When I changed it to proper URL generated, it didn't work, because admin-dev/filemanager/config/config.php had it's own assignment of PS_ADMIN_DIR, which was different from the admin one. Because of that, the method was generating links like https://localhost/dev/../index.php or something like that.
  • classes/helper/HelperList.php - currentIndex can never be AdminProducts since it's now removed
  • trad_link is not needed, because we can determine if there are any translation links by checking for emptiness of $translateLinks array.

@Hlavtox Hlavtox removed the Waiting for author Status: action required, waiting for author feedback label Feb 7, 2024
@florine2623 florine2623 self-assigned this Feb 7, 2024
Copy link
Contributor

@florine2623 florine2623 left a comment

Choose a reason for hiding this comment

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

Hello @Hlavtox ,

Tested the following :

  • Check you can upload carrier image in BO ✅ tested in Creation and Edition of carrier, in FO carrier is well displayed
  • Check that links to customer and orders in customer threads page in BO work correctly ✅ check links in Threads and in Orders and messages timeline
  • Check that if you go configure any module, the Translate button in the header works properly ✅ Translate button redirects to the right translation page, translation is well saved
  • Check that search in the BO on the top works correctly ✅ modules, customer, orders, feature.

LGTM

@florine2623 florine2623 added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Feb 8, 2024
@Hlavtox Hlavtox merged commit 69bcb95 into PrestaShop:develop Feb 8, 2024
23 checks passed
@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 8, 2024

Thank you @florine2623! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
develop Branch QA ✔️ Status: check done, code approved Refactoring Type: Refactoring
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

8 participants