Subscribe notify #1066

2018-03-06T09:40:56+01:00

liamcardenas commented

2018-03-06 09:40:56 +01:00

(Migrated from github.com)

This notifies users whenever their is new content available for them on a channel they have subscribed to:

it checks for new content
it notifies the user that 1 or more (up to "9+") items are available per channel
it starts downloading the most recent newly available video from each channel (if it is free)
its behavior was modeled after the iPhone podcast app

TODO: badges, I could add them into this PR, or maybe make a follow up

This notifies users whenever their is new content available for them on a channel they have subscribed to: - it checks for new content - it notifies the user that 1 or more (up to "9+") items are available per channel - it starts downloading the most recent newly available video from each channel (if it is free) - its behavior was modeled after the iPhone podcast app TODO: badges, I could add them into this PR, or maybe make a follow up

neb-b (Migrated from github.com) requested changes 2018-03-06 19:38:34 +01:00

neb-b (Migrated from github.com) left a comment

There are still a lot of codacity issues.

As discussed I think ideally we would notify after the file is downloaded, but this might be a good starting point. Another idea might just be to set a timeout before sending the notification, just to give a file a little more time to download. But to me that just seems like a hack to avoid notifying after the download is complete

There are still a lot of codacity issues. As discussed I think ideally we would notify after the file is downloaded, but this might be a good starting point. Another idea might just be to set a timeout before sending the notification, just to give a file a little more time to download. But to me that just seems like a hack to avoid notifying after the download is complete

src/renderer/page/file/view.jsx

					
				@ -17,6 +17,7 @@ class FilePage extends React.PureComponent {

				  componentDidMount() {

				    this.fetchFileInfo(this.props);

				    this.fetchCostInfo(this.props);

				    this.checkSubscriptionLatest(this.props);

neb-b (Migrated from github.com) commented

2018-03-06 19:17:38 +01:00

You can avoid a lot of unnecessary calls by only calling checkSubscriptionLatest if you are subscribed to that channel

You can avoid a lot of unnecessary calls by only calling `checkSubscriptionLatest` if you are subscribed to that channel

src/renderer/redux/actions/subscriptions.js

neb-b (Migrated from github.com) commented

2018-03-06 19:13:27 +01:00

Why do you need to pass in the new cost object. Shouldn't doPurchaseUri be able to select that data with the uri?

Why do you need to pass in the new cost object. Shouldn't `doPurchaseUri` be able to select that data with the uri?

src/renderer/redux/actions/subscriptions.js

					
				@ -17,0 +90,4 @@

				      };

				    }

				    //$FlowIssue

neb-b (Migrated from github.com) commented

2018-03-06 18:40:52 +01:00

I think this should have a case for count === 1, which is probably the most likely scenario. I think in that case just showing the title would make the most sense.

I think this should have a case for `count === 1`, which is probably the most likely scenario. I think in that case just showing the title would make the most sense.

kauffj (Migrated from github.com) reviewed 2018-03-06 21:56:52 +01:00

kauffj (Migrated from github.com) left a comment

Light feedback, did not run code

src/renderer/redux/actions/subscriptions.js

kauffj (Migrated from github.com) commented

2018-03-06 21:55:55 +01:00

Can we wait to show this notification until after content is ready to watch? I.e. after get has successfully started? That way when the user clicks the notification they get something immediately.

Can we wait to show this notification until after content is ready to watch? I.e. after `get` has successfully started? That way when the user clicks the notification they get something immediately.

src/renderer/redux/actions/subscriptions.js

					
				@ -17,0 +113,4 @@

				        false

				      ) === uri

				    ) {

				      dispatch(setSubscriptionLatest(channel, uri));

kauffj (Migrated from github.com) commented

2018-03-06 21:53:36 +01:00

Is this function actually necessary? Could doCheckSubscription just call setSubscriptionLatest?

Is this function actually necessary? Could `doCheckSubscription` just call `setSubscriptionLatest`?

liamcardenas (Migrated from github.com) reviewed 2018-03-07 20:26:06 +01:00

src/renderer/redux/actions/subscriptions.js

					
				@ -17,0 +90,4 @@

				      };

				    }

				    //$FlowIssue

liamcardenas (Migrated from github.com) commented

2018-03-07 20:26:06 +01:00

it does, it just says

"Sean"
"Posted myVideo"

instead of, for example,

"Sean"
"Posted myVideo and 9+ other new items"

it does, it just says ``` "Sean" "Posted myVideo" ``` instead of, for example, ``` "Sean" "Posted myVideo and 9+ other new items" ```

liamcardenas (Migrated from github.com) reviewed 2018-03-07 20:27:56 +01:00

src/renderer/redux/actions/subscriptions.js

liamcardenas (Migrated from github.com) commented

2018-03-07 20:27:56 +01:00

its is more efficient this way because I already have the cost information so we don't need to fetch it again and

liamcardenas (Migrated from github.com) reviewed 2018-03-07 20:29:46 +01:00

src/renderer/page/file/view.jsx

					
				@ -17,6 +17,7 @@ class FilePage extends React.PureComponent {

				  componentDidMount() {

				    this.fetchFileInfo(this.props);

				    this.fetchCostInfo(this.props);

				    this.checkSubscriptionLatest(this.props);

liamcardenas (Migrated from github.com) commented

2018-03-07 20:29:46 +01:00

good call

liamcardenas (Migrated from github.com) reviewed 2018-03-07 20:34:27 +01:00

src/renderer/redux/actions/subscriptions.js

					
				@ -17,0 +113,4 @@

				        false

				      ) === uri

				    ) {

				      dispatch(setSubscriptionLatest(channel, uri));

liamcardenas (Migrated from github.com) commented

2018-03-07 20:34:27 +01:00

check and set are different, set simply sets the value in the redux store. Check does an asynchronous call and sets the value accordingly by dispatching set, if that makes sense.

liamcardenas (Migrated from github.com) reviewed 2018-03-07 20:37:11 +01:00

src/renderer/redux/actions/subscriptions.js

liamcardenas (Migrated from github.com) commented

2018-03-07 20:37:11 +01:00

Yes, @seanyesmunt brought this up as well. I have reasons for why I didn't make it this way, which I think we should discuss -- although I think you are right that we probably want to download it first and I probably should have implemented it this way. I will schedule this in the next sprint.

kauffj (Migrated from github.com) reviewed 2018-03-07 22:51:27 +01:00

src/renderer/redux/actions/subscriptions.js

kauffj (Migrated from github.com) commented

2018-03-07 22:51:26 +01:00

I checked out the code here:

doLoadVideo and doPurchaseUri are confusingly -- almost backwardly -- named. It may make sense to tweak these.
Since you are already checking that the price is free, it seems like you could potentially skip doPurchaseUri and go right to doLoadVideo.
Both doLoadVideo and doPurchaseUri can trigger modal popups. It is weird that asynchronous background loading of content can trigger a modal. Fairly guaranteed that this will cause confusion at some point.
It doesn't seem particularly tricky to attach a notification to the success of the call to get. I think a light modification of doLoadVideo to take a notification or callback would get you there.

I checked out the code here: 1) `doLoadVideo` and `doPurchaseUri` are confusingly -- almost backwardly -- named. It may make sense to tweak these. 2) Since you are already checking that the price is free, it seems like you could potentially skip `doPurchaseUri` and go right to `doLoadVideo`. 3) Both `doLoadVideo` and `doPurchaseUri` can trigger modal popups. It is weird that asynchronous background loading of content can trigger a modal. Fairly guaranteed that this will cause confusion at some point. 4) It doesn't seem particularly tricky to attach a notification to the success of the call to `get`. I think a light modification of `doLoadVideo` to take a notification or callback would get you there.

kauffj (Migrated from github.com) reviewed 2018-03-07 22:58:38 +01:00

src/renderer/redux/actions/subscriptions.js

					
				@ -17,0 +113,4 @@

				        false

				      ) === uri

				    ) {

				      dispatch(setSubscriptionLatest(channel, uri));

kauffj (Migrated from github.com) commented

2018-03-07 22:58:38 +01:00

I'm asking two things in my question:

If we're already fetching the latest subscription data at time interval n, is it actually necessary to check for the latest for this same information when accessing a file page?
If it is necessary (for staleness reasons, presumably), why can't more of the code be shared between the two? Isn't doCheckSubscription a superset of the functionality provided by checkSubscriptionLatest? and why wouldn't you want checkSubscriptionLatest to also begin downloads? Furthermore, why do both of these call Lbry.claim_list_by_channel when they could call doFetchClaimsByChannel instead?

I'm asking two things in my question: 1) If we're already fetching the latest subscription data at time interval `n`, is it actually necessary to check for the latest for this same information when accessing a file page? 2) If it is necessary (for staleness reasons, presumably), why can't more of the code be shared between the two? Isn't `doCheckSubscription` a superset of the functionality provided by `checkSubscriptionLatest`? and why wouldn't you want `checkSubscriptionLatest` to also begin downloads? Furthermore, why do _both_ of these call `Lbry.claim_list_by_channel` when they could call `doFetchClaimsByChannel` instead?

kauffj (Migrated from github.com) reviewed 2018-03-09 00:04:47 +01:00

src/renderer/redux/actions/subscriptions.js

					
				@ -17,0 +113,4 @@

				        false

				      ) === uri

				    ) {

				      dispatch(setSubscriptionLatest(channel, uri));

kauffj (Migrated from github.com) commented

2018-03-09 00:04:47 +01:00

@liamcardenas did you see above?

Sign in to join this conversation.

No reviewers

neb-b

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbry-desktop#1066