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

Add SQL upgrade queries for new unit price column #441

Merged
merged 1 commit into from
Jul 5, 2022

Conversation

jolelievre
Copy link
Contributor

@jolelievre jolelievre commented Dec 17, 2021

Questions Answers
Description? These upgrade queries are related to this PR PrestaShop/PrestaShop#26762 which adds a new column in product tables
In 178 the product table used the unit_price_ratio to save the unit price, so the actual value was not saved only the ratio instead. In 8.0.0 we now have a unit_price column which persists the actual value specified in the BO and used in the FO. But we kept the unit_price_ratio for backward compatibility for now.

This upgrade PR does to things:
- it adds the new column unit_price in the database
- it computes its value based on existing unit_price_ratio and price column
Type? improvement
BC breaks? no
Deprecations? no
Fixed ticket? ~
How to test? You should create (or edit) some products with defined prices AND unit prices. After the upgrade check that the unit_price column has been created in the product table and product_shop table, the unit_price values should be prefilled based on previous values. The formula to compute it is the following unit_price = price / unit_price_ratio, when you go back to the product edition the value for the unit price should be the same even if it is now based on a different column.
Possible impacts? ~

PierreRambaud
PierreRambaud previously approved these changes Dec 21, 2021
@kpodemski
Copy link
Contributor

@jolelievre rebase needed 👍

Copy link
Contributor

@kpodemski kpodemski left a comment

Choose a reason for hiding this comment

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

To be rebased after merge of the related PR from core

PierreRambaud
PierreRambaud previously approved these changes Dec 24, 2021
Copy link
Contributor

@PierreRambaud PierreRambaud left a comment

Choose a reason for hiding this comment

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

PR has been rebased

@marwachelly
Copy link

Hello,

This PR cannot be tested now. It is blocked by the autoupgrade issue #26503 since the PR related is on develop branch. It will be tested as soon as the issue will be fixed.

Thanks,

@marwachelly marwachelly added the Blocked Status: The issue is blocked by another task label Jan 5, 2022
@PierreRambaud PierreRambaud removed this from the 4.14.0 milestone Jan 18, 2022
@khouloudbelguith
Copy link

Hi @jolelievre, @marwachelly,

As discussed with @atomiix while testing the blocked PR
There are some bugs should be fixed in this PR as commented here: #434 (review)

Thank you!

@marwachelly
Copy link

Hello @jolelievre ,

Could you please check the conflicted files?

Thanks,

@marwachelly marwachelly added waiting for author and removed waiting for QA Blocked Status: The issue is blocked by another task labels Feb 17, 2022
Copy link
Member

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

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

@jolelievre Could you rebase your PR ?

Progi1984
Progi1984 previously approved these changes Jun 3, 2022
@Progi1984 Progi1984 added this to the 4.15.0 milestone Jun 3, 2022
atomiix
atomiix previously approved these changes Jun 10, 2022
Copy link

@khouloudbelguith khouloudbelguith left a comment

Choose a reason for hiding this comment

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

Hello @jolelievre,

I followed these steps:

  1. Install PS1786 with English language & France country
  2. Uninstall some incompatible modules
  3. Install the upgrade module from this PR
  4. Upgrade to 8.0.x
  5. After the upgrade, I'm blocked, I cannot test the product V2 because it is hidden
Untitled_.Jun.14.2022.10_54.AM.mp4

We should wait for this PR: #489 to be merged

Thanks!

@khouloudbelguith khouloudbelguith added the Blocked Status: The issue is blocked by another task label Jun 14, 2022
Copy link

@khouloudbelguith khouloudbelguith left a comment

Choose a reason for hiding this comment

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

Hi @jolelievre,

The PR is merged.
Could you please check the Conflicting files?

Thanks!

@khouloudbelguith khouloudbelguith added waiting for author and removed waiting for QA Blocked Status: The issue is blocked by another task labels Jun 21, 2022
@jolelievre
Copy link
Contributor Author

Thanks @khouloudbelguith

the branch is rebased no content was modified only addressed the conflict because a PR that modified the same place was merged. It should be testable even if I'm waiting for a re-approval

Copy link

@khouloudbelguith khouloudbelguith left a comment

Choose a reason for hiding this comment

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

Hi @jolelievre,

It is ok ✔️

  • I checked an upgrade from PS1778 to 8.0.x

  • A rollback from 8.0.x to PS1778

  • I checked an upgrade from PS1786 to 8.0.x

  • A rollback from 8.0.x to PS1786

image
image

Untitled_.Jul.4.2022.2_56.PM.mp4

I tried:

  • BO > Add new order / view order / edit order
  • BO > View & delete shopping Cart
  • BO > Add/Edit a product V1/V2 (multistore enabled / disabled)
  • BO > View customer
  • BO > Email theme > Http of order_conf
  • FO > Create order
  • FO > Sign in / create an account

Thanks!

@atomiix atomiix merged commit b830a97 into PrestaShop:dev Jul 5, 2022
@atomiix
Copy link
Contributor

atomiix commented Jul 5, 2022

Thanks @jolelievre!

atomiix added a commit to atomiix/autoupgrade that referenced this pull request Sep 16, 2022
Add SQL upgrade queries for new unit price column
@AureRita AureRita mentioned this pull request Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants