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

Mydspace #384

Merged
merged 77 commits into from
May 10, 2019
Merged

Mydspace #384

merged 77 commits into from
May 10, 2019

Conversation

atarix83
Copy link
Contributor

@atarix83 atarix83 commented Apr 5, 2019

This PR implements MyDSpace page.

This PR must be test against this REST PR DSpace/DSpace#2391 and can be merged only after the REST PR is merged in the master.

To test this PR please use an environment where configurable workflow is properly configured.

There is a known issue while trying to deposit a workspaceitems to a collection without a configured workflow. Issue depends on a problem on REST side and has already reported here to resolve it

…ions' into mydspace

# Conflicts:
#	resources/i18n/en.json
#	src/app/+search-page/search-filters/search-filter/search-facet-filter/search-facet-filter.component.ts
#	src/app/+search-page/search-filters/search-filter/search-filter.component.ts
#	src/app/+search-page/search-filters/search-filters.component.html
#	src/app/+search-page/search-filters/search-filters.component.ts
#	src/app/+search-page/search-page.component.ts
#	src/app/+search-page/search-page.module.ts
#	src/app/+search-page/search-service/search.service.ts
#	src/app/core/cache/builders/remote-data-build.service.ts
#	src/app/core/core.module.ts
#	src/app/core/data/paginated-list.ts
#	src/app/core/data/request.models.ts
#	src/app/core/data/request.service.spec.ts
#	src/app/core/data/request.service.ts
#	src/app/shared/services/route.service.ts
#	src/app/shared/shared.module.ts
# Conflicts:
#	resources/i18n/en.json
#	src/app/core/data/request.service.spec.ts
#	src/app/core/data/request.service.ts
#	src/app/core/json-patch/selectors.ts
#	src/app/shared/shared.module.ts
# Conflicts:
#	src/app/core/data/request.service.ts
@atarix83
Copy link
Contributor Author

@artlowel

The browse link is lower than the rest of the text by a few px. The new submission button is higher than the drop-box by a few px.

They should be aligned now

The validation label at the top is squared with rounded corners, the submitter name label is entirely rounded.

Fixed

there is not enough spacing, e.g. between the Edit/Delete/Approve/Reject/Return to pool buttons

Fixed

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 : I gave this a quick test today as well (against the latest master code). I'm seeing the same error that @paulo-graca reported here whenever I attempt to click "Deposit" on an in-progress submission (/workspaceitems/[id]/edit). However, if I manually return to the MyDSpace page, I do see that the Item is moved to the "Archived" status (so the deposit was successful, even though I saw an error message).

I've also found I cannot drag & drop a PDF onto the MyDSpace page (/mydspace?configuration=workspace). Doing so results in an error that says "Error creating new workspace. Please verify the content uploaded before retry." However, I can successfully drag & drop a PDF into an active New Submission (/submit or /workspaceitems/[id]/edit). It only seems to fail from the MyDSpace page.

Beyond those two oddities in testing, the rest of the features seem to work well. Thanks!

@artlowel
Copy link
Member

artlowel commented Apr 30, 2019

Thanks @atarix83!

The search issue I reported is fixed, and you've taken aboard all of my suggestions.

I can also reproduce the issues @paulo-graca en @tdonohue reported though (depositing an in-progess submission and adding a pdf on mydspace both cause error notifications)

@atarix83
Copy link
Contributor Author

atarix83 commented May 2, 2019

@tdonohue @artlowel @paulo-graca

I've checked issue with depositing an in-progess submission and problem occurs even with depositing a new one. However it happens only when trying to submit to a collection without a defined workflow and is not related to this PR but is due to a REST side issue that has already been reported here

@atarix83
Copy link
Contributor Author

atarix83 commented May 2, 2019

@tdonohue

I've also found I cannot drag & drop a PDF onto the MyDSpace page (/mydspace?configuration=workspace). Doing so results in an error that says "Error creating new workspace. Please verify the content uploaded before retry." However, I can successfully drag & drop a PDF into an active New Submission (/submit or /workspaceitems/[id]/edit). It only seems to fail from the MyDSpace page.

this should be fixed now

@tdonohue
Copy link
Member

tdonohue commented May 3, 2019

@atarix83 : Just a note, it looks like your most recent changes accidentally broke the build of this PR. See Travis builds: https://travis-ci.org/DSpace/dspace-angular/builds/527355394

@abollini
Copy link
Member

abollini commented May 3, 2019

@tdonohue the failure are due to the down of the rest api demo site that has been caused by the recent update as we haven't yet had the time to switch to solr7

@artlowel
Copy link
Member

artlowel commented May 7, 2019

@tdonohue

I've also found I cannot drag & drop a PDF onto the MyDSpace page (/mydspace?configuration=workspace). Doing so results in an error that says "Error creating new workspace. Please verify the content uploaded before retry." However, I can successfully drag & drop a PDF into an active New Submission (/submit or /workspaceitems/[id]/edit). It only seems to fail from the MyDSpace page.

this should be fixed now

I've retested and verified that it's fixed. Thanks @atarix83!

@artlowel
Copy link
Member

artlowel commented May 7, 2019

I've approved this PR, in order for us to move ahead with the preview release, but I want to note a couple of things we still need to address after it has been merged.

Edit: added links to issues

@tdonohue
Copy link
Member

tdonohue commented May 7, 2019

@artlowel : could you turn those actions into tickets? I think it'd be worth capturing them so that they are not forgotten.

I'd also like to see this PR merged ASAP. I was hoping we could first get the demo REST API back up and running though, so we can see Travis CI approve this PR. Longer term, this points out the obvious (and known) problem with our tests -- they are dependent on an external server. We will need to prioritize a resolution to that issue, so that future PRs are not put in a similar state as this one.

@paulo-graca
Copy link
Contributor

Thank you @atarix83 for addressing my comments. But I can only test this again this Friday. I will give a second look.

@LotteHofstede LotteHofstede mentioned this pull request May 9, 2019
@atarix83
Copy link
Contributor Author

atarix83 commented May 9, 2019

@tdonohue PR build is successful again

@artlowel thanks for approving, only a comment on your note :

It seems the rest api embeds objects the UI can't use and they need to be filtered from the response

it's only a filter to parse only useful objects, how it was done on public search https://github.com/4Science/dspace-angular/blob/mydspace/src/app/core/data/search-response-parsing.service.ts#L58

@atarix83
Copy link
Contributor Author

atarix83 commented May 9, 2019

@tdonohue @artlowel @paulo-graca
I've changed the default rest host to point to new rest demo with solr 7 https://dspace7.4science.cloud/dspace-spring-rest/#/dspace-spring-rest/api

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.

👍 I've retested today, and the bugs I previously found have been resolved (other than the bug related to Collections without a workflow, but that's a REST API bug logged in DS-4210). So, to me, this looks ready to merge.

As noted by @artlowel above, there are a few outstanding minor issues, but those can be logged as minor bugs and resolved in follow-up PRs.

As @paulo-graca has noted above that he wants to review this again on Friday (tomorrow), I will leave this PR open for one more day. I'll merge it tomorrow, or whenever Paulo has had a chance to do his final review.

@paulo-graca
Copy link
Contributor

paulo-graca commented May 10, 2019

I don't know if it's because of my database (which have entities) but I'm getting several errors with this PR, like:

Error: The server returned an invalid object
    at DSOResponseParsingService../src/app/core/data/base-response-parsing.service.ts.BaseResponseParsingService.addToObjectCache (base-response-parsing.service.ts:113)
    at 
....

But since @tdonohue and @artlowel already gave their thumbs up. I think this PR it's in the condition to be merged.

@tdonohue
Copy link
Member

@paulo-graca : Thanks for the notes. As both this PR and the Entities one (#372) are set to merge today, it's possible that some of what you are seeing will be worked out once we get both of these in master. However, It'd obviously be good for you (and everyone else) to test everything again once it is all merged...it's very possible we'll end up with minor bugs between these big PRs (which we can get resolved ASAP).

So, I'll move ahead and get this merged & Entities work merged. Then notify everyone to retest master to see if we notice any bugs that need squashing.

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.

5 participants