allow claims to be updated and abandoned #595
No reviewers
Labels
No labels
area: devops
area: discovery
area: docs
area: livestream
area: proposal
consider soon
dependencies
Epic
good first issue
hacktoberfest
help wanted
icebox
level: 1
level: 2
level: 3
level: 4
needs: exploration
needs: grooming
needs: priority
needs: repro
needs: tech design
on hold
Osprey
priority: blocker
priority: high
priority: low
priority: medium
protocol dependent
resilience
Tom's Wishlist
type: bug
type: discussion
type: error handling
type: improvement
type: new feature
type: refactor
type: task
type: testing
unplanned
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: LBRYCommunity/spee.ch#595
Loading…
Reference in a new issue
No description provided.
Delete branch "update-claims"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
#482
#554
Nothing too major
You've repeated this code a few times for getting the
asset
, I'd suggest creating a util methodSee above comment about adding a util
Please use spread operators to clean this up.
e.g.:
Same as above, use spread operators
Should use consistent formatting (I know, the existing code is inconsistent)
@ -0,0 +9,4 @@
const { claimData, history } = action.data;
const { claimId } = claimData;
const confirm = window.confirm('Are you sure you want to abandon this claim? This action cannot be undone.');
This works for now, but we should have some common prompt to match Spee.ch.
This logic exists in both conditions, can you move it out?
Can move this out as
publishChannel = yield call(makePublishRequestChannel, publishFormData, isUpdate);
You have this validation in more than one place, please combine
Looks good overall
@ -14,6 +14,19 @@ export function clearFile () {
};
}
export function setUpdateTrue () {
Is this necessary? My inclination is that there must be another state value corresponding to what is being edited (or even however you ended up on the edit page) that should allow you to know this without an additional explicit value.
@ -42,3 +42,15 @@ export function getClaimViews (claimId) {
const url = `/api/claim/views/${claimId}`;
return Request(url);
}
DRY
@ -6,3 +5,4 @@
const uri = `/api/claim/${isUpdate ? 'update' : 'publish'}`;
const xhr = new XMLHttpRequest();
// add event listeners
const onLoadStart = () => {
I know you didn't add this, but you should know we can just use the Fetch API.
@ -1,5 +1,6 @@
export const LOAD_START = 'LOAD_START';
export const LOADING = 'LOADING';
should be
SUCCEEDED
fwiwif this isn't being fixed before shipping, it needs to be filed, and it's a good practice to link the issue as an inline comment
@ -4,1 +4,4 @@
import { LOCAL_CHECK, UNAVAILABLE, ERROR, AVAILABLE } from '../../constants/asset_display_states';
import createCanonicalLink from '../../../../utils/createCanonicalLink';
class AvailableContent extends React.Component {
good refactor!
better to have this out of the h3 ("edit" isn't part of the title for SEO / accessibility purposes, etc.)
@ -0,0 +27,4 @@
asset.claimsData.channelName !== myChannel
)
) {
return (<Redirect to={'/'} />);
Do we need to redirect to login? Show an error? How would this happen to a user genuinely?
(one guess: sign in in one tab, then sign in a second tab, and click edit from the first tab)
@ -14,4 +10,1 @@
}
}
render () {
Why does this have to happen here? Does this have to happen for any component that includes
<PublishTool />
? If so, this should be avoided.@ -0,0 +8,4 @@
route to abandon a claim through the daemon
*/
const claimAbandon = async (req, res) => {
This function looks great! Looking forward to more like this
Do these need to be in a transaction if they need to both execute atomically?
Can be filed, but this is a rather important value that ought to be hoisted all the way to a site-specific config
@ -14,6 +14,19 @@ export function clearFile () {
};
}
export function setUpdateTrue () {
I think it would be more complex to do this another way e.g. looking at the router path
@ -0,0 +27,4 @@
asset.claimsData.channelName !== myChannel
)
) {
return (<Redirect to={'/'} />);
I was only considering a nefarious user. I don't think this matters much; just needs to not render the edit page.
@ -14,4 +10,1 @@
}
}
render () {
Nice catch/good point! Current speech only clears the file for file errors, reset, publish success, and cancel publish, so two features using the same
state.publish
were stepping on each others toesMy new approach uses
isUpdate
When the publish page mounts: if
isUpdate
thenclearFile
When edit page mounts: if
!isUpdate
thenclearFile, setUpdateTrue, setMetadata
This way a user can start a publish or update, leave the page, and come back to where they were, like current speech, unless they go to the other (Edit/Publish) page, in which case it starts fresh with publish or update
code
@ -14,4 +10,1 @@
}
}
render () {
👍
#609
Is this ready to test or are we waiting on more daemon changes?
the daemon is good now but I still have a few more things to address, I'll let you know
store
asset.claimData
in a local var since it's use many times@ -129,2 +146,2 @@
)}
</div>
</div>
)}
Can use spread operator for switching props, e.g.:
Should we order these?
use the aliases
use aliases
@ -18,2 +18,4 @@
const authenticateUser = require('./authentication.js');
const chainquery = require('chainquery');
const createCanonicalLink = require('../../../../../utils/createCanonicalLink');
Use aliases
Use aliases
I'd much prefer to avoid single line
if
logic, I find it reduced legibility.This is for a single claim. I need to do a lookup by channel_id. This can be more generalized in the near future.