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

Change feature value table to add position field #925

Merged
merged 3 commits into from
Nov 14, 2024

Conversation

ks129
Copy link
Contributor

@ks129 ks129 commented Sep 27, 2024

Questions Answers
Description? Added position field to feature value table in 9.0.0. Part of PrestaShop/PrestaShop#37042
Type? new feature
BC breaks? no
Deprecations? no
Fixed ticket? part of PrestaShop/PrestaShop#35499 and PrestaShop/PrestaShop#32267
Sponsor company -
How to test? After upgrading to 9.0.0, feature value table should have position field.

@ps-jarvis
Copy link
Collaborator

Hello @ks129!

This is your first pull request on autoupgrade repository of the PrestaShop project.

Thank you, and welcome to this Open Source community!

@ga-devfront ga-devfront added the Blocked Status: The issue is blocked by another task label Sep 30, 2024
@ga-devfront
Copy link
Contributor

Thank you for your contribution! I have blocked the PR while the PR relating to it is merged on the Core. Do not hesitate to ping me if it is merged and I have not reacted in the meantime.

Quetzacoalt91
Quetzacoalt91 previously approved these changes Sep 30, 2024
Copy link
Member

@Quetzacoalt91 Quetzacoalt91 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you. Labeling as "Blocked" until the PR is merged on PrestaShop repository.

Comment on lines 30 to 33
function ps_900_update_configuration()
{
Configuration::updateValue('PS_FEATURE_VALUES_ORDER', 'name');
}
Copy link
Member

@Quetzacoalt91 Quetzacoalt91 Oct 24, 2024

Choose a reason for hiding this comment

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

This method does not seem called by the 9.0.0.sql script.

If you want to add configuration values to the shop, you may refer to this other SQL query:

/* Allow cover configuration */
/* https://github.com/PrestaShop/PrestaShop/pull/33363 */
INSERT INTO `PREFIX_configuration` (`name`, `value`, `date_add`, `date_upd`) VALUES ('PS_USE_COMBINATION_IMAGE_IN_LISTING', '0', NOW(), NOW());

@Quetzacoalt91 Quetzacoalt91 added waiting for author and removed Blocked Status: The issue is blocked by another task Waiting for rebase labels Oct 24, 2024
@ks129 ks129 requested a review from Quetzacoalt91 October 24, 2024 11:22
Quetzacoalt91
Quetzacoalt91 previously approved these changes Oct 29, 2024
@Quetzacoalt91 Quetzacoalt91 added Blocked Status: The issue is blocked by another task and removed waiting for QA waiting for author labels Oct 29, 2024
@Quetzacoalt91 Quetzacoalt91 added Waiting for rebase and removed Blocked Status: The issue is blocked by another task labels Nov 13, 2024
@ks129 ks129 requested a review from Quetzacoalt91 November 14, 2024 08:44
@Quetzacoalt91
Copy link
Member

Hi @ks129,
Can you please rebase this PR so we can proceed with the review? Thanks

@ks129
Copy link
Contributor Author

ks129 commented Nov 14, 2024

@Quetzacoalt91 I already rebased it today.

@Quetzacoalt91
Copy link
Member

Quetzacoalt91 commented Nov 14, 2024

Indeed! Sorry for that, I checked the backlog of PRs yesterday and only remembered this morning that I did not post here.

Copy link
Contributor

@AureRita AureRita left a comment

Choose a reason for hiding this comment

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

Hi @ks129

Thank you for your PR, I tested it and it seems to works as you can see :

image

Tested from :
8.0.4 to 8.2
8.2 to 9.0.0
8.0.4 to 9.0.0

Because the PR seems to works as expected, It's QA ✔️

Thank you

@Quetzacoalt91 Quetzacoalt91 merged commit f1bd1ca into PrestaShop:dev Nov 14, 2024
33 checks passed
@Quetzacoalt91
Copy link
Member

Thanks @ks129

@Quetzacoalt91 Quetzacoalt91 added this to the 7.0.0 milestone Nov 14, 2024
@ps-jarvis
Copy link
Collaborator

PR merged, well done!

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

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.

5 participants