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

DS-4514 start a new submission via file #724

Merged
merged 11 commits into from
Aug 6, 2020
Merged

Conversation

ddinuzzo
Copy link
Contributor

@ddinuzzo ddinuzzo commented Jun 30, 2020

References

DS-4514
Related REST PR (but not fully dependent on REST PR)

Description

This PR include the following:

  • the drop box handles only one file at time, if the user tries to add more than one file, an error message is shown;
  • when REST detects that multiple records are found, it returns a response with http 422 status code in this case the UI shows a specific error message;
  • after selecting the file and before uploading it, the user interface opens the dialog for choosing the collection;

Instructions for Reviewers

This PR must have to testing with REST API presents in another PR (PR - 2791)

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
  • My PR passes TSLint validation using yarn run lint
  • My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • My PR passes all specs/tests and includes new/updated specs for any bug fixes, improvements or new features. A few reminders about what constitutes good tests:
    • Include tests for different user types (if behavior differs), including: (1) Anonymous user, (2) Logged in user (non-admin), and (3) Administrator.
    • Include tests for error scenarios, e.g. when errors/warnings should appear (or buttons should be disabled).
    • For bug fixes, include a test that reproduces the bug and proves it is fixed. For clarity, it may be useful to provide the test in a separate commit from the bug fix.
  • If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

@abollini abollini changed the title Cst 3091 DS-4514 start a new submission via file Jul 2, 2020
@abollini abollini added this to the 7.0beta3 milestone Jul 2, 2020
Copy link

@crosenbeck crosenbeck left a comment

Choose a reason for hiding this comment

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

@ddinuzzo Thank you for work on this PR. This PR looks good to me.

@tdonohue tdonohue changed the base branch from master to main July 13, 2020 21:38
@heathergreerklein heathergreerklein added the e/24 Estimate in hours label Jul 15, 2020
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

@ddinuzzo : Overall, this looks good. It works when tested with DSpace/DSpace#2791. However, I've noticed a few odd things that I wanted to ask about.

  1. First off, it looks like this is blocking the ability to upload more than one file at a time from the MyDSpace page. I don't think we decided to do that. See the inline comment below. If there's a reason for this change in behavior though, I'd like to understand why we decided to change it.
  2. Second, I'm finding that anytime I cause an error to occur (e.g. upload two files at once, or upload a BibTex with multiple entries), the MyDSpace page become "unusable" until I reload the entire browser window. It's possible this is my own testing environment playing tricks on me. But, it seems to happen every time an error occurs. Here's what I see:
    • First, I cause an error. For example, upload a Bibtex file with two entries. I get the proper error message. All seems fine so far.
    • Then, if I click on the "New Submission" button I see the Select a Collection popup, but it loads forever. It never completes. I'm forced to hit reload in my browser to get the page to work again.
    • Similarly, if I go back and cause the same error & then click on "Edit" next to an in progress item, the same thing occurs. It loads forever. It never completes. I'm forced to hit reload in my browser to get the page to work again.

I'm not yet certain whether the second issue is specific to this PR. If we find it isn't, we can always log a bug ticket and fix it later. But, I'd like more information on the first issue -- why we've chosen to limit the upload to only one file at a time. Thanks!

templateUrl: './collection-selector.component.html',
styleUrls: ['./collection-selector.component.scss']
})
export class CollectionSelectorComponent {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a TypeDocs description to this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@atarix83
Copy link
Contributor

@tdonohue

Second, I'm finding that anytime I cause an error to occur (e.g. upload two files at once, or upload a BibTex with multiple entries), the MyDSpace page become "unusable" until I reload the entire browser window. It's possible this is my own testing environment playing tricks on me. But, it seems to happen every time an error occurs. Here's what I see:
First, I cause an error. For example, upload a Bibtex file with two entries. I get the proper error message. All seems fine so far.
Then, if I click on the "New Submission" button I see the Select a Collection popup, but it loads forever. It never completes. I'm forced to hit reload in my browser to get the page to work again.
Similarly, if I go back and cause the same error & then click on "Edit" next to an in progress item, the same thing occurs. It loads forever. It never completes. I'm forced to hit reload in my browser to get the page to work again.

this problem should be due to this issue #819 and should not related this PR.

First off, it looks like this is blocking the ability to upload more than one file at a time from the MyDSpace page. I don't think we decided to do that. See the inline comment below. If there's a reason for this change in behavior though, I'd like to understand why we decided to change it.

This is a limit of the library that our uploader use that doesn't allow to make a POST with multiple file. To allow multiple simultaneous upload we have two options :

  • change the library we use
  • extends the current library to allow it

in both case further complicated developments are needed, and we think it'd be better to have a follow-up PR to solve it

@tdonohue
Copy link
Member

tdonohue commented Jul 24, 2020

@atarix83 : Thanks for the explanations there. I've created a new bug ticket about the multiple file upload issue (and tentatively scheduled for beta5, though we could fix it earlier if a volunteer came up): #820 I also agree with you that the MyDSpace page error issue is a separate bug.

So, this PR can mostly move ahead as-is. I had a minor request for TypeDocs & it looks like there's a new (hopefully minor) merge conflict to resolve.

@ddinuzzo ddinuzzo requested a review from tdonohue August 5, 2020 07:27
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Retested today and this works as described (and based on the scope of this PR as discussed in previous comments). Thanks @ddinuzzo !

This can be merged as soon as the backend PR (DSpace/DSpace#2791) is fully approved & merged. It's also very close & I think that should be approved sometime this week.

@tdonohue tdonohue merged commit 4f608eb into DSpace:main Aug 6, 2020
@abollini abollini deleted the CST-3091 branch August 11, 2020 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e/24 Estimate in hours
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants