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

[FEAT] Redirect single search result to product page #4697

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Mar 14, 2025

Description (*)

When product search has only one result, redirect to product page.

  • new config options added (enabled by default)

Manual testing scenarios (*)

  1. use sample data
  2. search for "wonderland"
  3. should redirect to a books product page

@github-actions github-actions bot added Component: Catalog Relates to Mage_Catalog translations Relates to app/locale labels Mar 14, 2025
@github-actions github-actions bot added the Component: Core Relates to Mage_Core label Mar 14, 2025
@sreichel sreichel marked this pull request as draft March 14, 2025 15:43
@sreichel sreichel marked this pull request as ready for review March 14, 2025 16:08
@sreichel sreichel marked this pull request as draft March 14, 2025 16:09
@sreichel sreichel marked this pull request as ready for review March 14, 2025 16:12
@github-actions github-actions bot removed the Component: Core Relates to Mage_Core label Mar 16, 2025
@fballiano
Copy link
Contributor

this last version is a copy of MahoCommerce/maho#131 again without crediting the source

@colinmollenhour @kiatng @Hanmac

@sreichel
Copy link
Contributor Author

sreichel commented Mar 17, 2025

I have waited for it ...

@fballiano do you think i am not able to put that into core myself? I took some year old extensions and added it ... w/o moving observer-stuff to core.

Did it came to your mind, to add that functionality at all? Nope. But it tooks you only a few hours to merge that - as every OM PR.

Instead of making noise again, better think about to contribute back in some way. Leave a thanks - a PR - , "i have some improvements". That would be nice.

(PS: i dont want to hear you cry when i port all of the no-prototype changes back - o/c /w leaving credits)


Fab, you cannot answer here (to my issues/PRs) anymore. I have finally blocked you - same you did to me long time ago. Please head up to discussions if there is something to tell.

@sreichel
Copy link
Contributor Author

Fab,

You'd better made a suggestion before merge, but ....

Copy link
Contributor

@Hanmac Hanmac left a comment

Choose a reason for hiding this comment

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

Add @copyright Maho

@justinbeaty
Copy link
Contributor

For the lazy, here's code from each PR:

Maho:

// Redirect to product if there's only one result
if (Mage::getStoreConfigFlag('catalog/search/redirect_to_product_if_one_result')) {
    $searchResultBlock = Mage::app()->getLayout()->getBlock('search_result_list');
    if ($searchResultBlock) {
        /** @var Mage_CatalogSearch_Model_Resource_Fulltext_Collection $productCollection */
        $productCollection = $searchResultBlock->getLoadedProductCollection();
        if ($productCollection && $productCollection->getSize() === 1) {
            $product = $productCollection->getFirstItem();
            $this->_redirectUrl($product->getProductUrl());
        }
    }
}

OM:

// Redirect to product if there's only one result
if ($helper->isRedirectSingleSearchResult()) {
    $searchResultBlock = $this->getLayout()->getBlock('search_result_list');
    if ($searchResultBlock) {
        /** @var Mage_CatalogSearch_Model_Resource_Fulltext_Collection $productCollection */
        $productCollection = $searchResultBlock->getLoadedProductCollection();
        if ($productCollection && $productCollection->getSize() === 1) {
            $product = $productCollection->getFirstItem();
            $this->_redirectUrl($product->getProductUrl());
        }
    }
}

@sreichel
Copy link
Contributor Author

sreichel commented Mar 17, 2025

You took my code from this unreleased PR and opened MahoCommerce/maho#131 two days later. Only difference you added the observer code directly to the controller.

No link to this PR, but adding your copyright?

I'll rename the variables back later.

@Hanmac
Copy link
Contributor

Hanmac commented Mar 17, 2025

I tried to extract some timestamp data from the github commits to have that in perspective:

  • The first OM Commit is from "2025-03-14T13:53:22.000Z" f7e70cc
  • Maho commit is from "2025-03-16T10:37:57.000Z" MahoCommerce/maho@d65d8c7
  • and the "Copied" OM commit is from "2025-03-16T17:29:50.000Z" d789e80

I would say yes, @fballiano used parts of the code to make his own PR (without linking the original PR, like they usally do)
But i would also say yes, that @sreichel copied the block of @fballiano

@justinbeaty
Copy link
Contributor

I tried to extract some timestamp data from the github commits to have that in perspective [...]

Since I don't believe @fballiano can respond here, I will add a few more details:

The original Maho commit did indeed have the link to this PR as Fabrizio always does.

Then, Fabrizio and I discussed the introduction of the new observer interface and decided against it, so he rewrote the feature.

We already have (c) OpenMage in our headers, and Fabrizio ports the copyright update commits. So even if Maho copies code verbatim from OM, there is no license infringement. The reverse is not true because @sreichel is against including (c) Maho.

@Hanmac
Copy link
Contributor

Hanmac commented Mar 17, 2025

The original Maho commit did indeed have the link to this PR as Fabrizio always does.

but isn't this the wrong issue? commit links to #4668 to disable it, not to add the Result redirect?

@justinbeaty
Copy link
Contributor

The original Maho commit did indeed have the link to this PR as Fabrizio always does.

but isn't this the wrong issue? commit links to #4668 to disable it, not to add the Result redirect?

You're right, I was mixed up because that was the commit that first introduced the observer->execute method.

However, my last point still stands, no?

@Hanmac
Copy link
Contributor

Hanmac commented Mar 17, 2025

However, my last point still stands, no?

In my opinion, it should be discussed how much own work justifies an extra @copyright

when i was comparing this f7e70cc with that MahoCommerce/maho@d65d8c7

the similarity was 79% (if changed the variable names, 83%)

But that's not something I want to decide.
There isn't much that can be different with that few lines of code

@sreichel
Copy link
Contributor Author

the similarity was 79% (if changed the variable names, 83%)

@justinbeaty @empiricompany who copied who?

@sreichel
Copy link
Contributor Author

Add @copyright Maho

Nope. Not for renaming some vars and move code from A to B. In german we have something like "Wertschöpfungstiefe" ...

@sreichel sreichel requested a review from Hanmac March 18, 2025 01:48
@empiricompany
Copy link
Contributor

Add @copyright Maho
and reverting fab0bfe to better variable names

Copy link
Member

@colinmollenhour colinmollenhour left a comment

Choose a reason for hiding this comment

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

Holding due to concerns regarding the potential need to include Maho copyright notice/credit. This is not a decision or indication of opinion, just putting it on hold while it is discussed internally.

@OpenMage OpenMage locked as too heated and limited conversation to collaborators Mar 18, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Component: Catalog Relates to Mage_Catalog Component: CatalogSearch Relates to Mage_CatalogSearch new feature translations Relates to app/locale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants