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

"Smarty Price formater" in basket, article details and article lists.… #129

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
3 participants
@Josef-A-Puckl
Copy link
Contributor

commented Aug 13, 2018

… This was already introduced in v4.8.0, but forgotten in several places in Flow ( https://oxidforge.org/en/oxid-eshop-version-4-8-0-ce-pe-5-1-0-ee.html#Simply_switch_the_position_of_the_currency_sign ).

"Smarty Price formater" in basket, article details and article lists.…
… This was already introduced in v4.8.0, but forgotten in several places in Flow (https://oxidforge.org/en/oxid-eshop-version-4-8-0-ce-pe-5-1-0- ee.html # Simply_switch_the_position_of_the_currency_sign).
@CLAassistant

This comment has been minimized.

Copy link

commented Aug 13, 2018

CLA assistant check
All committers have signed the CLA.

Josef-A-Puckl added some commits Aug 13, 2018

"Smarty Price formater" Part 2: Tprice in article details and article…
… lists. This was already introduced in v4.8.0, but forgotten in several places in Flow (https://oxidforge.org/en/oxid-eshop-version-4-8-0-ce-pe-5-1-0- ee.html # Simply_switch_the_position_of_the_currency_sign).

@Sieg Sieg added the in progress label Aug 24, 2018

Sieg added a commit that referenced this pull request Aug 27, 2018

Sieg added a commit that referenced this pull request Aug 27, 2018

Sieg added a commit that referenced this pull request Aug 27, 2018

PR-129 Use oxprice plugin in basket, article details and article lists
This was already introduced in v4.8.0, but forgotten in several places in Flow (https://oxidforge.org/en/oxid-eshop-version-4-8-0-ce-pe-5-1-0- ee.html # Simply_switch_the_position_of_the_currency_sign).

Related #129

Sieg added a commit that referenced this pull request Aug 27, 2018

Sieg added a commit that referenced this pull request Aug 27, 2018

Sieg added a commit that referenced this pull request Aug 27, 2018

Sieg added a commit that referenced this pull request Aug 27, 2018

@Sieg

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2018

Hello @Josef-A-Puckl, thanks for the pull request! The places you have tried to fix, are not so easy to fix: Please take a look how those methods like getFTPrice work - it prepares corresponding netto/brutto price on shop configurations :(, so for now, there is no simple solution, and yours is not right, as this logic will be skipped at all. I have reverted those templates, for now, please make a new the pull request if you have ideas how to improve it.

Regarding priceinfo.tpl, fbrutprice and fbrutamountprice are already formatted.. yes, it is bad, but need to fix it first, only after fixing we could apply the oxprice formatting on the top. What can you do:

  • Add same name (without "f" in front) variables nearby. There are some other formatted variables nearby too, maybe those could be fixed as well.. if they are needed at all...
  • Check tests for old variables and make it use new variables
  • Deprecate those we have now in the shop itself
  • Draw a deprecation line in changelog (optional, I will do it if you will not)
  • Put it as a pull request
    then we can apply a formatting on those new variables in a NEW, precise pull request. How does it sound for you?

Please try to make smaller pull requests, ideally - one little problem - one pull request, so we could check and take it faster. Here we have 3 or 4 places with quite different logic, even if it looks related and similar.

With the reverts I have mentioned, I have merged improvements from the Pull request (25e437a)!

If something I have found is wrong, please tell me, maybe I have misunderstood something.. I am human! :)

@Sieg Sieg closed this Aug 27, 2018

@Sieg Sieg removed the in progress label Aug 27, 2018

@Sieg Sieg self-assigned this Aug 27, 2018

@Sieg Sieg added the Ready to merge label Aug 27, 2018

@Sieg

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2018

The oxprice plugin cannot eat already formatted prices. Check prices higher than 1000 euros, and you will see why.

robertblank added a commit that referenced this pull request Nov 6, 2018

PR-129 Use oxprice plugin in basket, article details and article lists
This was already introduced in v4.8.0, but forgotten in several places in Flow (https://oxidforge.org/en/oxid-eshop-version-4-8-0-ce-pe-5-1-0- ee.html # Simply_switch_the_position_of_the_currency_sign).

Related #129

robertblank added a commit that referenced this pull request Nov 6, 2018

robertblank added a commit that referenced this pull request Nov 6, 2018

robertblank added a commit that referenced this pull request Nov 6, 2018

robertblank added a commit that referenced this pull request Nov 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.