Skip to content
This repository has been archived by the owner on May 9, 2019. It is now read-only.

Bumps to Lagom 1.4.1. Fixes some ExCtx issues #167

Merged
merged 3 commits into from Mar 7, 2018

Conversation

ignasi35
Copy link
Contributor

@ignasi35 ignasi35 commented Mar 6, 2018

Bumps Lagom to 1.4.1.

While testing I noticed a couple of controllers failed because the CompletionStage composition required the explicit ExecutionContext.

anonymizeBids(user, currency, bidHistory), user, currency, seller, winner, currentBidMaximum, bidResult, (Nav) nav));
}, ec.current());
}, ec.current())
, ec.current());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This huge change is mostly formatting. The interesting bit are the last two ec.current() added.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little surprised that it needs to be added in so many places. I thought it would only be the render call that needs to run in that EC.

This whole thing could stand some refactoring I think... later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole thing could stand some refactoring I think... later

Oh, it does!

.thenApplyAsync(nav -> {
ctx().session().clear();
return ok(views.html.index.render(nav));
}, httpExecutionContext.current());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All this change is formatting and a couple of ec.current() added in seldom methods.

@ignasi35
Copy link
Contributor Author

ignasi35 commented Mar 6, 2018

I've done a rather extensive manual test of the (including search features). Keeping an eye on terminal logs too.

With the changes suggested in this PR everything seems to work.

Hmm, not sure I tested the login operation (I did test signup and logout, though). 🤔

@TimMoore
Copy link
Contributor

TimMoore commented Mar 7, 2018

Easier to review with the secret "ignore whitespace" URL: https://github.com/lagom/online-auction-java/pull/167/files?w=1

@TimMoore
Copy link
Contributor

TimMoore commented Mar 7, 2018

Login page is in fact broken

@TimMoore
Copy link
Contributor

TimMoore commented Mar 7, 2018

It looks like the entire login function is broken. I suggest that we leave that out of this pull request.

@TimMoore
Copy link
Contributor

TimMoore commented Mar 7, 2018

I pushed my WIP changes to the login controller, but the back-end service also needs to be fixed.

@TimMoore
Copy link
Contributor

TimMoore commented Mar 7, 2018

Tests repeatedly timed out, so I increased the timeout duration.

@ignasi35
Copy link
Contributor Author

ignasi35 commented Mar 7, 2018

Thanks @TimMoore !

Copy link
Member

@octonato octonato left a comment

Choose a reason for hiding this comment

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

And then we see how convenient an implicit ec is.

@octonato octonato merged commit a140347 into lagom:master Mar 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants