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

Tx service gw adapters 2/X #1026

Merged
merged 13 commits into from Mar 3, 2015
Merged

Tx service gw adapters 2/X #1026

merged 13 commits into from Mar 3, 2015

Conversation

ovan
Copy link
Contributor

@ovan ovan commented Feb 26, 2015

No description provided.

@ovan ovan force-pushed the tx-service-gw-adapters-2 branch 6 times, most recently from 30fc4cf to ce79635 Compare March 2, 2015 11:55

def get_payment_details(tx:)
payment_total = Maybe(PaymentModel.where(transaction_id: tx[:id]).first).total_sum.or_else(nil)
total_price = tx[:unit_price] * 1 # TODO fixme for booking (model.listing_quantity)
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be done before merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to check this once more. It's a copy & paste comment so no behavior change here between master and this branch but I could either remove the comment or then change something.

@rap1ds
Copy link
Member

rap1ds commented Mar 3, 2015

You found out the bug (or at least very confusing behaviour) of result.maybe returning None if the data is empty. What do you think, shouldn't we fix it quite soon? I mean in general, not maybe in this branch.

@rap1ds
Copy link
Member

rap1ds commented Mar 3, 2015

TransactionStore is now tightly coupled to the Conversation (add_message, mark_as_unseen, e.g.) What do you think, is it ok for now or should it be changed?

@rap1ds
Copy link
Member

rap1ds commented Mar 3, 2015

TransactionService.charge_commission seems to be Paypal specific. Should it be in adapter instead?

@ovan
Copy link
Contributor Author

ovan commented Mar 3, 2015

result.maybe: Yes, should be fixed to avoid others having to discover this odd behavior and unnecessary debugging. Not in this branch.

coupled conversation: This is not modelled optimally but also not in the scope of this change. This just moves model level operations behind a store interface but doesn't change the way things are modelled under the hood. Hopefully this makes changing later easier because the surface area of the modelling implications is smaller.

Good point about charge commission. Adapter is not exactly the correct place, it's a process level thing somehow. I left out the part where processes would handle the async behavior consistently with the sync behavior because this includes moving actions away from statesman and MarketplaceService::Transaction. Not sure when this work will happen but at least we need to clarify what to do with preauth flow before that. Charge commission should be addressed then too.

@ovan ovan force-pushed the tx-service-gw-adapters-2 branch from 8d188a3 to 1adc73b Compare March 3, 2015 10:22
ovan added a commit that referenced this pull request Mar 3, 2015
@ovan ovan merged commit 61a5582 into master Mar 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants