Transaction Service - Submit delivery details #94

yg-apaza · 2017-06-01T04:57:07Z

Use Case 3: As a buyer, I want to submit delivery details for the seller

Update TransactionEntity, command: SubmitDeliveryDetails, event: DeliveryDetailsSubmitted and state
Add tests: shouldForbidSubmittingDeliveryDetailsByNonBuyer, shouldEmitEventWhenSubmittingDeliveryDetails
UI and Controllers on web-gateway
Add getTransaction and fill form of DeliveryDetails in UI
Show transaction details
Add integration tests (shouldCreateTransactionOnAuctionFinished, shouldNotCreateTransactionWithNoWinner, shouldSubmitDeliveryDetails)

yg-apaza · 2017-06-02T05:28:17Z

@jroper @TimMoore

I got failed test from latest build on Travis CI, why is this?
Should I use commandFailed method or instead throw new Forbidden(...)?
I was looking for some examples in other services and found 2 TODOs: Use Forbidden instance and Avoid using TransportException
Aren't these contradictory?

yg-apaza · 2017-06-12T08:55:17Z

This PR is now finished 😅
@jroper @TimMoore @ignasi35 , could you please review?

ignasi35 · 2017-06-12T10:08:50Z

On it...

ignasi35 · 2017-06-12T10:31:21Z

transaction-impl/src/main/java/com/example/auction/transaction/impl/DeliveryData.java

+import lombok.Value;
+
+@Value
+public class DeliveryData {


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.

ignasi35 · 2017-06-12T11:23:40Z

transaction-impl/src/main/java/com/example/auction/transaction/impl/TransactionEntity.java

+                );
+            }
+            else
+                throw new Forbidden("Only the auction winner can submit delivery details");


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.

Changing this will break your tests.
Also, I've just noticed there's a similar case few lines below this.

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.

So, can I throw an exception? Using ctx.failed breaks test, is it related with lagom/lagom#325?

While browsing the Lagom source code, found this
As @jroper said, commands are inside a try catch, so I think it's safe throwing an exception
I changed this in the latest commit

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.

Since some services use throw new(...) and others ctx.commandFailed, I raised #108

ignasi35 · 2017-06-12T11:27:13Z

transaction-impl/src/main/java/com/example/auction/transaction/impl/TransactionMappers.java

+        }
+        else
+            return Optional.empty();
+    }


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.

ignasi35 · 2017-06-12T11:30:31Z

transaction-impl/src/main/java/com/example/auction/transaction/impl/TransactionMappers.java

+        );
+    }
+
+    public static TransactionInfo toApi(TransactionState data) {


I think this is dead code. Is that correct? Is that WIP?

I forgot to delete it 😅

Nevermind, I'm using TransactionInfo toApi(TransactionState data)here

ignasi35 · 2017-06-12T11:32:21Z

transaction-impl/src/main/java/com/example/auction/transaction/impl/TransactionMappers.java

+    }
+
+    public static TransactionInfo toApi(TransactionState data) {
+        Transaction transaction = data.getTransaction().get();


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.

I added a comment, using get() is safe

ignasi35 · 2017-06-12T11:33:25Z

This is great work and you are progressing steadily!
I haven't reviewed the UI/Play portion but it looks awesome.

ignasi35 · 2017-06-12T11:40:26Z

I was looking for some examples in other services and found 2 TODOs: Use Forbidden instance and Avoid using TransportException. Aren't these contradictory?

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.

ignasi35 · 2017-06-12T14:07:15Z

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?
Would it be benefitial to add a couple of happy-path tests as we move forward with this service ?

yg-apaza · 2017-06-15T04:55:11Z

How about now? @ignasi35 @TimMoore @jroper

ignasi35 · 2017-06-15T13:02:32Z

...rc/test/java/com/example/auction/transaction/impl/TransactionServiceImplIntegrationTest.java

            TransactionInfo retrievedTransaction = transactionService.getTransaction(itemId)
                    .handleRequestHeader(authenticate(creatorId))
                    .invoke()
                    .toCompletableFuture()
                    .get(5, SECONDS);
+
            assertEquals(retrievedTransaction, transactionInfoWithDelivery);


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 the TransactionServiceImpl + TransactionEntity classes.

ignasi35 · 2017-06-15T13:03:09Z

Other than my previous comment it LGTM

ignasi35 · 2017-06-16T12:52:55Z

...rc/test/java/com/example/auction/transaction/impl/TransactionServiceImplIntegrationTest.java


+        eventually(new FiniteDuration(15, SECONDS), () -> {


TimMoore

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 😄

TimMoore · 2017-06-21T11:48:55Z

web-gateway/app/controllers/TransactionController.java

+                        );
+                    }
+                    else {
+                        String msg = ((TransportException) exception.getCause()).exceptionMessage().detail();


I'm not sure that it's safe to always assume that exception.getCause() will always be a TransportException.

I'm going to fix this as part of #107

yg-apaza added 4 commits May 31, 2017 23:49

[TRANSACTION SERVICE] Submit delivery details

eff0e91

Merge with master

fb0a017

Finish submitDeliveryDetails callback

69181bd

Add test shouldEmitEventWhenSubmittingDeliveryDetails

789aa48

yg-apaza force-pushed the UC03 branch from d3b07c4 to 6dc2b4b Compare June 2, 2017 02:27

Add test shouldEmitEventWhenSubmittingDeliveryDetails

b7476de

yg-apaza force-pushed the UC03 branch from ec7d9bf to b7476de Compare June 2, 2017 04:18

Add test shouldForbidSubmittingDeliveryDetailsByNonBuyer

d7f3dfd

yg-apaza added 3 commits June 2, 2017 20:05

UI Setup

78707b1

UI Setup

b8effcb

UI for submit delivery details finished

6836d6c

yg-apaza force-pushed the UC03 branch from 66d90e8 to 6836d6c Compare June 3, 2017 08:06

yg-apaza added 3 commits June 5, 2017 14:47

Add UI messages

46cf4bb

Testing fixed

81c6c0b

Finish UC03

7f32f9a

yg-apaza changed the title ~~[WIP] Transaction Service - Submit delivery details~~ Transaction Service - Submit delivery details Jun 6, 2017

yg-apaza changed the title ~~Transaction Service - Submit delivery details~~ [WIP] Transaction Service - Submit delivery details Jun 7, 2017

yg-apaza added 5 commits June 8, 2017 02:59

GetTransaction in PersistentEntity and tests

99deb00

Merge branch 'master' into UC03

2a3e8e9

Check status in UI

0ee2fba

Fill DeliveryDataForm with data

c0783b6

Set delivery details to readonly for seller and show transaction details

4b81f9e

yg-apaza changed the title ~~[WIP] Transaction Service - Submit delivery details~~ Transaction Service - Submit delivery details Jun 12, 2017

ignasi35 reviewed Jun 12, 2017

View reviewed changes

ignasi35 added the enhancement New feature or request label Jun 12, 2017

ignasi35 requested a review from TimMoore June 12, 2017 14:07

Fix for review

ab2430a

yg-apaza force-pushed the UC03 branch from 104a174 to ab2430a Compare June 13, 2017 18:47

Add integration test shouldCreateTransactionOnAuctionFinished

ec88a6d

Fix entity tests and integration tests

8ed1425

yg-apaza force-pushed the UC03 branch from cedc993 to 8ed1425 Compare June 15, 2017 06:24

ignasi35 reviewed Jun 15, 2017

View reviewed changes

shouldSubmitDeliveryDetails

c3e4dea

ignasi35 reviewed Jun 16, 2017

View reviewed changes

TimMoore reviewed Jun 21, 2017

View reviewed changes

TimMoore merged commit fb7e30c into lagom:master Jun 21, 2017

yg-apaza deleted the UC03 branch June 22, 2017 03:44

yg-apaza mentioned this pull request Jul 21, 2017

Use Case: As a user, I want to see my transactions #107

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transaction Service - Submit delivery details #94

Transaction Service - Submit delivery details #94

yg-apaza commented Jun 1, 2017 •

edited

yg-apaza commented Jun 2, 2017 •

edited

yg-apaza commented Jun 12, 2017

ignasi35 commented Jun 12, 2017

ignasi35 Jun 12, 2017

yg-apaza Jun 12, 2017

ignasi35 Jun 12, 2017

ignasi35 Jun 12, 2017

jroper Jun 13, 2017

yg-apaza Jun 13, 2017

yg-apaza Jun 15, 2017

TimMoore Jun 21, 2017

yg-apaza Jun 23, 2017

ignasi35 Jun 12, 2017 •

edited

ignasi35 Jun 12, 2017

yg-apaza Jun 12, 2017

yg-apaza Jun 13, 2017

ignasi35 Jun 12, 2017

yg-apaza Jun 15, 2017

ignasi35 commented Jun 12, 2017

ignasi35 commented Jun 12, 2017

ignasi35 commented Jun 12, 2017

yg-apaza commented Jun 15, 2017

ignasi35 Jun 15, 2017

ignasi35 commented Jun 15, 2017

ignasi35 Jun 16, 2017

TimMoore left a comment

TimMoore Jun 21, 2017

yg-apaza Jun 23, 2017

Transaction Service - Submit delivery details #94

Transaction Service - Submit delivery details #94

Conversation

yg-apaza commented Jun 1, 2017 • edited

yg-apaza commented Jun 2, 2017 • edited

yg-apaza commented Jun 12, 2017

ignasi35 commented Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ignasi35 Jun 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ignasi35 commented Jun 12, 2017

ignasi35 commented Jun 12, 2017

ignasi35 commented Jun 12, 2017

yg-apaza commented Jun 15, 2017

Choose a reason for hiding this comment

ignasi35 commented Jun 15, 2017

Choose a reason for hiding this comment

TimMoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yg-apaza commented Jun 1, 2017 •

edited

yg-apaza commented Jun 2, 2017 •

edited

ignasi35 Jun 12, 2017 •

edited