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

Submission miscellaneous fixes #432

Merged
merged 35 commits into from Sep 26, 2019

Conversation

atarix83
Copy link
Contributor

This PR solves some small miscellaneous issues for submission module.
Here the the most important changes:

  • during submission show only authorized collections in the collection dropdown list
  • Add hint message to form fields, hint message it's already present in rest configuration but it's displayed on the form
  • fix issues DS-4280 and DS-4279
  • during workspaceitem edit hide upload section on init when section is not mandatory and there are no file uploaded in the submission
  • other small bug fixes

Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @atarix83! Most of the issues mentioned are indeed fixed.

I can still reproduce https://jira.duraspace.org/browse/DS-4279 however, although the edit button does work now.

Are you using very large page sizes when fetching collections (here and here) in order to bypass this issue: #373? If so, I don't think larger page sizes can be a permanent solution. It will slow things down and there will usually be some institution with more collections than the limit. I agree that until we have a suitable rest endpoint a large page-size could work in most cases, but please update #373 with the fact that this is a temporary workaround that still needs to be replaced.

A final thing I noticed was that if you:

  1. start a new submission
  2. change the collection
  3. upload a file

The warning in the upload files panel will still mention the original collection. It wouldn't surprise me if
this was a pre-existing issue though.

Screenshot 2019-07-25 at 14 12 42

@@ -40,6 +41,22 @@ export class CollectionDataService extends ComColDataService<Collection> {
super();
}

/**
* Get all collections whom user has authorization to submit to by community
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be "Get all collections the user is authorized to submit to, by community"
I'm not a native speaker, but I think "whom" can only be used to refer to a person, not a thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@atarix83
Copy link
Contributor Author

@artlowel thanks for the review

I can still reproduce https://jira.duraspace.org/browse/DS-4279 however, although the edit button does work now.

This is how I see the buttons, there is space between all buttons, is not the same for you?

Schermata del 2019-07-26 09-54-13

Are you using very large page sizes when fetching collections (here and here) in order to bypass this issue: #373?

No this is not a permanent solution, is only to avoid to miss some collection in the list. I think that a better solution until we have a suitable rest endpoint, should be to remove community of the collection from the list, this solution would make the loading less heavy

The warning in the upload files panel will still mention the original collection. It wouldn't surprise me if this was a pre-existing issue though.
I've fixed the issue

@artlowel
Copy link
Member

@atarix83 I see the same buttons, but https://jira.duraspace.org/browse/DS-4279 says that in the first step you should only be able to see accept and reject buttons (so no edit), and in the last step only edit and accept buttons (so no reject).

I think that a better solution until we have a suitable rest endpoint, should be to remove community of the collection from the list, this solution would make the loading less heavy

Yeah that sounds like a good idea 👍

@atarix83
Copy link
Contributor Author

@artlowel

I think that a better solution until we have a suitable rest endpoint, should be to remove community of the collection from the list, this solution would make the loading less heavy

this has a side effect, in case of more collection belonging to different communities with same name, the user would not know to which collection it's submitting to

says that in the first step you should only be able to see accept and reject buttons (so no edit), and in the last step only edit and accept buttons (so no reject).

Based on how workspaceitems/workflowitems endpoints are currently implemented, I think it is not possible to have this distinction, so I'd leave out resolution of https://jira.duraspace.org/browse/DS-4279 from this PR

Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

this has a side effect, in case of more collection belonging to different communities with same name, the user would not know to which collection it's submitting to

Ok then leave it as is for now.

Approved, but note there's still a merge conflict with a spec file

@artlowel
Copy link
Member

Retested, and everything looks good. Thanks @atarix83 👍

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.

@atarix83 : The code itself looks good to me. However, I'm having a great amount of difficulty testing the /submit or /workflowitems/[id]/edit UI, as both seem to "hang" after initially loading. Any clicking or typing I do in the submission form is extremely slow / delayed (by seconds).

Eventually, the Submission form fully loads (after about 10-15 seconds if I don't touch it) and I'm able to interact with it as normal. But, it seems to take forever.

This behavior seems brand new in this PR (I just tried latest master and I don't see that 'hanging' behavior, but it reappeared as soon as I jumped back to this PR's branch).

Any ideas what could be going on? Have you or @artlowel seen this? By the way, I'm using the latest version of Chrome on Windows, and I'm starting the Angular UI using yarn start

@artlowel
Copy link
Member

@tdonohue I've noticed that once, when I first tried this PR, but wasn't able to reproduce it since.

@tdonohue
Copy link
Member

tdonohue commented Sep 4, 2019

This PR needs rebasing, as it now has merge conflicts.

…miscellaneous-fixes

# Conflicts:
#	src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.html
@atarix83
Copy link
Contributor Author

atarix83 commented Sep 5, 2019

@tdonohue
I've investigated on the issue reported.

It seems that is due to the way the collection list is retrieved when submission page is loading.
In order to be able to view only the collection the user can submit to I've changed the submission-form-collection.component.ts.
Indeed even if the submission page seems to be loaded, the submission-form-collection.component.ts is still retrieving the collection list by using getAuthorizedCollectionByCommunity. Unfortunately to have the entire collection list we need the entire community list, but this cause an extremely expensive REST call (because of fetching all logos and licenses) that is the cause the delay reported.

To fix this behaviour there are these possible solution:

  • Revert all the changes and show always all collection list, but this should cause error when submitting in an unauthorized collection.
  • Fetch collection list with getAuthorizedCollection, in this case we can't see the parent community and we can't discern between collections with the same name but in different community
  • as suggested by @abollini we should need to add, on REST side, a parent link to the collection model, in order to retrieve parent community information from collection

@tdonohue @artlowel
Please let me know what do you think about it

…miscellaneous-fixes

# Conflicts:
#	src/app/shared/form/builder/ds-dynamic-form-ui/ds-dynamic-form-control-container.component.html
@tdonohue
Copy link
Member

tdonohue commented Sep 6, 2019

@atarix83 : I understand the problem now. This sounds like a performance issue that could be helped by REST API projections, as well as the suggestion from @abollini to perhaps add a "parent" link (which might be useful for other purposes too).

👍 I'm OK with merging this as-is (with the known performance issues). However, before we do so, I'd like someone to log a bug ticket describing the problem and possible REST API solution(s). That way we can get this scheduled/assigned, and possibly link it up to similar work (like Projections). (Unfortunately, I'm adding this note just before heading out of the office for next week, so I won't be able to create this JIRA ticket myself. But, if someone else can do so, I'm fine with moving this PR forward as-is.)

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.

Looks good to me now. There are some outstanding performance issues here, but a separate ticket is logged for those: #487

@tdonohue tdonohue merged commit eea2506 into DSpace:master Sep 26, 2019
@atarix83 atarix83 deleted the submission-miscellaneous-fixes branch January 15, 2020 08:35
@tdonohue tdonohue added this to the 7.0beta1 milestone Jan 26, 2021
kosarko pushed a commit to kosarko/dspace-angular that referenced this pull request Apr 3, 2024
* BitstreamChecksum values are fetched and parsed from the BE

* Checksum info is showed up.

* Added messages and translations.

* Added docs and refactored code

* Fixed failing tests

* Fixed wrong czech translations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants