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

Manually send order messages and trigger offline message scan #1584

Merged
merged 27 commits into from
Jul 29, 2019

Conversation

amangale
Copy link
Collaborator

@amangale amangale commented May 17, 2019

This PR adds order_messages table which is used to store order related messages for a node. It also adds two api end points : GET /ob/scanofflinemessages and POST /ob/sendordermessage to manually trigger offline message scan and send any previously sent order message of type - Message_ORDER, Message_ORDER_REJECT, Message_ORDER_CANCEL, Message_ORDER_CONFIRMATION, Message_ORDER_FULFILLMENT, Message_ORDER_COMPLETION.

Fixes #1459
Fixes #1584

@amangale amangale requested review from placer14 and cpacia May 17, 2019 11:55
@amangale amangale self-assigned this May 17, 2019
@hoffmabc hoffmabc mentioned this pull request May 20, 2019
@placer14
Copy link
Member

Initial conversation around this improvement suggests we will move forward with this PR, pending final peer review of the implementation.

Copy link
Member

@placer14 placer14 left a comment

Choose a reason for hiding this comment

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

Some technical thoughts being mindful of using this solution beyond order messages. Nice schema design upfront. 🤝

api/jsonapi.go Outdated Show resolved Hide resolved
api/endpoints.go Outdated Show resolved Hide resolved
api/jsonapi.go Outdated Show resolved Hide resolved
repo/datastore.go Outdated Show resolved Hide resolved
repo/db/order_messages.go Outdated Show resolved Hide resolved
repo/db/order_messages.go Outdated Show resolved Hide resolved
schema/constants.go Outdated Show resolved Hide resolved
schema/constants.go Show resolved Hide resolved
@placer14 placer14 self-assigned this May 20, 2019
Copy link
Member

@placer14 placer14 left a comment

Choose a reason for hiding this comment

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

Some changes required here. Please also consider rewriting the godocs so they don't have a hyphen in their description. They should look like:

// FunctionName does some sort of behavior and returns values

Thank you!

core/net.go Outdated Show resolved Hide resolved
core/net.go Outdated Show resolved Hide resolved
core/net.go Outdated Show resolved Hide resolved
core/net.go Outdated Show resolved Hide resolved
core/net.go Outdated Show resolved Hide resolved
repo/migrations/Migration024.go Outdated Show resolved Hide resolved
repo/migrations/Migration024.go Outdated Show resolved Hide resolved
repo/migrations/Migration024.go Outdated Show resolved Hide resolved
repo/migrations/Migration024.go Outdated Show resolved Hide resolved
repo/migrations/Migration024_test.go Show resolved Hide resolved
@amangale amangale requested a review from placer14 July 10, 2019 16:56
Copy link
Member

@placer14 placer14 left a comment

Choose a reason for hiding this comment

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

Another round of changes to consider.

core/net.go Outdated Show resolved Hide resolved
core/net.go Outdated Show resolved Hide resolved
core/net.go Outdated Show resolved Hide resolved
core/net.go Outdated Show resolved Hide resolved
repo/message_test.go Show resolved Hide resolved

fmt.Println(string(retMsg.GetPayload().Value), " ", peer)

if !(string(retMsg.GetPayload().Value) == payload) {
Copy link
Member

Choose a reason for hiding this comment

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

You missed this one.

repo/db/messages.go Outdated Show resolved Hide resolved
repo/datastore.go Outdated Show resolved Hide resolved
schema/constants.go Outdated Show resolved Hide resolved
Copy link
Member

@placer14 placer14 left a comment

Choose a reason for hiding this comment

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

The only other concern which was unaddressed is https://github.com/OpenBazaar/openbazaar-go/pull/1584/files#r301834135 but not worth holding this PR longer. Excellent work here, @amangale.

Copy link
Member

@placer14 placer14 left a comment

Choose a reason for hiding this comment

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

Caught a bug while testing. Please check the comments.

core/net.go Outdated Show resolved Hide resolved
core/net.go Outdated Show resolved Hide resolved
core/net.go Outdated Show resolved Hide resolved
core/net.go Outdated Show resolved Hide resolved
Copy link
Member

@placer14 placer14 left a comment

Choose a reason for hiding this comment

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

Some minor nitpicks.

api/jsonapi.go Outdated Show resolved Hide resolved
api/jsonapi.go Outdated Show resolved Hide resolved
api/jsonapi.go Outdated Show resolved Hide resolved
@placer14 placer14 merged commit 3da783d into OpenBazaar:master Jul 29, 2019
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.

ACKs for Some or All Direct Messages
2 participants