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 ability to change payment status #460

Merged
merged 6 commits into from
Oct 30, 2013

Conversation

stloyd
Copy link
Contributor

@stloyd stloyd commented Oct 25, 2013

Additionally:

  • be consistent and use sylius_price instead of sylius_money,
  • fixed wrong interface used in payment method model

Refs: #458.

</td>
<td><span class="label label-success">{{ payment.state }}</span></td>
<td><span class="label {{ payment.state == 'completed' ? 'label-success' : (payment.state in ['checkout', 'processing', 'new'] ? 'label-info' : 'label-danger') }}">{{ payment.state }}</span></td>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this condition will be used several times (in Order list, etc.), shouldn't it be intesresting to put it in a macro?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH. I didn't saw any point on using macro here... Order list use different states than payments so it can't be re-used (have a lookt at #459).

And if we support all those states the macro will need to have more lines than this file =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, changed my mind =) It's friday =)

@makasim
Copy link
Contributor

makasim commented Oct 25, 2013

@stloyd

  • Does this PR about changing state in the backend?
  • Does it logged somewhere that payment status has changed manually and a reason why?
  • In future we may have a button sync in the backed. This sync will updates payment details and state depending on new details. That's ok if payment state not changed manually. In case of manual status we can overwrite it with sync and it is not how it should be. So there should be a flag to find out that state changed manually or not.

Just to clarify: as far as I can see it does not fix #457.

@stloyd
Copy link
Contributor Author

stloyd commented Oct 25, 2013

@makasim I never said it fixes the #457, it's for #458.

@makasim
Copy link
Contributor

makasim commented Oct 25, 2013

@stloyd my bad I've typed the wrong issue number. It is not fixes #458 😄

*/
public function getCurrencyName()
{
return Intl::getCurrencyBundle()->getCurrencyName($this->currency);
Copy link
Contributor

Choose a reason for hiding this comment

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

are there strict requirements with the intl extension version installed in the system? I remember something in the composer.lock changed when running the SystemRequirements.php when some things are not installed properly. Do you have any pointers on this? If you don't follow what i am saying then nvm 👶

pjedrzejewski pushed a commit that referenced this pull request Oct 30, 2013
Add ability to change payment status
@pjedrzejewski pjedrzejewski merged commit 8f90c97 into Sylius:master Oct 30, 2013
@pjedrzejewski
Copy link
Member

@makasim I am good with merging this, we will simply rework this once we introduce notifications/sync for payments. Thanks Joseph!

@stloyd stloyd deleted the feature/backend branch October 30, 2013 09:17
@@ -49,7 +49,7 @@ Feature: Exchange rates

Scenario: Creating new exchange rate
Given I am on the exchange rate creation page
When I fill in "Currency" with "PLN"
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks it may even seem to be useful for Warsaw :P

Copy link
Member

Choose a reason for hiding this comment

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

:D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants