-
Notifications
You must be signed in to change notification settings - Fork 109
Transaction Service - Submit delivery details #94
Conversation
|
On it... |
import lombok.Value; | ||
|
||
@Value | ||
public class DeliveryData { |
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.
This is the private representation of the transaction-api
's DeliveryInfo
.
We have a terrible mix of naming convetions in that sometimes the private copy is PXyz
, sometimes it's the exact same name with only a package difference, ...
In general I hate using Data
and Info
for anything other than signifying that data
is master and info
can be derived from data
.
I don't think this issue of naming should be solved in this PR but it' be good if we settled and considered creating a convention.
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.
Raised #98
); | ||
} | ||
else | ||
throw new Forbidden("Only the auction winner can submit delivery details"); |
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 think you'd better use ctx.failed
here. Throwing an exception will kill the underlying actor and cause it to be rebuilt. If you use ctx.failed
you are signaling the command process completed successfully with a rejection.
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.
Changing this will break your tests.
Also, I've just noticed there's a similar case few lines below this.
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'm not sure if that's the case - I think all commands are processed in a try catch
, and when a command handler throws an exception, Lagom catches it and passes it to ctx.failed
for you.
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.
So, can I throw an exception? Using ctx.failed
breaks test, is it related with lagom/lagom#325?
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.
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 think ctx.failed
would be a better example of what we consider best practices... I'm not sure why it's failing the test, but we can look at it in a separate pull request. This is OK for now.
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.
Since some services use throw new(...)
and others ctx.commandFailed
, I raised #108
} | ||
else | ||
return Optional.empty(); | ||
} |
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.
This is a very common antipattern of Optional
. A if(opt.isPresent) {...} else {Optional.empty}
is equivalent to Optional.map()
. The code can be rewritten as:
return data.map(deliveryData -> new DeliveryInfo(
deliveryData.getAddressLine1(),
deliveryData.getAddressLine2(),
deliveryData.getCity(),
deliveryData.getState(),
deliveryData.getPostalCode(),
deliveryData.getCountry()
));
NOTE: IntelliJ made the suggestion and the rewrite for me. IntelliJ detected the antipattern and marked the code in brown, then using Alt-return
made the fix.
); | ||
} | ||
|
||
public static TransactionInfo toApi(TransactionState data) { |
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 think this is dead code. Is that correct? Is that WIP?
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 forgot to delete it 😅
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.
Nevermind, I'm using TransactionInfo toApi(TransactionState data)
here
} | ||
|
||
public static TransactionInfo toApi(TransactionState data) { | ||
Transaction transaction = data.getTransaction().get(); |
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.
get()
could throw a NullPointerException
.
Should this code consider the case where data.getTransaction()
returns empty
? If that can't happen, then It'd be great to have a comment: // this code is always executed when transactions is set so we can get() safely
.
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 added a comment, using get()
is safe
This is great work and you are progressing steadily! |
They are. So far the approach we're using is: if the service is mall enough then use some transport exceptions inside the Persistent Entity. If you think using more domain-rich exceptions will make the code more maintainable then don't. |
Hi @yg-apaza earlier I forgot to mention that this PR doesn't have any integration test (start a server, send some requests and/or messages via a topic stub). Is that on purpose? |
TransactionInfo retrievedTransaction = transactionService.getTransaction(itemId) | ||
.handleRequestHeader(authenticate(creatorId)) | ||
.invoke() | ||
.toCompletableFuture() | ||
.get(5, SECONDS); | ||
|
||
assertEquals(retrievedTransaction, transactionInfoWithDelivery); |
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'm not sure this test is correct.
I mean, the test does pass, but it uses eventually
in a way that's not safe and which we shouldn't promote:
- eventually is a loop that is executing a block of code with an assertion that eventually becomes successful. The key thing is that the code block is run over and over, which means the code in
transactionService.submitDeliveryDetails...
is invoked several times. IIUC, that code is not idempotent in theTransactionServiceImpl + TransactionEntity
classes.
Other than my previous comment it LGTM |
|
||
eventually(new FiniteDuration(15, SECONDS), () -> { |
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.
👍
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.
Nice work!
I had a couple of comments, but I'm going to go ahead and merge this. Please consider those changes in another pull request. In general, it would be better to keep pull requests small and focused. This one is large enough that it's hard to review it carefully without getting lost 😄
); | ||
} | ||
else { | ||
String msg = ((TransportException) exception.getCause()).exceptionMessage().detail(); |
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'm not sure that it's safe to always assume that exception.getCause()
will always be a TransportException
.
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'm going to fix this as part of #107
Use Case 3: As a buyer, I want to submit delivery details for the seller
TransactionEntity
, command:SubmitDeliveryDetails
, event:DeliveryDetailsSubmitted
and stateshouldForbidSubmittingDeliveryDetailsByNonBuyer
,shouldEmitEventWhenSubmittingDeliveryDetails
web-gateway
getTransaction
and fill form of DeliveryDetails in UIshouldCreateTransactionOnAuctionFinished
,shouldNotCreateTransactionWithNoWinner
,shouldSubmitDeliveryDetails
)