Allow to download NMLs of older versions for read-only tracings #3660
Conversation
…ML of older versions
…/webknossos into read-only-version-download
…/webknossos into read-only-version-download
|
🎉 Didn't look at the code, yet, but tested already. It seems to work, but I had some hiccups (because I probably didn't use it as intended).
Otherwise, the testing worked well 👍 I noticed that the re-upload of the NML swalled empty trees, but I guess, we designed this on purpose at some point. |
| }/download`; | ||
| win.location.href = downloadUrl; | ||
| win.document.body.innerHTML = messages["download.close_window"]; | ||
| downloadNml(this.props.annotationId, this.props.tracingType); |
| setCurrentLayout: string => void, | ||
| saveCurrentLayout: () => void, | ||
| }, | ||
| layoutProps: LayoutProps, |
There was a problem hiding this comment.
Thank you for cleaning this up 🙈
| }; | ||
|
|
||
| componentDidMount() { | ||
| // Remember whether the tracing could originally be updated | ||
| this.setState({ originalAllowUpdate: this.props.allowUpdate }); |
There was a problem hiding this comment.
Why does this prop need to be written into state? If I'm not missing anything, the state is never changed? Couldn't we access the prop directly where needed? If we need to keep it, I'd rename it to "initialAllowUpdate" btw, since this is more idiomatic afaik :)
There was a problem hiding this comment.
The problem is that we change the allowUpdate value while the version restore view is open and therefore the prop changes as well. We need the value allowUpdate has when the component is mounted. I'll rename it to initialAllowUpdate.
There was a problem hiding this comment.
Hm, ok, but where is allowUpdate set to true? I only see the calls to setAnnotationAllowUpdateAction, where the initial value is passed (which would be false in the case we are talking about).
There was a problem hiding this comment.
Not sure whether we're talking about the same thing :D
If we're in a read-only tracing, allowUpdate is false when opening the version view, therefore, initialAllowUpdate is false as well and when closing the view setAnnotationAllowUpdateAction will set allowUpdate to initialAllowUpdate which is false - unnecessary in that case, but ok.
If we're in a normal tracing, ' allowUpdate' is true when opening the version view, initialAllowUpdate as well. We'll then set allowUpdate to false, because we don't want the tracing to be modified while the version is open. When closing the view or restoring an older version setAnnotationAllowUpdateAction will set allowUpdate to initialAllowUpdate which is true, so the user can modify the tracing again.
Hope that clears things up :)
There was a problem hiding this comment.
Ahh, ok, sorry, you had to type this out 🙈
|
@philippotto I’ll create separate issues for the permission problems you described. Should be independent of this PR |
…d-only-version-download
|
@philippotto Would you agree to merge this? I'd say the error messages, although they are not the nicest, are ok for now until the permission issues are fixed - the functionality will probably be mostly used by us (or logged in users) for now. |
Sure, go ahead 👍 |
* allow version restore view in read-only tracings, offer to download NML of older versions * use correct version parameters for nml download * annotation download: pass on version parameters to tracingstore * scalafmt * rename tracingType and APITracingType to annotationType where appropriate * update changelog * apply PR feedback
Allow to display the version restore view for read-only tracings. Offer to download an NML of older versions.
@philippotto I also renamed the APITracingType and related types/variables to APIAnnotationType etc in this PR. For review I would suggest to exclude this commit to get a better sense of the actual changes :)
URL of deployed dev instance (used for testing):
Steps to test:
Preview an older version, the button should say "Download" instead of "Restore" and should download an NML of the previewed older version.
Issues:
- [ ] Updated migration guide if applicable- [ ] Updated documentation if applicable- [ ] Needs datastore update after deployment