Skip to content
This repository has been archived by the owner on Jan 14, 2020. It is now read-only.

Client-side notifications #222

Merged
merged 21 commits into from
Jun 9, 2018
Merged

Client-side notifications #222

merged 21 commits into from
Jun 9, 2018

Conversation

tyleryasaka
Copy link
Contributor

@tyleryasaka tyleryasaka commented Jun 1, 2018

Checklist:

  • Code contains relevant tests for the problem you are solving
  • Ensure all new and existing tests pass
  • Update any relevant READMEs and docs
  • Submit to the develop branch instead of master

Description:

This creates an API for client-side notifications with no dependencies other than local storage. Notifications are built directly from blockchain data, and read/unread state is stored in local storage.

I implemented the following notification types:

  • For seller:
    • seller_listing_purchased
    • seller_review_received
  • For buyer:
    • buyer_listing_shipped
    • buyer_review_received

This can be adjusted/expanded in the future as needed.

Closes #211

@micahalcorn
Copy link
Member

micahalcorn commented Jun 6, 2018

Thanks, @tyleryasaka 👏 Really great work.

I am getting two notifications with the same id when I call notifications.all from account 0x627306090abaB3A6e1400e9345bC60c78a8BEf57. The duplicate notification is as follows:

{
  id: "seller_listing_purchased_0x23172349165f59c95567e1e993e473ba914d0dd4fe59ee398b051cfda4cea622",
  type: "seller_listing_purchased",
  status: "read"
}

Also, I'm going to need to map the role of the user to this object in addition to obviously getting the relevant data (listing details, purchase details, etc) to produce a friendly message. Should any of this be done on the origin-js side since you already seem to have strung the necessary async calls together?

Thoughts evolved below 👇

@@ -38,6 +38,7 @@
"form-data": "^2.3.2",
"map-cache": "^0.2.2",
"rlp": "^2.0.0",
"store": "^2.0.12",
Copy link
Member

Choose a reason for hiding this comment

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

👍

@micahalcorn
Copy link
Member

micahalcorn commented Jun 6, 2018

@tyleryasaka here is my WIP branch: https://github.com/OriginProtocol/demo-dapp/tree/notifications

Since we aren't going to be "indexing notifications" on the bridge server, then I'm wondering what is the best way to limit the number of requests for this information. Perhaps it makes sense to put all of a user's listings and purchases in the DApp redux store as soon as they load the first page. Then we can probably map all of my content described above using the stored collections. That would effectively cache all of the other routes too.

Maybe we at least return the listingId and/or purchaseId in the notifications API response?

@tyleryasaka
Copy link
Contributor Author

@micahalcorn The duplicate notification seems to have had something to do with a bug in the purchase logs. I refactored that solidity code and I'm not seeing duplicate notifications any more. Let me know if you have any more issues with that.

Sorry about leaving out listing and purchase in the notification object! That would be helpful to have. They're there now, under the resources property. ✅

Yeah, on the react side you should probably implement some sort of caching. Redux store seems like the way to do that.

I think what I have now should address all the concerns you mentioned - let me know if not or if you need anything else!

@micahalcorn micahalcorn merged commit 28299fb into develop Jun 9, 2018
@tyleryasaka tyleryasaka deleted the tyleryasaka/notifications branch June 9, 2018 06:20
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.

None yet

2 participants