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 missing fields to Shop, Customer and Order, and fix tests for shop in Norway #675

Merged
merged 5 commits into from
Nov 16, 2021

Conversation

@Zikoat
Copy link
Contributor Author

Zikoat commented Oct 12, 2021

Hiii @nozzlegear, could you take a look at this and see if it can be merged? Having the order's "current" fields would be a lot of help to our app as it would be much easier to calculate the total amount and total outstanding amount on the order.

If you have some questions about the things done in the tests, they were needed to run the tests for our shop which is in Norway.

@nozzlegear
Copy link
Owner

nozzlegear commented Oct 12, 2021 via email

Copy link
Owner

@nozzlegear nozzlegear left a comment

Choose a reason for hiding this comment

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

Alrighty, I've reviewed your PR and most of it looks good to me! I just need to request a couple changes to the new properties you've added:

  1. DateTime should be changed to DateTimeOffset?.
  2. bool should be changed to bool?.

In general I try to ensure that all fields are clearly marked as nullable where possible, as we've run into many issues in the past where an unexpected null value breaks deserialization.

@Zikoat
Copy link
Contributor Author

Zikoat commented Oct 19, 2021

@nozzlegear Could you review this again? I also changed CheckoutToken to String.

@Zikoat
Copy link
Contributor Author

Zikoat commented Nov 11, 2021

@nozzlegear Could you take a look at this and see if you could get it published?

@nozzlegear
Copy link
Owner

nozzlegear commented Nov 11, 2021 via email

@nozzlegear nozzlegear merged commit 40ba362 into nozzlegear:master Nov 16, 2021
@nozzlegear
Copy link
Owner

Published in v5.14.0 on nuget. Thank you very much for the pull request!

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