Fix publish issues #1574

2018-06-11T04:00:26+02:00

neb-b commented

2018-06-11 04:00:26 +02:00

(Migrated from github.com)

Fixes

https://github.com/lbryio/lbry-app/issues/1219
This kinda looks like I'm just renaming/moving stuff around but the main change for 1219 is that it was only looking for the previous claim by name and channel, not claim_id.

https://github.com/lbryio/lbry-app/issues/1520
This was fixed by ensuring there are no duplicates when we load the "you publishes" page. It will be cleared out on an interval, but there is a chance that we have the new content if you navigate to a different page then come back to "the your" publishes page.

Still some minor testing to be done, but it's ready for a first pass

## Fixes https://github.com/lbryio/lbry-app/issues/1219 This kinda looks like I'm just renaming/moving stuff around but the main change for 1219 is that it was only looking for the previous claim by name and channel, not claim_id. https://github.com/lbryio/lbry-app/issues/1520 This was fixed by ensuring there are no duplicates when we load the "you publishes" page. It will be cleared out on an interval, but there is a chance that we have the new content if you navigate to a different page then come back to "the your" publishes page. Still some minor testing to be done, but it's ready for a first pass

neb-b (Migrated from github.com) reviewed 2018-06-12 18:18:52 +02:00

src/renderer/redux/selectors/publish.js

					
				@ -26,2 +44,4 @@

				    )[0];

				  });

				// Is the current uri the same as the uri they clicked "edit" on

neb-b (Migrated from github.com) commented

2018-06-12 18:18:52 +02:00

Not really a fan of the above code. Can this be simplified?
Ensures that users are still editing if they have the same claim name, but different channels (anonymous => channel)

Not really a fan of the above code. Can this be simplified? Ensures that users are still editing if they have the same claim name, but different channels (anonymous => channel)

kauffj (Migrated from github.com) reviewed 2018-06-12 21:34:16 +02:00

src/renderer/redux/selectors/publish.js

					
				@ -26,2 +44,4 @@

				    )[0];

				  });

				// Is the current uri the same as the uri they clicked "edit" on

kauffj (Migrated from github.com) commented

2018-06-12 21:34:16 +02:00

Is this a code golf challenge? Here's my entry:

const currentName = currentIsChannel ? currentContentName : currentClaimName;
const editName = editIsChannel ? editContentName : editClaimName;
return currentName === editName;

Alternatively (and possibly contentiously):

return (currentIsChannel ? currentContentName : currentClaimName) === (editIsChannel ? editContentName : editClaimName);

Is this a code golf challenge? Here's my entry: ``` const currentName = currentIsChannel ? currentContentName : currentClaimName; const editName = editIsChannel ? editContentName : editClaimName; return currentName === editName; ``` Alternatively (and possibly contentiously): ``` return (currentIsChannel ? currentContentName : currentClaimName) === (editIsChannel ? editContentName : editClaimName); ```

neb-b (Migrated from github.com) reviewed 2018-06-12 21:59:25 +02:00

src/renderer/redux/selectors/publish.js

					
				@ -26,2 +44,4 @@

				    )[0];

				  });

				// Is the current uri the same as the uri they clicked "edit" on

neb-b (Migrated from github.com) commented

2018-06-12 21:59:25 +02:00

Much cleaner. Nice.

neb-b commented

2018-06-12 21:59:43 +02:00

(Migrated from github.com)

@kauffj Realized I left the WIP tag on here. This is ready for a full review.

@kauffj Realized I left the `WIP` tag on here. This is ready for a full review.

kauffj (Migrated from github.com) approved these changes 2018-06-12 22:56:19 +02:00

kauffj (Migrated from github.com) left a comment

I did not run this code - should I gave a UX review as well?

package.json

					
				@ -50,3 +50,3 @@

				    "keytar": "^4.2.1",

				    "lbry-redux": "lbryio/lbry-redux#7759bc6e8c482bed173d1f10aee6f6f9a439a15a",

				    "lbry-redux": "lbryio/lbry-redux#121ba56f47fff05531e27a7c99d7d417e79cd3ee",

				    "localforage": "^1.7.1",

kauffj (Migrated from github.com) commented

2018-06-12 22:44:11 +02:00

Not blocking - but what's it look like to package this properly? Does it make dev cycle more work / more annoying? Possibly worth filing.

src/renderer/component/publishForm/view.jsx

kauffj (Migrated from github.com) commented

2018-06-12 22:48:56 +02:00

It looks like the code in this section could be more succinct by just directly copying the desired properties from this.props directly to publishParams (that don't need to be modified/checked).

It looks like the code in this section could be more succinct by just directly copying the desired properties from `this.props` directly to `publishParams` (that don't need to be modified/checked).

src/renderer/page/publish/index.js

kauffj (Migrated from github.com) commented

2018-06-12 22:51:50 +02:00

null seems more correct than undefined - we know there's not one; we're not implying it will be set later

`null` seems more correct than `undefined` - we know there's not one; we're not implying it will be set later

src/renderer/redux/actions/publish.js

					
				@ -258,38 +245,40 @@ export const doPublish = (params: PublishParams) => (dispatch: Dispatch, getStat

				export const doCheckPendingPublishes = () => (dispatch: Dispatch, getState: GetState) => {

				  const state = getState();

				  const pendingPublishes = selectPendingPublishes(state);

kauffj (Migrated from github.com) commented

2018-06-12 22:53:10 +02:00

Why dispatch this for every item in the loop?

kauffj (Migrated from github.com) commented

2018-06-12 22:53:25 +02:00

Should these dispatches be grouped and bulk dispatched?

kauffj (Migrated from github.com) commented

2018-06-12 22:54:04 +02:00

reduce candidate if you want to sharpen those skills

`reduce` candidate if you want to sharpen those skills

src/renderer/redux/selectors/publish.js

					
				@ -11,3 +28,4 @@

				export const selectPublishFormValues = createSelector(selectState, state => {

				  const { pendingPublish, ...formValues } = state;

				  return formValues;

kauffj (Migrated from github.com) commented

2018-06-12 22:54:32 +02:00

Another reduce candidate

Another `reduce` candidate

src/renderer/redux/selectors/publish.js

					
				@ -26,2 +44,4 @@

				    )[0];

				  });

				// Is the current uri the same as the uri they clicked "edit" on

kauffj (Migrated from github.com) commented

2018-06-12 22:55:38 +02:00

Could be return isStillEditing ? <> : <> if that's allowed.

Could be `return isStillEditing ? <> : <>` if that's allowed.

tzarebczan commented

2018-06-13 01:03:55 +02:00

(Migrated from github.com)

The UX is not changing. I gave this a run through found 2 issues:

got into a state where claim_list_mine kept being called in network tab, very frequently. This makes everything after a publish really slow since the call locks up others.
looks like checking of existing claims while editing broke - if I type a claim name, or select a file which populates claim name, that I already have a claim for. Should see the "You already have a claim at lbry://...". I think making "Publishing will update your existing claim." stick out more is a good idea too - different color or add a warning?

The UX is not changing. I gave this a run through found 2 issues: 1) got into a state where claim_list_mine kept being called in network tab, very frequently. This makes everything after a publish really slow since the call locks up others. 2) looks like checking of existing claims while editing broke - if I type a claim name, or select a file which populates claim name, that I already have a claim for. Should see the "You already have a claim at lbry://...". I think making "Publishing will update your existing claim." stick out more is a good idea too - different color or add a warning?

neb-b (Migrated from github.com) reviewed 2018-06-13 03:59:35 +02:00

package.json

					
				@ -50,3 +50,3 @@

				    "keytar": "^4.2.1",

				    "lbry-redux": "lbryio/lbry-redux#7759bc6e8c482bed173d1f10aee6f6f9a439a15a",

				    "lbry-redux": "lbryio/lbry-redux#121ba56f47fff05531e27a7c99d7d417e79cd3ee",

				    "localforage": "^1.7.1",

neb-b (Migrated from github.com) commented

2018-06-13 03:59:34 +02:00

Not really sure. It's pretty simple right now, just build lbry-redux push the new build and grab the latest commit. I would think it would be more work to actually package it.

I think that could be valuable in the future, but while we are adding lots of stuff, I think this is working fine.

Not really sure. It's pretty simple right now, just build `lbry-redux` push the new build and grab the latest commit. I would think it would be more work to actually package it. I think that could be valuable in the future, but while we are adding lots of stuff, I think this is working fine.

neb-b (Migrated from github.com) reviewed 2018-06-13 04:18:58 +02:00

src/renderer/redux/actions/publish.js

					
				@ -258,38 +245,40 @@ export const doPublish = (params: PublishParams) => (dispatch: Dispatch, getStat

				export const doCheckPendingPublishes = () => (dispatch: Dispatch, getState: GetState) => {

				  const state = getState();

				  const pendingPublishes = selectPendingPublishes(state);