[WIP][www] Add notification mechanism #393

fernandoabolafio · 2018-07-18T21:43:30Z

This PR implements a notification mechanism for www. So, every relevant action generates a new notification into the user's mailbox.
closes #372

More information about the implementation design and current status can be found here:
https://gist.github.com/fernandoabolafio/0d13902f05294eb83480b1dab11e92c6

note: This PR refers to the stage 1 mentioned on the issue #372.

Update 1: New Routes specs

user/notifications -> Return all users notifications (user is infered from the session)
Example of response:

{
   "notifications":[
      {
         "notificationid":18,
         "notificationtype":2,
         "timestamp":1533204654,
         "contextinfo":[
            "20"
         ],
         "viewed":false
      },
      {
         "notificationid":19,
         "notificationtype":4,
         "timestamp":1533213456,
         "contextinfo":[
            "0f9a30377a891e08d5ee15bd7cdd648508baadfcd923df72f1920d425cb9d6bd"
         ],
         "viewed":false
      },
      {
         "notificationid":20,
         "notificationtype":4,
         "timestamp":1533213557,
         "contextinfo":[
            "df33cce806d0e0286ed136d686d5226a26ab8fe3ef3dd3b1ad02556ac9c75a6f"
         ],
         "viewed":false
      }
   ]
}

user/notifications/check: set one or multiple notifications as viewed
Example of request:

{
	"notificationids": [18, 19]
}

Example of response:

{
   "notifications":[
      {
         "notificationid":18,
         "notificationtype":2,
         "timestamp":1533204654,
         "contextinfo":[
            "20"
         ],
         "viewed": true
      },
      {
         "notificationid":19,
         "notificationtype":4,
         "timestamp":1533213456,
         "contextinfo":[
            "0f9a30377a891e08d5ee15bd7cdd648508baadfcd923df72f1920d425cb9d6bd"
         ],
         "viewed": true
      },
      {
         "notificationid":20,
         "notificationtype":4,
         "timestamp":1533213557,
         "contextinfo":[
            "df33cce806d0e0286ed136d686d5226a26ab8fe3ef3dd3b1ad02556ac9c75a6f"
         ],
         "viewed":false
      }
   ]
}

Update 2: Context info mapping

Notification type	Context info value
5 - proposal voting started	["censorshiptoken"]
4 - proposal published	["censorshiptoken"]
3 - proposal censored	["censorhiptoken"]
2 - proposal paywall paid	["amount_of_credits"]
1 - signup paywall paid	none
0 - invalid	none

sndurkin

Thanks for this PR, I see you've done a lot of work on it already. I didn't do a full review, I just looked over about 3/4 of the changes so far and made some comments inline.

It may already be handled in your implementation (I'm not sure), but one general concern I had was how many notifications will we send to the user, and how many will we store in the db? Maybe a maximum of 5-10 at a time?

sndurkin · 2018-07-21T04:52:04Z

politeiawww/api/v1/v1.go

+	NotTypeProposalPublished             NotificationType = 3
+	NotTypeProposalStartedVoting         NotificationType = 4
+	NotTypeProposalFinishedVoting        NotificationType = 5
+	NotTypeInvalid                       NotificationType = 6


The invalid type's value should be 0.

sndurkin · 2018-07-21T04:52:55Z

politeiawww/api/v1/v1.go

@@ -147,6 +150,15 @@ const (
 	PropVoteStatusStarted     PropVoteStatusT = 2 // Proposal vote has been started
 	PropVoteStatusFinished    PropVoteStatusT = 3 // Proposal vote has been finished
 	PropVoteStatusDoesntExist PropVoteStatusT = 4 // Proposal doesn't exist
+
+	// Notification indentifiers
+	NotTypeSignupPaywallPaymentConfirmed NotificationType = 0


I would prefer to prefix these with Notification instead of NotType, since NotType is a little confusing out of context. Also, can you rename the type to NotificationT to be consistent with the others?

sndurkin · 2018-07-21T04:53:15Z

politeiawww/api/v1/v1.go

@@ -219,6 +231,17 @@ var (
 		PropVoteStatusFinished:    "voting finished",
 		PropVoteStatusDoesntExist: "proposal does not exist",
 	}
+
+	// NotificationIdentifier converts a Notification Id to human readable text
+	NotificationIndentifier = map[NotificationType]string{


Typo, should be NotificationIdentifier

sndurkin · 2018-07-21T04:54:05Z

politeiawww/api/v1/v1.go

+
+	// NotificationIdentifier converts a Notification Id to human readable text
+	NotificationIndentifier = map[NotificationType]string{
+		NotTypeSignupPaywallPaymentConfirmed: "Paywall payment for Politeia access confirmed",


This is very nitpicky but can you make the strings lowercase to be consistent with the others?

sndurkin · 2018-07-21T04:59:11Z

politeiawww/database/localdb/localdb.go

+	n.Viewed = false
+	n.Timestamp = time.Now().Unix()
+	// Check if the user has already a mailbox registered
+	exists, err = l.userdb.Has(key, nil)


Couldn't we store the notifications themselves within the user object?

We can actually. The way I did was totally an implementation option in order to have a flatter design but on the other hand, having it within the user object would reduce the number of database queries. Since we plan to move www database to SQL at some point, I think that having them under different keys can simplify the migration. Having the notifications collection inside the user object feels more like a noSQL approach. What do you think?

Yeah that's true, if we migrate to sql then we would use a separate table.

fernandoabolafio · 2018-07-21T11:57:24Z

Thanks for your review @sndurkin. I will address your inline comments and I let you know.

About your concern of how many notifications should be stored, I didn't handle it yet. I think that 10 is a good number to start with, we can always increase that later as we see a need for that. Especially when comments notifications come into play (Not in this PR).

tiagoalvesdulce · 2018-07-30T15:56:54Z

Can you change the response so it returns notifications instead of Notifications? Uppercase keys are a nono in JSON.

tiagoalvesdulce · 2018-07-30T18:10:07Z

{"notifications":[{"notificationid":0,"NotificationType":5,"timestamp":1532974112,"url":"/proposals/d305ee6f404ec555c1b19624f952b1184a5d774bed4334c11fa0c22aecd9afa0","viewed":false},{"notificationid":1,"NotificationType":2,"timestamp":1532974117,"viewed":false}]}

The request is returning NotificationType instead of notificationtype

tiagoalvesdulce · 2018-07-30T19:30:40Z

The response for marking a notification as viewed doesn't match the example. See the example below:

{"notifications":
  [{
    "notificationid":0,
      "NotificationType":0,
     "timestamp":0,
     "viewed":false
  },{
  "notificationid":1,
  "NotificationType":2,
  "timestamp":1532974117,
  "viewed":true}
]}

Here I marked notification 1 as viewed, works nice. But it messed up with the info for notification 0. I would say we should return the right info or doesn't return anything, just a success message.

fernandoabolafio · 2018-07-30T20:06:58Z

Thanks for the review @tiagoalvesdulce. Both of your comments are addressed and it should be working now.

sndurkin

I've done some initial testing and this notification system is looking good, nice job! I left a couple comments inline.

sndurkin · 2018-08-01T03:56:10Z

politeiawww/database/database.go

+	ID               uint64 // Notification Id
+	NotificationType int    // Type of the notification
+	Timestamp        int64  // When the notification was created
+	URL              string // URL associated to the notification


Instead of constructing a PoliteiaGUI URL (which increases the dependence www has on it), I think we should pass an array of string params, which can change depending on the notification type. And then we let the GUI build the URL using the params.

So for the proposal-related notifications, we can pass the token as the only param., and for the payment received for proposal credits, we can pass the number of proposal credits purchased, and etc.

sndurkin · 2018-08-01T04:07:22Z

politeiawww/convert.go

+func convertNotificationFromDatabase(dbnotif database.Notification) www.Notification {
+	return www.Notification{
+		NotificationID:   dbnotif.ID,
+		NotificationType: convertNotificationTypeFromDatabase(dbnotif.NotificationType),


Can't you just cast it to a NotificationT here?

I think so, good catch.

sndurkin · 2018-08-01T04:15:32Z

politeiawww/mailbox.go

+	}, email)
+}
+
+func (m *mailbox) getUserEmailByPubkey(pubkey string) (string, error) {


This is a more general utility function and probably belongs in a new user.go file or something. It might actually be better to return *database.User and then have the caller fetch the Email property. I think there are 1 or 2 places where a getUserByPubkey() function could be used, I think that logic is already being replicated elsewhere in www.

fernandoabolafio · 2018-08-01T13:10:44Z

Thanks for the review @sndurkin! I like your idea about sending a string associated to it notification instead of building the link.

fernandoabolafio · 2018-08-02T13:09:57Z

@tiagoalvesdulce @sndurkin I update this PR, you can see the new data shape in the PR description.
Now instead of returning a URL associated only to proposals notifications, it will return a field called contextinfo which is an array of string that may contain multiple parameters related to that notification. So far, notifications for proposal published/censored will contain the proposal token and the proposal paywall paid notification will now contain the number of credits bought.

@tiagoalvesdulce

Add notification on proposal paywall payment confirmed Notifications reply: set json formatting to “notifications” Fixes from @tiagoalvesdulce review Fix update notifications logic Updates from review rename mailbox.go to notifications.go and turn its functions into backend methods Indicate functions that must be called with the mutex held Add cli commands Add docs

fernandoabolafio · 2018-10-30T20:30:02Z

This is PR not applicable anymore.

Fix tests for RECEIVE_LIKE_COMMENT Save page context on login modal and fix LoginSide inputs names fix signup error

fernandoabolafio force-pushed the notifications branch 2 times, most recently from b1744ec to bc29e61 Compare July 20, 2018 21:04

sndurkin reviewed Jul 21, 2018

View reviewed changes

tiagoalvesdulce mentioned this pull request Jul 27, 2018

Add notification mechanism UI decred/politeiagui#484

Closed

fernandoabolafio force-pushed the notifications branch 2 times, most recently from e8227c3 to 7eb5f44 Compare July 27, 2018 15:06

tiagoalvesdulce mentioned this pull request Jul 30, 2018

[WIP] Add notifications mechanism UI decred/politeiagui#489

Closed

sndurkin suggested changes Aug 1, 2018

View reviewed changes

fernandoabolafio force-pushed the notifications branch from 0f072e4 to 3a1629c Compare August 2, 2018 13:06

fernandoabolafio force-pushed the notifications branch 4 times, most recently from ac060ad to 09329b5 Compare August 13, 2018 17:48

fernandoabolafio changed the title ~~[WIP][www] Add notification mechanism~~ [www] Add notification mechanism Aug 13, 2018

fernandoabolafio force-pushed the notifications branch 2 times, most recently from 87d29b7 to fda5001 Compare August 30, 2018 15:23

fernandoabolafio force-pushed the notifications branch from fda5001 to 6aaf724 Compare August 30, 2018 15:25

fernandoabolafio changed the title ~~[www] Add notification mechanism~~ [WIP][www] Add notification mechanism Sep 5, 2018

fernandoabolafio closed this Oct 30, 2018

vibros68 pushed a commit to vibros68/politeia that referenced this pull request Aug 17, 2021

Add LoginSignup Modal (decred#393)

9532650

Fix tests for RECEIVE_LIKE_COMMENT Save page context on login modal and fix LoginSide inputs names fix signup error

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP][www] Add notification mechanism #393

[WIP][www] Add notification mechanism #393

fernandoabolafio commented Jul 18, 2018 •

edited

Loading

sndurkin left a comment

sndurkin Jul 21, 2018

sndurkin Jul 21, 2018

sndurkin Jul 21, 2018

sndurkin Jul 21, 2018

sndurkin Jul 21, 2018

fernandoabolafio Jul 21, 2018 •

edited

Loading

sndurkin Jul 21, 2018

fernandoabolafio commented Jul 21, 2018 •

edited

Loading

tiagoalvesdulce commented Jul 30, 2018

tiagoalvesdulce commented Jul 30, 2018

tiagoalvesdulce commented Jul 30, 2018 •

edited

Loading

fernandoabolafio commented Jul 30, 2018

sndurkin left a comment

sndurkin Aug 1, 2018

sndurkin Aug 1, 2018

fernandoabolafio Aug 1, 2018

sndurkin Aug 1, 2018

fernandoabolafio commented Aug 1, 2018

fernandoabolafio commented Aug 2, 2018

fernandoabolafio commented Oct 30, 2018

[WIP][www] Add notification mechanism #393

[WIP][www] Add notification mechanism #393

Conversation

fernandoabolafio commented Jul 18, 2018 • edited Loading

sndurkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernandoabolafio Jul 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernandoabolafio commented Jul 21, 2018 • edited Loading

tiagoalvesdulce commented Jul 30, 2018

tiagoalvesdulce commented Jul 30, 2018

tiagoalvesdulce commented Jul 30, 2018 • edited Loading

fernandoabolafio commented Jul 30, 2018

sndurkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fernandoabolafio commented Aug 1, 2018

fernandoabolafio commented Aug 2, 2018

fernandoabolafio commented Oct 30, 2018

fernandoabolafio commented Jul 18, 2018 •

edited

Loading

fernandoabolafio Jul 21, 2018 •

edited

Loading

fernandoabolafio commented Jul 21, 2018 •

edited

Loading

tiagoalvesdulce commented Jul 30, 2018 •

edited

Loading