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

PricePart default currency #26

Merged
merged 4 commits into from
Apr 17, 2019
Merged

PricePart default currency #26

merged 4 commits into from
Apr 17, 2019

Conversation

microposmp
Copy link
Contributor

I've implemented a few small changes for the PricePart.

  • Fixed a NullReferenceException when creating a new content item that contains a PricePart.
  • Added SEK (Swedish krona) currency. Thinking about making an editor for maintaining currency data.
  • Added a settings page with option to configure default currency.

I'm interested in participating in the development of the OrchardCore.Commerce module.

Copy link
Member

@bleroy bleroy left a comment

Choose a reason for hiding this comment

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

Awesome, first PR, thanks! Please delete the commented out code and I'll just merge this, looks good!

public class CommerceSettingsConfiguration : IConfigureOptions<CommerceSettings>
{
private readonly ISiteService _site;
//private readonly IDataProtectionProvider _dataProtectionProvider;
Copy link
Member

Choose a reason for hiding this comment

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

Please delete, rather than comment out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed commented out code and squashed the commit into the previous commit.

@bleroy
Copy link
Member

bleroy commented Apr 16, 2019

If you could also squash out those gitignore changes, to keep history clean, that would be great. Thanks!

@bleroy
Copy link
Member

bleroy commented Apr 16, 2019

Great thanks. Unless you're going to do it yourself, I'll squash some of those before I merge.

@microposmp
Copy link
Contributor Author

Hi, I don't know how to do the squash. I'll learn that for future commits. Can you do it for this change? Thanks

@bleroy
Copy link
Member

bleroy commented Apr 16, 2019

Sure, no problem. I'll do it, that's perfectly fine. FWIW, the way you do it is that you do an interactive rebase, something like git rebase -i HEAD~7 where 7 should be replaced with how many changes you want to act on. Then you get a text editor with the list of changes, and on each line you can replace the operation according to the comment that got inserted in there, to squash changes with the ones above (I used 'f' the most, personally). Once you're satisfied with the result, and you have a clean change list, you do a push of the branch with the --force-with-lease option. That last one is what rewrites history on the github side.

…t attached.

This issue is solved by assigning the default currency when creating a new object.
Changed to display IsoCode in PricePart editor. Many currencies have the same symbol, so it's not clear what currency is selected.
@bleroy bleroy merged commit 7c614cb into OrchardCMS:master Apr 17, 2019
@microposmp microposmp deleted the currencyfixes branch April 17, 2019 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants