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

Manage edit/view link for customers in HelperList #14721

Merged

Conversation

@jolelievre
Copy link
Contributor

commented Jul 17, 2019

Questions Answers
Branch? 1.7.6.x
Description? Manage edit/view link for customers in HelperList
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #14672
How to test? See issue, test customer links Perform other researches (on products, orders, ...) to be sure nothing is broken.

This change is Reviewable

@jolelievre jolelievre requested a review from PrestaShop/prestashop-core-developers as a code owner Jul 17, 2019
@jolelievre jolelievre added this to the 1.7.6.1 milestone Jul 17, 2019
protected function getViewLink($token, $id)
{
switch ($this->table) {
case 'customer':

This comment has been minimized.

Copy link
@jolelievre

jolelievre Jul 17, 2019

Author Contributor

Ok clearly it's a quick fix, the thing is do I extend this behavior to other migrated entities as well?

This comment has been minimized.

Copy link
@matks

matks Jul 17, 2019

Contributor

😅
Yeah, but this violates the O of SOLID.

This comment has been minimized.

Copy link
@jolelievre

jolelievre Jul 18, 2019

Author Contributor

do you really want me to change this? ^^
I could try and perform a generic change but I'm afraid of the side effects..

This comment has been minimized.

Copy link
@matks

matks Jul 18, 2019

Contributor

Since the issue is a trivial one, I'm not sure the cost is worth it :s

This comment has been minimized.

Copy link
@jolelievre

jolelievre Jul 19, 2019

Author Contributor

sadly I agree.. it's just that we will have to add other switch cases as we continue migrating pages
by the way do you think there are other pages that would need to be managed? based on the contents managed in the quick search:
product, customer, order, invoices, cart, modules
There might be a bug on the module links, but I don't know what was the expected result?

This comment has been minimized.

Copy link
@matks

matks Jul 19, 2019

Contributor

The more we dig, the worse it gets 😄

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 5, 2019

Member

This is hard to fix for several reasons:

  • If we use getAdminLink() then it's a breaking change (in this class' contract, links are meant to be built following a specific schema)
  • If we don't, then the component will be more and more broken as we migrate pages
  • We cannot inject dependencies because it would be a BC

Basically, it's broken by design, but we cannot let it stay broken.

My suggestion:

  1. Extract these two methods into an interface.
  2. Create at least two implementations, one for the previous link generation, one for the new one.
  3. Use a factory to build the correct implementation according to parameters.
  4. It needs to be covered by tests
  5. It must support all the migrated CRUD pages.
Copy link
Contributor

left a comment

LGTM

@marionf

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

Is it ready for QA @jolelievre ?

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

@matks Wasn't in favor of making this change :/

@jolelievre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

@PierreRambaud I know when you read his comment, but I actually wasn't sure if it was a troll or not ^^
The thing is this helper is used in various places (including modules), juste in the search page it is used for 7 different entities/tables, but the way it generates the links is not good it concatenates various strings to build the url but it should use getAdminLink

Now to minimize the side effects of my modification, I only managed the proper generation for customer table, and I left the other tables unchanged. So I have no doubts that we will have other issues with other tables as they get migrated (we may even have some already) and we will have to update this class later.

That's why I asked him if it was worth changing it for all other cases which he responded with Since the issue is a trivial one, I'm not sure the cost is worth it :s So, I might misunderstand him but I think we're good to go.

Maybe what we can do @marionf is at least check if all the results form the search page have correct links (products, customers, orders, invoices, carts, IPs and modules)?

@marionf

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

Maybe what we can do @marionf is at least check if all the results form the search page have correct links (products, customers, orders, invoices, carts, IPs and modules) ?

Yes, we can do that, so let's go for QA @khouloudbelguith

@khouloudbelguith

This comment has been minimized.

@PierreRambaud

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

@PierreRambaud I know when you read his comment, but I actually wasn't sure if it was a troll or not ^^

He wasn't ^^'

@matks

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

That's why I asked him if it was worth changing it for all other cases which he responded with Since the issue is a trivial one, I'm not sure the cost is worth it :s So, I might misunderstand him but I think we're good to go.

Hi there ! Sorry, I can see that indeed my comment was ambiguous 😓.

What I meant was this "the issue that this PR fixes has been labeled 'minor' and this PR uses a very dirty fix that is going to decrease the quality/reliability/maintainability of the codebase if we merge it*, so is this cost (the decrease in quality/reliability/maintainability) not too expensive to fix a minor issue ?"

I would rather be in favor of leaving it un-fixed in 1.7.6.1 as I think it is not used a lot, and perform an heavier refactoring work on the HelperList class in 1.7.7.0 in order to allow it to use getAdminLink() for all BO links, which will make it fully compliant with BO URL migration.

Doing this refactoring now, for 1.7.6.1 looks overkill to me because it might introduce massive regressions in a patch version. But for 1.7.7.0 it makes sense, and we could use unit and integration tests to cover ourselves.

And only merging this dirty fix for 1.7.6.1 does not look worth it to me because:

  • it's a quick dirty fix, so merging it will bring all the bad consequences we all know
  • as @jolelievre stated, similar issues might be already there or will pop because this can happen for any BO link built by HelperList and targeting a migrated BO page

But since I'm supposed to be off right now I will let you decide whether or not to merge this PR in 1.7.6.x branch 😉

*For only a bit, yeah, but bit by bit this is how a codebase rots

@jolelievre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

I totally agree with the refacto of this class for 177! And about the fact that the fix is ugly 😅
However if we create the issue for the refacto right now and make sure to do it for 177, I don't see why we could not merge this PR for 1761, I prefer this (knowing that we will do better rather soon) than leaving a know issue on purpose.. The ufgly fiw won't stay for that long in the codebase anyway What do you think?

@@ -269,6 +269,8 @@ public function displayListContent()
}
}
$this->_list[$index]['onclick'] = in_array('view', $this->actions) ? $this->getViewLink($this->token, $id) : $this->getEditLink($this->token, $id);

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 5, 2019

Member

Shouldn't this be link instead of onclick?

This comment has been minimized.

Copy link
@jolelievre

jolelievre Aug 6, 2019

Author Contributor

This is linked to the modif I made in the template where the "onclick" url was generated.., I wanted to keep in mind the use of this variable

https://github.com/PrestaShop/PrestaShop/pull/14721/files#diff-a3eeb66715b0f2f29851fb427bf72e14R52

But I can rename it with link and adapt the template accordingly if you prefer?

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 7, 2019

Member

I think it would be best since it's semantically wrong. Its value is not something that you'd put in onclick directly, it's an URL.

@matthieu-rolland matthieu-rolland dismissed their stale review Aug 6, 2019

new suggestions / change requests have been added

@marionf marionf removed the QA ✔️ label Aug 6, 2019
@jolelievre jolelievre force-pushed the jolelievre:fix-customer-helper-list branch from 2e8e9f5 to c9bf0e6 Aug 6, 2019
@jolelievre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@eternoendless I created the interface we talked about with two implementations, one based on Link::getAdminLink and the other one on concatenation as in the "legacy way"
There is room for improvement of course, but I thought for a patch it's enough Let me know if you think otherwise.

@matks your feedback is more than welcome ;)

Copy link
Member

left a comment

Changes requested

public function __construct()
{
$this->base_folder = 'helpers/list/';
$this->base_tpl = 'list.tpl';
$adminLinkBuilder = new AdminLinkBuilder(Context::getContext()->link, [

This comment has been minimized.

Copy link
@jolelievre

jolelievre Aug 8, 2019

Author Contributor

@eternoendless I addressed your feedback
I was wondering about this initialization, maybe it would be handy to have this info about migrated controllers somewhere in a configuration? To centralize this information which might be used somewhere else, or in a future service definition

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 8, 2019

Member

It could be a class constant.

This comment has been minimized.

Copy link
@jolelievre

jolelievre Aug 8, 2019

Author Contributor

Yes, but I wonder which would be the best candidate to store this const? Should we use AdminLinkBuilder for now? or do you think of a more generic/transversal class?

This comment has been minimized.

Copy link
@eternoendless

eternoendless Aug 8, 2019

Member

I think we can leave it as is for now.

@eternoendless

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Thank you @jolelievre

@eternoendless eternoendless merged commit ef8e018 into PrestaShop:1.7.6.x Aug 9, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jolelievre jolelievre deleted the jolelievre:fix-customer-helper-list branch Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.