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

Fixes for #1564 and #1289 #1568

Merged
merged 5 commits into from
May 16, 2021
Merged

Fixes for #1564 and #1289 #1568

merged 5 commits into from
May 16, 2021

Conversation

addison74
Copy link
Contributor

@addison74 addison74 commented Apr 23, 2021

This PR fixes #1564 and #1289 . Please read bellow my comments for every commit.

ADDISON added 2 commits April 23, 2021 23:18
By PR #1480 which is merged in OM the content field is no longer required. If it is not filled in an empty container will be visible in frontend. This PR comes to solve this issue.
This commit will change in Backend -> Sales menu a link name from "Terms and conditions" to "Terms and Conditions". 

It is not necessary to add the text in /app/locale/en_US/Mage_Checkout.csv because it already exists. I chose to keep the old version too.
@github-actions github-actions bot added the Component: Catalog Relates to Mage_Catalog label Apr 23, 2021
@addison74 addison74 changed the title Checkout - Agreement content when empty (Terms and Conditions) Fixes for #1564 and #1289 Apr 23, 2021
@addison74 addison74 closed this Apr 23, 2021
@addison74 addison74 reopened this Apr 23, 2021
@kiatng
Copy link
Contributor

kiatng commented Apr 24, 2021

The 2 issues are not related and should be split into 2 separate PRs. It will be easier to review.

@addison74
Copy link
Contributor Author

@kiatng: I used the GitHub interface when I posted them and I'm not very familiar with it. I'm sorry for this inconvenience.

The only commit that needs attention is the one related to the order of the attributes in the comparison window. The code does not belong to me, but it works, I have been testing it for over 5 years. It may require optimization or another approach. In any case, this issue had to be solved for a long time.

kiatng
kiatng previously approved these changes Apr 24, 2021
fballiano
fballiano previously approved these changes Apr 26, 2021
kkrieger85
kkrieger85 previously approved these changes May 12, 2021
@Flyingmana Flyingmana merged commit ec729b2 into OpenMage:1.9.4.x May 16, 2021
@addison74
Copy link
Contributor Author

addison74 commented Jun 30, 2024

I did not find in the latest OpenMage the changes related to this #1289, even this PR was approved and fix it.

Maybe I am wrong ... but in the comparison window attributes are not following their set up position.

@addison74
Copy link
Contributor Author

In this file

/app/code/core/Mage/Catalog/Model/Resource/Product/Compare/Item/Collection.php

line 209 function getComparableAttributes() was change again after this PR to the initial behavior. This is the reason the attributes in comparison window are not following the order set up the Backend.

@addison74
Copy link
Contributor Author

This PR is responsible #2993. The function mentioned above has been radically modified, but the check whether the attributes are in order or not has not been done. fortunately, thanks to the migration of a store where I had custom changes, I was able to identify this problem. my opinion that that PR needs to be reviewed carefully.

@davidhiendl - may I ask why you changed the function getComparableAttributes() although it solved the problem related to the order of the attributes in the comparison window?

@davidhiendl
Copy link
Contributor

davidhiendl commented Jun 30, 2024

@addison74 The aim of the PR was to reduce the countless DB queries made for fetching attributes for a single batch call to cache storage or database. getComparableAttributes however does not have any impact on the layered navigation plus I cannot reproduce the problem. Also the new function implements the same database logic from cached data:

  • fetch attributes for setIds
  • filter by is_comparable
  • sort order by position

Edit: Actually there might be, DB did sort by two dimensions: attribute_group_id and sort_order but I still don't think it would have any impact.

Edit2: Also I was unaware that it got merged back into the old 1.9.x branch, I was under the impression it was only on 20.x

@addison74
Copy link
Contributor Author

addison74 commented Jun 30, 2024

My PR was related to the wrong positioning of the attributes in the comparison window. Your PR came after and modified my changes and now I am not able to see the attributes in the order I set up.

Please note it is not related to layering navigation, that one is a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Catalog Relates to Mage_Catalog Component: Checkout Relates to Mage_Checkout Template : base Relates to base template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checkout - Agreement content when empty (Terms and Conditions)
7 participants