10000 Add displayCartExtraProductInfo hook by jf-viguier · Pull Request #699 · PrestaShop/hummingbird · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add displayCartExtraProductInfo hook #699

New iss 8000 ue

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

Merged

Conversation

jf-viguier
Copy link
Contributor
Questions Answers
Description? Add displayCartExtraProductInfo hook
Type? new feature
BC breaks? no
Deprecations? no
Fixed ticket? Fixes PrestaShop/PrestaShop#38692
Sponsor company Creabilis.com
How to test? See issue

@ps-jarvis ps-jarvis moved this from Ready for review to To be tested in PR Dashboard May 14, 2025
@Hlavtox Hlavtox merged commit eb87698 into PrestaShop:develop May 15, 2025
6 checks passed
@github-project-automation github-project-automation bot moved this from To be tested to Merged in PR Dashboard May 15, 2025
@ps-jarvis
Copy link

PR merged, well done!

Message to @PrestaShop/committers: do not forget to milestone it before the merge.

@nicosomb
Copy link
Contributor

No QA here? 🤔

@matks

@Hlavtox
Copy link
Contributor
Hlavtox commented May 15, 2025

@nicosomb Not needed, it's only a smarty hook and the syntax is identical to all other simmilar hooks. I verified it on a live store.

@Hlavtox
Copy link
Contributor
Hlavtox commented May 15, 2025

@nicosomb But thanks for keeping an eye on things, as always.

@jolelievre
Copy link
Contributor

Sorry, @Hlavtox but even though we agree there's little risk in this modification it's not up to you to unilaterally decide that QA is not needed and ignore this step by using your privileges to merge...

Remember, with great power comes great responsibility 😅 Just a kind warning: if you keep abusing those privileges, I'm afraid we'll be forced to revoke your merge authorisations

@Hlavtox
Copy link
Contributor
Hlavtox commented May 15, 2025

@jolelievre Jo, this topic again. :-) I’ve seen much more impactful PRs merged without QA and no warnings were issued. If we’re going to strictly enforce rules, I’m all for it — but it should be done consistently, not selectively.

@ShaiMagal
Copy link
Contributor
ShaiMagal commented May 15, 2025

@Hlavtox @jolelievre @nicosomb
I think that this PR does not need any QA.
However, I am not against QA having to be done on every PR.
But it would be good to define it clearly so that there are no mistakes. If we are not able to distinguish which PRs need QA and which do not, then why not just solve QA everywhere and only then do the merge after QA?

WDYT guys?
We need to stick together and not argue about someone "abusing" their powers.
Let's be united and move PrestaShop forward together.

@matks
Copy link
Contributor
matks commented May 15, 2025

@ShaiMagal @Hlavtox One simple solution that has been followed in the past is

  1. We see a PR that seems eligible to be tested by a developer or does not require testing at all because CI tests it all
  2. We ask a QA analyst whether he/she agrees with that opinion
  3. QA analyst says "Yes, I agree, let's go" and PR can be merged without QA

At the end of the day QA analysts are the ones deciding whether or not they should test it 😉

@jolelievre
Copy link
Contributor

Yep, to put it simply we just need to ask the QA if its ok to merge it

tivuno added a commit to tivuno/hummingbird that referenced this pull request May 28, 2025
Merge pull request PrestaShop#699 from jf-viguier/hookDisplayCartExtraProductInfo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants
0