Bulk Update Currencies for Listings #1423

hoffmabc · 2019-01-23T16:17:12Z

This is a very similar API call to the one used to update moderators for all listings.

POST http://localhost:4002/ob/bulkupdatecurrency
Payload: { "currencies":["ZEC", "BTC", "BCH", "LTC"] }

This will update the listings.json as well as ALL listings themselves with the updated currency array.

cpacia · 2019-01-23T16:22:13Z

Looks good at first glance. I'll take a deeper look and test it later today.

coveralls · 2019-01-23T16:26:38Z

Coverage increased (+0.04%) to 34.873% when pulling 4499e3a on update-currencies into 7df4799 on master.

placer14

This is fine as a quick solution. Before this is merged, I'd like to ensure theres:

unit test protecting the success case
unit test protecting walkpath's failure handling
unit test protecting updater's failure handling
updating the postman docs for this new endpoint

api/jsonapi.go

placer14

I think we should also suspend the root hash updater while this bulk update is completing. Otherwise, each individual listing update will trigger the root hash to be updated in the push nodes and create a lot of network chatter. The root hash could be resumed after the bulk update and the final merkle root could be enough to cover all the changes.

jjeffryes · 2019-01-24T21:38:11Z

It looks like this branch set all the listings to track inventory, zero inventory on 2 of my nodes. I'll try and confirm that.

jjeffryes · 2019-01-25T18:56:49Z

I've verified that calling bulkupdatecurrency will set all of your listings' skus to have a quantity of zero instead of whatever they had before, which sets them to track inventory with an inventory of 0, making them unbuyable.

If they have a coupon, it also removes the discountCode entirely, which makes the coupon invalid.

This is a fairly serious bug, in my opinion.

hoffmabc · 2019-01-26T12:19:22Z

The reason this bug occurs is that we perform this bulk update incorrectly.

The way it currently works is:

It pulls in a listing off disk
Update the currencies in the JSON
Update the listing by sending that to the API

This causes a problem because what's on disk is not what actually should get sent to the API. It's missing inventory and coupon information.

I will go ahead and update this code to perform this properly.

hoffmabc · 2019-01-26T18:17:21Z

Ok @jjeffryes it looks like this is now considering the inventory and coupons. It wasn't as bad as I thought it would be. @placer14 we need a double check to make sure we're not doing anything else wrong here as we completely overlooked that it wasn't putting the coupons and inventory in there.

jjeffryes · 2019-01-28T15:19:39Z

This seems to be working as intended now.

hoffmabc · 2019-01-28T15:20:13Z

Great. I just need to apply the other fixes @placer14 suggested.

hoffmabc · 2019-01-30T20:34:46Z

I think we should also suspend the root hash updater while this bulk update is completing. Otherwise, each individual listing update will trigger the root hash to be updated in the push nodes and create a lot of network chatter. The root hash could be resumed after the bulk update and the final merkle root could be enough to cover all the changes.

This latest commit should defer the publish until everything is completed.

hoffmabc · 2019-01-30T20:48:28Z

@jjeffryes ready for testing again with updates.

@placer14 what else is needed here?

placer14

Some thoughts on how to approach the testing.

placer14 · 2019-02-15T15:16:23Z

core/moderation.go

+		return err
+	}
+
+	n.SeedNode()


This returns an error that needs to be checked and handled.

placer14 · 2019-02-15T15:29:24Z

core/moderation.go

+
+		}
+		return nil
+	}


Looking over the code again, my biggest concern is the amount of behavior we have inside of walkpath that is not verified. (https://github.com/OpenBazaar/openbazaar-go/pull/1423/files#diff-3ef083f7462052e03c5b8d5fed226805R188) The API test verifies the response, but doesn't look at the data to make sure the walkpath functionality worked properly. And it touches a bunch of points.

Two ways to solve this:
The harder (but preferred) of the ways to solve this would be pulling out each piece of behavior and unit testing the individual behavior. Then you can call the methods and be confident they are doing the right thing. You wouldn't need to test in the API layer and could have tighter tests around each specific behavior by itself. Bonus points if you walk through the code and replace other matching uses with your extracted/tested version.

The other easier way to solve this (though costs more in the longterm) would be by crafting a listing which exercises all of these features. A proper exercise would ensure:

sl.Listing.Metadata.AcceptedCurrencies = currencies was changed to a new value from an old different value

that coupons were properly reapplied to the listing

that shippingService.AdditionalItemPrice is set properly on legacy listings (how did you know to check this?)

that inventory is properly reapplied to the listing
If you could craft such a perfect listing, you could simply do a GET /listing and ensure the above items match. (you might need to do more with inventory checking...)

A magic listing test runs the risk of not creating the example to sufficiently catch any regressions which may happen later (an impossible task). Another reason the unit tests would be preferred, as regressions in one part wouldn't necessarily be hard to fix across the codebase once it occurs.

Ok I believe the 100% coverage on this stuff should cover your requests.

Skeleton for signed listings tests Unit tests for signed listings [1] Additional unit tests signed listings

hoffmabc · 2019-02-16T21:16:13Z

Ready for review @placer14

hoffmabc · 2019-02-25T16:18:13Z

GTG @cpacia

Bulk Update Currencies for Listings

62dbe72

hoffmabc added the feature Feature or enhancement to openbazaar-go label Jan 23, 2019

hoffmabc self-assigned this Jan 23, 2019

hoffmabc requested review from placer14 and cpacia January 23, 2019 16:17

Check to make sure file is .json

e765302

placer14 approved these changes Jan 23, 2019

View reviewed changes

api/jsonapi.go Outdated Show resolved Hide resolved

hoffmabc added 2 commits January 23, 2019 12:11

Handle error

86e05a2

Refactor to use updateListing

8f2f983

jjeffryes mentioned this pull request Jan 23, 2019

Add Bulk Coin Accepted Control OpenBazaar/openbazaar-desktop#1683

Closed

Remove unnecessary updatelistingonindex

113f85c

placer14 suggested changes Jan 23, 2019

View reviewed changes

hoffmabc mentioned this pull request Jan 24, 2019

[WIP] Bulk update coupons [Relies on PR #1423] #1424

Closed

hoffmabc changed the title ~~Bulk Update Currencies for Listings~~ Bulk Update Currencies for Listings [DO NOT MERGE] Jan 26, 2019

Handle coupons and inventory

cd192f2

hoffmabc changed the title ~~Bulk Update Currencies for Listings [DO NOT MERGE]~~ Bulk Update Currencies for Listings Jan 26, 2019

hoffmabc added in progress needs confirmation labels Jan 26, 2019

placer14 changed the title ~~Bulk Update Currencies for Listings~~ [WIP] Bulk Update Currencies for Listings Jan 28, 2019

jjeffryes mentioned this pull request Jan 30, 2019

Bulk coin update OpenBazaar/openbazaar-desktop#1688

Merged

Updated so publish to seed nodes is deferred

d5fff0c

Unit test for API call

de1085b

Return false success response if no currencies

b08b622

placer14 suggested changes Feb 15, 2019

View reviewed changes

hoffmabc added 7 commits February 15, 2019 15:02

Migrate to listings.go

80df03a

Handle error on publish

d734ef7

Refactoring signed listing methods

4992ae3

Create refactored signed_listings file

fb88b66

Move SL methods to new file

55f7c96

Adding example contracts for unit testing

00c3ca1

Skeleton for signed listings tests Unit tests for signed listings [1] Additional unit tests signed listings

Refactor unit tests for signed listings

6158289

hoffmabc force-pushed the update-currencies branch from 4dc7753 to 6158289 Compare February 16, 2019 21:12

hoffmabc changed the title ~~[WIP] Bulk Update Currencies for Listings~~ Bulk Update Currencies for Listings Feb 16, 2019

cpacia approved these changes Feb 25, 2019

View reviewed changes

cpacia added needs rebase and removed in progress labels Feb 25, 2019

Merge branch 'master' into update-currencies

4499e3a

cpacia merged commit c92e4d4 into master Feb 25, 2019

placer14 deleted the update-currencies branch March 21, 2019 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bulk Update Currencies for Listings #1423

Bulk Update Currencies for Listings #1423

hoffmabc commented Jan 23, 2019

cpacia commented Jan 23, 2019

coveralls commented Jan 23, 2019 •

edited

Loading

placer14 left a comment

placer14 left a comment

jjeffryes commented Jan 24, 2019

jjeffryes commented Jan 25, 2019

hoffmabc commented Jan 26, 2019

hoffmabc commented Jan 26, 2019

jjeffryes commented Jan 28, 2019

hoffmabc commented Jan 28, 2019

hoffmabc commented Jan 30, 2019

hoffmabc commented Jan 30, 2019

placer14 left a comment

placer14 Feb 15, 2019

placer14 Feb 15, 2019

placer14 Feb 15, 2019

hoffmabc Feb 16, 2019

hoffmabc commented Feb 16, 2019

hoffmabc commented Feb 25, 2019

Bulk Update Currencies for Listings #1423

Bulk Update Currencies for Listings #1423

Conversation

hoffmabc commented Jan 23, 2019

cpacia commented Jan 23, 2019

coveralls commented Jan 23, 2019 • edited Loading

placer14 left a comment

Choose a reason for hiding this comment

placer14 left a comment

Choose a reason for hiding this comment

jjeffryes commented Jan 24, 2019

jjeffryes commented Jan 25, 2019

hoffmabc commented Jan 26, 2019

hoffmabc commented Jan 26, 2019

jjeffryes commented Jan 28, 2019

hoffmabc commented Jan 28, 2019

hoffmabc commented Jan 30, 2019

hoffmabc commented Jan 30, 2019

placer14 left a comment

Choose a reason for hiding this comment

placer14 Feb 15, 2019

Choose a reason for hiding this comment

placer14 Feb 15, 2019

Choose a reason for hiding this comment

placer14 Feb 15, 2019

Choose a reason for hiding this comment

hoffmabc Feb 16, 2019

Choose a reason for hiding this comment

hoffmabc commented Feb 16, 2019

hoffmabc commented Feb 25, 2019

coveralls commented Jan 23, 2019 •

edited

Loading