Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

Order details dispute #529

Merged
merged 41 commits into from
Jul 6, 2017
Merged

Order details dispute #529

merged 41 commits into from
Jul 6, 2017

Conversation

rmisio
Copy link
Contributor

@rmisio rmisio commented Jun 27, 2017

This PR adds the following functionality to the Order Details overlay:

  • Ability to open a dispute
  • Ability for a mod to decide on a dispute
  • The UI for a user to accept a payout
  • The Pay For Order section as in the Purchase flow

There are some server bugs blocking the completion of the dispute resolution, notably:

closes #529

rmisio added 26 commits June 15, 2017 16:34
@@ -40,6 +40,15 @@ export default class extends BaseModel {
// convert price fields
response.vendorContract.buyerOrder.payment.amount =
integerToDecimal(response.vendorContract.buyerOrder.payment.amount, true);

// if (response.resolution) {
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 will be needed once the server is properly providing the payout amount. I'll uncomment it then.

@@ -47,6 +47,15 @@ export default class extends BaseModel {
// convert price fields
response.contract.buyerOrder.payment.amount =
integerToDecimal(response.contract.buyerOrder.payment.amount, true);

// if (response.contract.disputeResolution) {
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 will be needed once the server is properly providing the payout amount. I'll uncomment it then.

@@ -312,7 +312,7 @@ export default class extends BaseModal {
this.totalPrice = _totalPrice;
const adjPrice = convertAndFormatCurrency(this.totalPrice,
this.model.get('metadata').get('pricingCurrency'), app.settings.get('localCurrency'));
this.getCachedElement('.js-price').text(adjPrice);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shortened this guy's name.

@@ -1,29 +1,44 @@
/*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made a few tweaks to this view to make it more friendly to be re-used in multiple areas, notably:

  • Made the inputs more explicit. It had been relying on two models to be passed in, but the Order Detail overlay does not have the same models (at least with the same structure), so I switched it were you just directly pass in the data it needs as options.
  • Updated the view so it handles partial payments. In case someone (probably accidentally) makes a partial payment from an external wallet, the totals will be updated to reflect that and if they want to pay the rest from the internal wallet (Pay From Wallet action), it will only send the remaining balance.

@jjeffryes jjeffryes self-requested a review June 29, 2017 15:13
Copy link
Contributor

@jjeffryes jjeffryes left a comment

Choose a reason for hiding this comment

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

Tested, no issues found. Just have some questions in the review and one spot that may need an unbind in remove.

@@ -15,6 +15,10 @@ export default class extends BaseModel {
.get('contractType');
}

get orderTotal() {
return 99;
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do?

throw new Error('Please provide an OrderFulfillment model.');
}

const isValidParticipantObject = (participant) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function gets repeated several times, is it worth centralizing it somewhere?

throw new Error(getInvalidParticpantError('buyer'));
}

options.buyer.getProfile().done(profile => {
Copy link
Contributor

Choose a reason for hiding this comment

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

For this and the various other get profile calls, is it worth doing something if the call fails?

Copy link
Contributor Author

@rmisio rmisio Jun 30, 2017

Choose a reason for hiding this comment

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

It's not necessary because all the profiles are lazy loaded in and the view supports them not being there. For example, let's say we display the text "Billy made a purchase" with Billy being the buyer's name. Until Billy's profile is loaded, it will say "The buyer made a purchase" and when the profile arrives the name will pop in. When it's cached, it's so fast you barely notice the change.

In every case it should work similarly (sometimes, it's a GUID in place of handle, etc...). And, the lack of a profile does not prevent any functionality.

this.onResolveDisputeAlways);

this.boundOnDocClick = this.onDocumentClick.bind(this);
$(document).on('click', this.boundOnDocClick);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be unbound in the remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

return this._state;
}

setState(state, replace = false, renderOnChange = true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

setState is used a lot, would it be worth it to add it to the base view?

Copy link
Contributor Author

@rmisio rmisio Jun 30, 2017

Choose a reason for hiding this comment

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

Yes, it probably should. I didn't initially because this is a very specific way of working that only I was using and it was evolving, but even if only I use it, it won't hurt for it to be in the baseVw and devs who aren't interested in it, could leave it be.

If you don't mind though, I think I'll leave this as a separate task to circle back to, since there's now 35 occurrences of it in the app and it's probably not super high priority to work through this now. Going forward, I'll put setState() and getState() in the baseVw so we don't accumulate more technical debt and at some point I'll re-factor the other views.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine.

@rmisio
Copy link
Contributor Author

rmisio commented Jun 30, 2017

@jjeffryes I just addressed the code review questions / requests, but please hold off before merging. I'll look into that local pickup bug. If it's a quicky, I'll sneak it in here.

Copy link
Contributor

@jjeffryes jjeffryes left a comment

Choose a reason for hiding this comment

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

I was merging the changes you made to purchasing into my own purchasing patch branch so I wouldn't overwrite them later, and it looks like the action button bug was in this PR.

this.listenTo(this.payment, 'walletPaymentComplete',
(pmtCompleteData => this.completePurchase(pmtCompleteData)));
this.$('.js-pending').append(this.payment.render().el);
this.updatePageState('pending');
Copy link
Contributor

Choose a reason for hiding this comment

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

The page state needs to be updated before actionBtn is rendered, or it renders the wrong state.

I think this is what caused #533.

this.actionBtn.render();
this.purchase.set(this.purchase.parse(data));
this.payment.render();
this.payment = this.createChild(Payment, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't there be a if (this.payment) this.payment.remove(); before this?

Copy link
Contributor Author

@rmisio rmisio Jun 30, 2017

Choose a reason for hiding this comment

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

Since we're only going to post ob/purchase once, I don't think it's necessary. Do you see any possible scenario where that guy would be posted more than once?

Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't, but in #533 it looked like it was being appended twice. Though that may just be because the pay button could be clicked twice.

@jjeffryes jjeffryes mentioned this pull request Jun 30, 2017
@jjeffryes jjeffryes mentioned this pull request Jul 2, 2017
@jjeffryes
Copy link
Contributor

jjeffryes commented Jul 3, 2017

@rmisio there's a weird bug in fulfillment for physical orders. Somehow when the physicalDelivery data is passed from FulfillOrder.js to utils/order.js/fulfillOrder, the shipper and tracking number are both reset to blank.

It throws an error and you can't fulfill any orders.

Uncaught Error: physicalDelivery.shipper: Please provide a shipping carrier.
    at C:/Users/J/ob/openbazaar-desktop/js/utils/order.js:174:17

@rmisio
Copy link
Contributor Author

rmisio commented Jul 5, 2017

@jjeffryes

there's a weird bug in fulfillment for physical orders. Somehow when the physicalDelivery data is passed from FulfillOrder.js to utils/order.js/fulfillOrder, the shipper and tracking number are both reset to blank.

ok, should be fixed now.

@jjeffryes
Copy link
Contributor

When a dispute is decided but not accepted, should the payout section show zero prices for everything?

image

@jjeffryes
Copy link
Contributor

Re: zero prices, that's probably because it's a low price transaction, and the payout was eaten by fees, wasn't it?

@morebrownies I think that could be confusing, should we show a message somewhere explaining that?

@jjeffryes
Copy link
Contributor

For some reason the dispute with a settlement of all zeroes is stuck in the decided state for the buyer and seller, but not the moderator. That appears to be a server issue.

@morebrownies
Copy link

Re: zero prices, that's probably because it's a low price transaction, and the payout was eaten by fees, wasn't it?

@morebrownies I think that could be confusing, should we show a message somewhere explaining that?

Well hmmm. That's an interesting scenario. I'm wondering if we should even allow a party to open a dispute when the payout will result in 0? At that point, you're disputing for the point of disputing, but there's no financial outcome for any party involved in the dispute, including the moderator.

That may open a bigger can of worms, but the easiest thing would probably be to display a message when opening a dispute and viewing the payout details, "The bitcoin transaction fee will be $2.47 (estimated), which has been deducted from the payout totals."

@jjeffryes
Copy link
Contributor

That may open a bigger can of worms, but the easiest thing would probably be to display a message when opening a dispute and viewing the payout details, "The bitcoin transaction fee will be $2.47 (estimated), which has been deducted from the payout totals."

I think that would be a decent approach. Though we'd also want to subtract the moderator fee (which is also estimated, since the moderator can change it).

I'll file an issue for it, it's not something we need to do immediately.

@jjeffryes jjeffryes merged commit da72b26 into master Jul 6, 2017
@jjeffryes jjeffryes deleted the order-details-dispute branch July 6, 2017 18:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants