-
Notifications
You must be signed in to change notification settings - Fork 1
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
Update Purchase API v2 #122
base: main
Are you sure you want to change the base?
Conversation
SonarCloud Quality Gate failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe there is one severe issue
@@ -152,7 +153,26 @@ public async Task<SinglePurchaseResponse> GetPurchase(int purchaseId, User user) | |||
$"No purchase was found by Purchase Id: {purchaseId} and User Id: {user.Id}"); | |||
} | |||
|
|||
var paymentDetails = await _mobilePayPaymentsService.GetPayment(Guid.Parse(purchase.TransactionId)); | |||
PaymentDetails paymentDetails; | |||
if (purchase.PaymentType == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding, this assumes that all purchases done prior to this pull request were done using mobilePay. This however is not correct, due to us having issued vouchers as part of the datamodel. As such, I believe line 159 will result in unintended behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this again, we would also have the same problem with the older purchases issued by the ipad in analog.
For addressing my first comment, we could wrap the call to mobilepay in a try-catch, and then assign the payment details to a unknownPurchase type or something like that in the catch if mobilepay were to throw an exception.
If we wanted to assign a specific purchase type for purchases from the ipad, we could check the OrderId, since they should all have the id "Analog". Anything that haven't been matched so far, should then be vouchers, assuming no discrepancies in the datamodel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in latest commit
@jonasanker This has been stale for quite a while. Do we want it fixed, and merged, or do we want a rework of the er model as a "proper" fix instead? |
@TTA777 Yea, I think it was lost when the focus changed to Azure. Can we maybe discuss next time or online how the new data model might look like so we can get a sense of the amount of changes? |
Awaits completion of #130 |
Purchase Type is currently used to distinguish between Free Purchases and MobilePay. However could be extended in the future. PaymentType is nullable as we old purchases has no assumed payment type.
a97d595
to
52712d5
Compare
Branch is now up to date. What now❓ @jonasanker @TTA777 |
f9d01aa
to
abc4b4b
Compare
Quality Gate failedFailed conditions |
Closes #120 , #121