optional-doi: fix new upload disabled states #1932
Conversation
zzacharo
force-pushed
the
fix-doi-disabled-new-upload
branch
2 times, most recently
from
1a2e600 to
7cdcedd
Compare
| } = this.props; | ||
|
|
||
| const doi = record?.pids?.doi?.identifier || ""; | ||
| const isNewUpload = record.status === "draft"; |
There was a problem hiding this comment.
Minor: We could user this to avoid hardcoding the state.
Also is isNewUpload a correct name? just find it a bit confusing 😅 Does this mean that after the first version is published if we create a new version it doesn't has the draft status?
EDIT: It actually seems that new versions do have new_version_draft, in that case I guess it's fine 👍
There was a problem hiding this comment.
no actually it has status=='new_version_draft'. Thanks for the suggestion, I will integrate it now!
zzacharo
force-pushed
the
fix-doi-disabled-new-upload
branch
from
7cdcedd to
64bbb95
Compare
| } = this.props; | ||
|
|
||
| const doi = record?.pids?.doi?.identifier || ""; | ||
| const alreadyPublished = [ |
There was a problem hiding this comment.
@jrcastro2 I changes to this. Better?
There was a problem hiding this comment.
Yes, thanks! more readable IMO 🚀
| DepositStatus.NEW_VERSION_DRAFT, | ||
| ].includes(record.status); | ||
|
|
||
| const hasDoi = doi !== ""; |
There was a problem hiding this comment.
minor: wouldn't isEmpty be safer?
There was a problem hiding this comment.
why safer? I agree it would be more elegant but I think also if we were to replace it would need more testing as the isEmpty would catch all falsy values...If you think is major, I could have a look!
| record, | ||
| } = this.props; | ||
|
|
||
| const doi = record?.pids?.doi?.identifier || ""; |
There was a problem hiding this comment.
minor: see my comment below(can be replacing empty string) why the empty string is needed in the first place?
There was a problem hiding this comment.
The empty string is the initial state when the form is rendered. This was carried over from the existing implementation and I didn't refactor it. If you don't think it is a major blocker I would avoid refactoring more now...
On new upload, unreserving a local DOI was not re-enabling all options. This PR refactors the relevant code to fix that.