Sparrow dom/historical listing fix by sparrowDom · Pull Request #3250 · OriginProtocol/origin

sparrowDom · 2019-09-06T17:49:12Z

Description:

When user is observing a historical listing replace the purchase option with a "View current listing" button.
Also hide total payment amount and Your Eth will be transferred to escrow info box.

Related Issue: #3237

Checklist:

Test your work and double-check to confirm that you didn't break anything
Wrap any new text/strings with fbt for translation
If there are any new/changed strings to translate, npm run translate
Update any relevant READMEs and docs

…alListingFix

micahalcorn · 2019-09-06T20:43:01Z

Thanks @sparrowDom 👏 Can we get more whitespace below that yellow alert? Probably about the same as below the blue button.

Also, I'm wondering since you used the word "available", do we necessarily know here that the listing has availability? Or could a buyer click the blue button and then see "sold out" on the next screen. Or will that entire section change state on this initial screen and not display the alert/button at all?

micahalcorn · 2019-09-06T20:44:56Z

@sparrowDom let's also hide the "Your ETH will..." in this case. And for that matter, I would probably recommend hiding ~~all of~~ the ~~quantity+total+~~ payment as we do when the buyer has not yet onboarded. 🤔

…ing is available. Also refactor code

sparrowDom · 2019-09-06T21:48:17Z

As requested warning now looks like this @micahalcorn :

Do tell if any additional tweaks are required.

@nick PTAL when you can spare some time

shahthepro

Great work @sparrowDom. Few comments inline. Looks good to me. But @nick may want to take a pass too.

shahthepro · 2019-09-07T04:56:44Z

+      } else if (isHistoricalListing(this.props.listing)) {
+        action = (
+          <HistoricalListingWarning
+            listingId={currentListingIdFromHistoricalId(this.props.listing)}


nit: If the component is named HistoricalListingWarning, I'd expect the parameter listingId to be the id of the historical listing and not of the latest listing. Perhaps, could move currentListingIdFromHistoricalId function call inside the HistoricalListingWarning or rename listingId to latestListingId. Just my opinion though.

Yeah agreed. Moving the original listing resolution to historicalListingWarning component

shahthepro · 2019-09-07T04:58:21Z

@@ -0,0 +1,19 @@
+const regex = /(\d*)-(\d*)-(\d*)-(\d*)/


You may want to switch it to /(\d+)-(\d+)-(\d+)-(\d+)/. But shouldn't be problem with * too since we won't have listing Ids with trailing or leading '-'.

Also, you may want to specify ^ and $ characters to be more defensive.

All good points thanks

shahthepro · 2019-09-07T04:59:32Z

+ * This can happen when for example user clicks on a listing from a Purchase detail view
+ */
+export function isHistoricalListing(listing) {
+  return !!listing.id.match(regex)


Instead of !! and .match, regex.test(listing.id) can be used. .test() will always return a boolean.

nice, changed to that

shahthepro · 2019-09-07T05:03:10Z

    noEthOrDai = false

-  if (hasIdentity) {
+  if (hasIdentity && !isHistoricalListing(listing)) {


Why not move this check out of this component? We could avoid rendering PaymentOptions if it is a historical component right?

PaymentOptions has children that it needs to render. If we disable the rendering of payment options then components like _BuySingleUnit and Buy do not get rendered either:

origin/dapps/marketplace/src/pages/listing/_BuySingleUnit.js

Lines 62 to 75 in 5aa1d52

<PaymentOptions

tokens={prices}

price={listing.price}

acceptedTokens={listing.acceptedTokens}

value={token}

hasBalance={tokenStatus.hasBalance}

hasEthBalance={tokenStatus.hasEthBalance}

>

<ConfirmShippingAndPurchase

listing={listing}

className="btn btn-primary"

children={fbt('Purchase', 'Purchase')}

/>

</PaymentOptions>

Yeah, But we don't want users to make offer on historical listing, if I'm not mistaken.

From what I understand, we would have a "View Current Listing" instead of "Purchase" button right?

The button that displays the "View Current Listing" is a Buy button component. This ensures us that a user is never able to make an offer on a historical listing since the buy button will always say "View current listing" instead of "Purchase".

And that buy button is part of the children that get passed to the PaymentOption Component.

micahalcorn

I haven't tested this myself, but I'm under the impression that we still need to make some changes here.

The basic issue (I'm told) is that, if a buyer visits a listing detail with a block number in the URL's listing id param, then after making an offer from that block-specific listing, the user will be routed to a transaction detail page with its URL offer id param being polluted with an unnecessary block number, which gets interpreted incorrectly and throws the exception described in #3237. This issue applies whether or not the listing has been updated.

So here is what I was proposing for when the listing detail page is block-number-specific:

If the listing has been updated and I am not looking at the newest version, then display the new UI as this PR does.
If the listing has not been updated or I am looking at the newest version, then the transaction detail navigation should be smart enough not to route me using an invalid offer id (polluted by a block number).

sparrowDom · 2019-09-08T08:37:57Z

thanks @shahthepro for a great review. Merging in once tests pass

…otocol/origin into sparrowDom/historicalListingFix

…alListingFix

… current one

sparrowDom · 2019-09-08T09:47:48Z

@micahalcorn just saw your comment if you are ok with it going with the following solution:

If a user is looking at the newest version of the listing in listingDetail the component just redirects to newest version (without the block number in the url) before rendering the "historical view" of the listing version.

/cc @shahthepro the commit doing this: b4cc886

sparrowDom · 2019-09-08T11:51:04Z

Merging this in...

@micahalcorn & @shahthepro if any additional changes requested will be resolved in subsequent PRs

micahalcorn · 2019-09-08T13:40:51Z

I don’t quite understand, @sparrowDom. 😕 The block numbered version only renders if it is NOT the current version? And if it is, it redirects to a URL with no block number?

sparrowDom · 2019-09-08T22:49:49Z

Yes @micahalcorn it works exactly like you describe:

listing with a block number in the url that is also the current listing version will immediately redirect to the current listing view that has no block number in the url. Rendering the purchase button (the whole redirect happens fast is not something I noticed)
listing with a block number in the url that is not the current listing version will render the historical listing having a "View current listing" button instead of a purchase button and a yellow warning.

Do you think we should take a different approach, or that this one could be confusing?

micahalcorn · 2019-09-08T22:52:59Z

@sparrowDom thanks, that certainly works from the user's perspective, and it sounds like the simplest from the engineering perspective. I would think that it might not be ideologically appropriate, but I don't have strong opinions. 🤷‍♂

sparrowDom · 2019-09-09T22:50:13Z

Yeah it was the simplest solution. Happy to change it to something else if need/wish be.

sparrowDom added 3 commits September 6, 2019 19:43

make user jump to the current listing before purchasing

29d49ed

prettier

a416403

update translations

30768aa

sparrowDom self-assigned this Sep 6, 2019

sparrowDom requested a review from shahthepro September 6, 2019 17:51

sparrowDom added 2 commits September 6, 2019 19:51

Merge remote-tracking branch 'origin/master' into sparrowDom/historic…

dfd80ae

…alListingFix

update translations

25c34e1

sparrowDom requested a review from nick September 6, 2019 17:53

Merge remote-tracking branch 'origin/master' into sparrowDom/historic…

7e6b8a8

…alListingFix

nick requested a review from micahalcorn September 6, 2019 19:53

sparrowDom mentioned this pull request Sep 6, 2019

VM Execution Error #3237

Closed

sparrowDom added 2 commits September 6, 2019 23:41

hide eth disclamer and total payment value when newer version of list…

c129989

…ing is available. Also refactor code

run translations

0eb89dd

Merge branch 'master' into sparrowDom/historicalListingFix

0ad4355

shahthepro reviewed Sep 7, 2019

View reviewed changes

shahthepro approved these changes Sep 7, 2019

View reviewed changes

micahalcorn suggested changes Sep 8, 2019

View reviewed changes

some minor fixes and enhancements

8d4b91e

sparrowDom added 3 commits September 8, 2019 10:40

Merge branch 'sparrowDom/historicalListingFix' of github.com:OriginPr…

dcc162c

…otocol/origin into sparrowDom/historicalListingFix

Merge remote-tracking branch 'origin/master' into sparrowDom/historic…

8887cdf

…alListingFix

redirect to non block number listing if the historical listing equals…

b4cc886

… current one

sparrowDom merged commit f5ed22b into master Sep 8, 2019

sparrowDom deleted the sparrowDom/historicalListingFix branch September 8, 2019 11:51

micahalcorn mentioned this pull request Sep 10, 2019

Historical Listing Information Not Displayed #3335

Open

	<PaymentOptions
	tokens={prices}
	price={listing.price}
	acceptedTokens={listing.acceptedTokens}
	value={token}
	hasBalance={tokenStatus.hasBalance}
	hasEthBalance={tokenStatus.hasEthBalance}
	>
	<ConfirmShippingAndPurchase
	listing={listing}
	className="btn btn-primary"
	children={fbt('Purchase', 'Purchase')}
	/>
	</PaymentOptions>

Uh oh!

Conversation

sparrowDom commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Checklist:

Uh oh!

micahalcorn commented Sep 6, 2019

Uh oh!

micahalcorn commented Sep 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sparrowDom commented Sep 6, 2019

Uh oh!

shahthepro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micahalcorn left a comment

Choose a reason for hiding this comment

Uh oh!

sparrowDom commented Sep 8, 2019

Uh oh!

sparrowDom commented Sep 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sparrowDom commented Sep 8, 2019

Uh oh!

micahalcorn commented Sep 8, 2019

Uh oh!

sparrowDom commented Sep 8, 2019

Uh oh!

micahalcorn commented Sep 8, 2019

Uh oh!

sparrowDom commented Sep 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sparrowDom commented Sep 6, 2019 •

edited

Loading

micahalcorn commented Sep 6, 2019 •

edited

Loading

sparrowDom commented Sep 8, 2019 •

edited

Loading