Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(resource): upload pdf document (DSP-1776) #481

Merged
merged 7 commits into from Jul 9, 2021

Conversation

kilchenmann
Copy link
Contributor

resolves DSP-1776

@kilchenmann kilchenmann self-assigned this Jul 7, 2021
@kilchenmann kilchenmann added the enhancement New feature or request label Jul 7, 2021
Copy link
Contributor

@waychal waychal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am running DSP-APP locally using test server configuration. When I upload file, I do not see it once I save the changes. I have attached recording below. Also When I add resource label including -(e.g. file-test, it disables next button and gives me wrong error in red text below. If I remove -, it works fine.

Screen.Recording.2021-07-08.at.12.33.49.mov

@kilchenmann
Copy link
Contributor Author

I am running DSP-APP locally using test server configuration. When I upload file, I do not see it once I save the changes. I have attached recording below. Also When I add resource label including -(e.g. file-test, it disables next button and gives me wrong error in red text below. If I remove -, it works fine.

Screen.Recording.2021-07-08.at.12.33.49.mov

Thanks @waychal for your comment. There are two things:

The resource instance form has a bug in the label: it does not allow to use special characters. For me it's not understandable, why the label is restricted this way. But it's another topic and has to be resolved in another task. This task is only the implementation of an upload form for documents and that leads me to the second point: The viewer for a document is not implemented yet and will be handled in a separate task.

@waychal
Copy link
Contributor

waychal commented Jul 8, 2021

I am running DSP-APP locally using test server configuration. When I upload file, I do not see it once I save the changes. I have attached recording below. Also When I add resource label including -(e.g. file-test, it disables next button and gives me wrong error in red text below. If I remove -, it works fine.
Screen.Recording.2021-07-08.at.12.33.49.mov

Thanks @waychal for your comment. There are two things:

The resource instance form has a bug in the label: it does not allow to use special characters. For me it's not understandable, why the label is restricted this way. But it's another topic and has to be resolved in another task. This task is only the implementation of an upload form for documents and that leads me to the second point: The viewer for a document is not implemented yet and will be handled in a separate task.

Ok. But then in this case, how can I test the functionality? How can I know that the file is actually uploaded? I would like to get at least some alert or console message saying that if it is successful or there is an error.

@kilchenmann
Copy link
Contributor Author

I am running DSP-APP locally using test server configuration. When I upload file, I do not see it once I save the changes. I have attached recording below. Also When I add resource label including -(e.g. file-test, it disables next button and gives me wrong error in red text below. If I remove -, it works fine.
Screen.Recording.2021-07-08.at.12.33.49.mov

Thanks @waychal for your comment. There are two things:
The resource instance form has a bug in the label: it does not allow to use special characters. For me it's not understandable, why the label is restricted this way. But it's another topic and has to be resolved in another task. This task is only the implementation of an upload form for documents and that leads me to the second point: The viewer for a document is not implemented yet and will be handled in a separate task.

Ok. But then in this case, how can I test the functionality? How can I know that the file is actually uploaded? I would like to get at least some alert or console message saying that if it is successful or there is an error.

I know this is a problem, but the upload is needed to start with the document viewer. You can have a look in the Network tab in browser's developer tools, if the API responses with the correct object and if this object has information about the file value. Additional it should be possible to open the pdf with sipi. But there, in some cases we have some permission problems. When you test it locally, you can check in finder if the file is in the sipi image folder dsp-api/sipi/images/[project-shortcode-folder]/. In this case the file upload was successful.

@waychal
Copy link
Contributor

waychal commented Jul 8, 2021

I am running DSP-APP locally using test server configuration. When I upload file, I do not see it once I save the changes. I have attached recording below. Also When I add resource label including -(e.g. file-test, it disables next button and gives me wrong error in red text below. If I remove -, it works fine.
Screen.Recording.2021-07-08.at.12.33.49.mov

Thanks @waychal for your comment. There are two things:
The resource instance form has a bug in the label: it does not allow to use special characters. For me it's not understandable, why the label is restricted this way. But it's another topic and has to be resolved in another task. This task is only the implementation of an upload form for documents and that leads me to the second point: The viewer for a document is not implemented yet and will be handled in a separate task.

Ok. But then in this case, how can I test the functionality? How can I know that the file is actually uploaded? I would like to get at least some alert or console message saying that if it is successful or there is an error.

I know this is a problem, but the upload is needed to start with the document viewer. You can have a look in the Network tab in browser's developer tools, if the API responses with the correct object and if this object has information about the file value. Additional it should be possible to open the pdf with sipi. But there, in some cases we have some permission problems. When you test it locally, you can check in finder if the file is in the sipi image folder dsp-api/sipi/images/[project-shortcode-folder]/. In this case the file upload was successful.

Ok. I do not have dsp-api running locally because of new MacBook processor. So I can not test it.
I can see the requests in network tab but I am not very sure about this.
For now I am approving this PR but it would be good if someone else also have a look at it.

@kilchenmann
Copy link
Contributor Author

I am running DSP-APP locally using test server configuration. When I upload file, I do not see it once I save the changes. I have attached recording below. Also When I add resource label including -(e.g. file-test, it disables next button and gives me wrong error in red text below. If I remove -, it works fine.
Screen.Recording.2021-07-08.at.12.33.49.mov

Thanks @waychal for your comment. There are two things:
The resource instance form has a bug in the label: it does not allow to use special characters. For me it's not understandable, why the label is restricted this way. But it's another topic and has to be resolved in another task. This task is only the implementation of an upload form for documents and that leads me to the second point: The viewer for a document is not implemented yet and will be handled in a separate task.

Ok. But then in this case, how can I test the functionality? How can I know that the file is actually uploaded? I would like to get at least some alert or console message saying that if it is successful or there is an error.

I know this is a problem, but the upload is needed to start with the document viewer. You can have a look in the Network tab in browser's developer tools, if the API responses with the correct object and if this object has information about the file value. Additional it should be possible to open the pdf with sipi. But there, in some cases we have some permission problems. When you test it locally, you can check in finder if the file is in the sipi image folder dsp-api/sipi/images/[project-shortcode-folder]/. In this case the file upload was successful.

Ok. I do not have dsp-api running locally because of new MacBook processor. So I can not test it.
I can see the requests in network tab but I am not very sure about this.
For now I am approving this PR but it would be good if someone else also have a look at it.

Sure. Thank you @waychal . I'm pretty sure that @mdelez will also have a look at it.

Copy link
Collaborator

@mdelez mdelez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2021-07-09 at 11 33 12

The upload works well! However I've noticed that you can save the file without providing a title, is this intended?

Also it would be nice to have title label fit nicely. Does it look okay on your end @kilchenmann?
Screen Shot 2021-07-09 at 11 11 02

@kilchenmann
Copy link
Contributor Author

Screen Shot 2021-07-09 at 11 33 12

The upload works well! However I've noticed that you can save the file without providing a title, is this intended?

Also it would be nice to have title label fit nicely. Does it look okay on your end @kilchenmann?
Screen Shot 2021-07-09 at 11 11 02

The document title property is defined in the ontology and in this case cardinality is set to 0-n, so it's not required. This is an ontology setup.
Yes we know that the property value inputs incl. their labels doesn't fit well and has to be fixed in a separate task. The whole resource instance form has to be updated.

@mdelez mdelez self-requested a review July 9, 2021 09:51
@kilchenmann kilchenmann merged commit d916b4b into main Jul 9, 2021
@kilchenmann kilchenmann deleted the wip/dsp-1776-upload-document branch July 9, 2021 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants