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
Collaborator

@kilchenmann kilchenmann commented Jul 7, 2021

resolves DSP-1776

@kilchenmann kilchenmann self-assigned this Jul 7, 2021
Copy link
Contributor

@waychal waychal left a comment

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

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann 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.

Loading

@waychal
Copy link
Contributor

@waychal 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.

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann 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.

Loading

@waychal
Copy link
Contributor

@waychal 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.

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann 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.

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

Loading

Copy link
Contributor

@mdelez mdelez left a comment

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

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented Jul 9, 2021

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.

Loading

@mdelez mdelez self-requested a review Jul 9, 2021
mdelez
mdelez approved these changes Jul 9, 2021
@kilchenmann kilchenmann merged commit d916b4b into main Jul 9, 2021
8 checks passed
Loading
@kilchenmann kilchenmann deleted the wip/dsp-1776-upload-document branch Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants