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

Migrate Theme Catalog Page of Design Section #9154

Merged
merged 15 commits into from Jun 6, 2018

Conversation

Projects
None yet
8 participants
@michaelKaefer
Contributor

michaelKaefer commented May 31, 2018

Questions Answers
Branch? develop
Description? Migrate AdminThemesCatalogueController to ThemeController
Type? improvement
Category? CO
BC breaks? yes
Deprecations? no
Fixed ticket? -
How to test? Admin > Improve > Design > Themes Catalog

This change is Reviewable

michaelKaefer added some commits May 11, 2018

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented May 31, 2018

Hello @michaelKaefer,

sounds good to me.

About your git history, let me fix it for you. Time for review :)

@michaelKaefer

This comment has been minimized.

Contributor

michaelKaefer commented May 31, 2018

Hi @mickaelandrieu
I do not understand - but thank you anyway 😄

/**
* Displays themes from Addons under "Improve > Design > Themes Catalog"
*
* @author Michael Käfer <michael.kaefer1@gmx.at>

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

we usually don't add author name in classes ... but as you're the very first one community contributor, I'm in favor of keeping it.

WDYT @eternoendless ?

/**
* @var string The legacy controller name for routing.
*/
const CONTROLLER_NAME = 'AdminThemesCatalog';

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

This constant is not required anymore, sorry for that docs are outdated already.

*/
public function indexAction(Request $request)
{
$pageContent = file_get_contents('https://addons.prestashop.com/iframe/search-1.7.php?'.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

How about:

$context = $this->getContext();
$configuration = $this->get('prestashop.adapter.legacy.configuration');

$iframeURI = 'https://addons.prestashop.com/iframe/search-1.7.php' 
    . http_build_query([
        'psVersion' => $configuration->get('_PS_VERSION_'),
        'isoLang' => $context->language->iso_code,
        'isoCurrency' => $context->currency->iso_code,
        'isoCountry' => $this->getContext()->country->iso_code,
        'activity' => $configuration->getInt('PS_SHOP_ACTIVITY'),
        'parentUrl' => $request->getSchemeAndHttpHost(),
        'onlyThemes' => 1
    ])
  • with http_build_query you don't need to care about "&" and "?" anymore,
  • we have some helpers in 1.7 with Configuration like getInt() and getBoolean()
'level' => $this->authorizationLevel($this::CONTROLLER_NAME),
]);
}
}

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

please add an extra line at the end of the file.

@mickaelandrieu

some minor changes but it's already working good!

Thanks @michaelKaefer

'enableSidebar' => true,
'help_link' => $this->generateSidebarLink('AdminThemesCatalog'),
'requireFilterStatus' => false,
'level' => $this->authorizationLevel($this::CONTROLLER_NAME),

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

you can remove level property, you don't need it (and it should be removed from modules list iframe too, in a future contribution)

'parentUrl='.$request->getSchemeAndHttpHost().'&'.
'onlyThemes=1');
return $this->render('@PrestaShop/Admin/Improve/Design/Theme/addons_store.html.twig', [

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor
- @PrestaShop/Admin/Improve/Design/Theme/addons_store.html.twig
+ @PrestaShop/Admin/Improve/Design/ThemesCatalogPage/addons_store.html.twig

and move the template to the right location.

methods: [GET]
defaults:
_controller: 'PrestaShopBundle:Admin\Improve\Design\Theme:index'
_legacy_controller: AdminThemesCatalog

This comment has been minimized.

@mickaelandrieu

mickaelandrieu May 31, 2018

Contributor

also add an extra empty line at the end of the file

@@ -35,6 +35,10 @@ _admin_supplier_routing:
resource: "admin/routing_supplier.yml"
prefix: /supplier
_admin_theme_routing:

This comment has been minimized.

@mickaelandrieu

@mickaelandrieu mickaelandrieu changed the title from Migrate themecontroller to Migrate Theme Catalog Page of Design Section May 31, 2018

@michaelKaefer

This comment has been minimized.

Contributor

michaelKaefer commented May 31, 2018

Thank you @mickaelandrieu again! Made another commit :)

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented May 31, 2018

I do not understand - but thank you anyway smile

We shouldn't have these commits:

git_history

as they are not related to this contribution.

I'll clean up before merge :)

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented May 31, 2018

Dear @marionf, I want you to check this great contribution as soon as possible! 🎉

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented May 31, 2018

@michaelKaefer tbh I don't know for the @author notation, I don't see all of them in PrestaShop code...

This is something we need to discuss with @eternoendless and document :)

$context = $this->getContext();
$configuration = $this->get('prestashop.adapter.legacy.configuration');
$pageContent = file_get_contents('https://addons.prestashop.com/iframe/search-1.7.php'

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 1, 2018

Contributor

Can 1.7 be a parameter or something related to the current PS_VERSION? :)

This comment has been minimized.

@michaelKaefer

michaelKaefer Jun 1, 2018

Contributor

@PierreRambaud I don't know, just copied the URL from the old controller. I also would suppose it's the version and made another commit 👍

@marionf

This comment has been minimized.

Contributor

marionf commented Jun 1, 2018

Hello @michaelKaefer

I have a page not found when I click on Design > Themes Catalog because it's still the old link

capture du 2018-06-01 09-39-48

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 1, 2018

Hi! To update the menu link, what you need to do is to update the class Link https://github.com/PrestaShop/prestafony-project/blob/master/docs/migration-guide.md#routing-in-prestashop, this will use the Symfony router for the menu item.

Nice spotted @marionf :)

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 4, 2018

@marionf ok I can reproduce it, I was on Module catalog page and not on Theme Catalog page 👍

@michaelKaefer

This comment has been minimized.

Contributor

michaelKaefer commented Jun 4, 2018

Thank you @marionf ! I will try on friday.

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 4, 2018

@michaelKaefer you can't change the design of the page as the content comes from an iframe.

For me, your work is done here, and we need to update the iframe from our side.

What do you think @marionf ?

@marionf marionf added QA ✔️ and removed waiting for QA labels Jun 5, 2018

@marionf

This comment has been minimized.

Contributor

marionf commented Jun 5, 2018

It's ok for me :)

@mickaelandrieu mickaelandrieu added this to the 1.7.5.0 milestone Jun 6, 2018

@@ -743,21 +743,14 @@ public function getAdminLink($controller, $withToken = true, $sfRouteParams = ar
'AdminModulesManage' => 'admin_module_manage',
'AdminModulesNotifications' => 'admin_module_notification',
'AdminModulesSf' => 'admin_module_manage',
'AdminCustomerPreferences' => 'admin_customer_preferences',

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 6, 2018

Contributor

Duplicate line 737

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Jun 6, 2018

Contributor

not anymore :)

This comment has been minimized.

@PierreRambaud

PierreRambaud Jun 6, 2018

Contributor

Lines were sorted and you broke it 😿

fixed, thanks !

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 6, 2018

Thanks @michaelKaefer, do you want to share your feelings about this first migration?

We'd like to onboard more people like you, so your feedback is really important to us.

Also, this is only the start of the Open Source journey for you if you want to discover PrestaShop 1.7, let me know and I can present you some tasks I'm working on right now for 1.7.5.

Also, if you have ideas of improvements, they are very welcome!

Thanks for your time and your commitment, you're awesome :-)

@mickaelandrieu mickaelandrieu merged commit 5bca435 into PrestaShop:develop Jun 6, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kpodemski

This comment has been minimized.

Contributor

kpodemski commented Jun 6, 2018

Well done @michaelKaefer

and i'm glad to see that team is willing to accept this kind of PR here, me and many other developers were completely ignored many times in the past after submitting more complex PRs

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 6, 2018

@kpodemski time for you to contribute again maybe? If you want to try to migrate a page to learn how PrestaShop 1.7 back office work under the hood, I'll help you 👍

@kpodemski

This comment has been minimized.

Contributor

kpodemski commented Jun 6, 2018

@mickaelandrieu yes, i'm back at it :) this is why i'm digging through all recent PRs to see what is going on currently :)

@michaelKaefer

This comment has been minimized.

Contributor

michaelKaefer commented Jun 6, 2018

@kpodemski I'm new to contributing to PS. But in this short period quite some people where really helpful, especially @mickaelandrieu 👍

@mickaelandrieu My feelings: an easy controller to migrate, you have choosen a perfect start for somebody new. I really like the decision to rely on Symfony concerning all the not-webshop stuff and it's cool to contribute to open source - even if it's a small part. So, thank you for your effort and patience, I really appreciate it :)

@mickaelandrieu Yeah, I'm ready for tasks, do you plan to maintain a list or something where we can choose?

@mickaelandrieu

This comment has been minimized.

Contributor

mickaelandrieu commented Jun 6, 2018

There is one roadmap here => PrestaShop/prestafony-project#10 this is what we'd like to do concerning the migration of back office (our top priority, for both us and the community).

What we can do now is working on a harder controller together, what do you think? Take a look at the list and let me know if something sounds interesting for you ;) good night !

@mickaelandrieu mickaelandrieu referenced this pull request Jun 26, 2018

Closed

Roadmap migration (1.7.5) #10

26 of 40 tasks complete

@marionf marionf removed their assignment Aug 10, 2018

@mickaelandrieu mickaelandrieu referenced this pull request Sep 5, 2018

Closed

Symfony migration roadmap for 1.7.5 #10301

27 of 40 tasks complete

@matks matks added the migration label Sep 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment