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

Create/Edit for Communities and Collections #349

Merged
merged 87 commits into from Feb 8, 2019

Conversation

Projects
None yet
5 participants
@LotteHofstede
Copy link
Contributor

commented Jan 4, 2019

This PR closes #217
The main goal of this PR is to make creating and editing of communities and collections possible. It also contains code to handle post requests better and code to patch changes in objects.

Create forms:
To create a new community, visit [base URL]/communities/create
To create a subcommunity, visit [base URL]/communities/create?parent=[uuid-of-parent]
To create a collection, visit [base URL]/collections/create?parent=[uuid-of-parent]

Edit forms:
To edit a community, visit [base URL]/communities/[uuid-of-community]/edit
To edit a collection, visit [base URL]/collections/[uuid-of-collection]/edit

NB: currently, patches are sent to the server as PUT requests, because PATCH requests have not been implemented by the REST side. However, the frontend code stores the changes as patches, so this can be changed easily in the future.

NB2: error messages are not shown properly at the moment, this is because this PR uses the dynamic form code, which currently has an issue with showing error messages. This is very likely fixed in the Submission PR.

Atmire-Kristof and others added some commits Jul 20, 2018

Merge branch 'master' into w2p-54472_Create-community-and-collection-…
…pages

Conflicts:
	src/app/+collection-page/collection-page-routing.module.ts
	src/app/+collection-page/collection-page.module.ts
	src/app/+community-page/community-page-routing.module.ts
	src/app/core/data/community-data.service.ts
	src/app/core/data/dso-response-parsing.service.ts
@tdonohue
Copy link
Member

left a comment

Similar to #348 , this PR looks to have a large number of unrelated commits/changes? Does this feature really require 80 commits and over 3,500 lines of new code?

@artlowel could you take a closer look at this PR please.

@LotteHofstede

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

@tdonohue As mentioned in the description this PR also contains code to solve issues with caching non-idempotent requests (POST, PUT, PATCH, DELETE...), that's why it closes issue #217.
To make editing possible, I also implemented everything necessary for PATCH requests, even though the backend cannot handle these requests yet for all endpoints. When a community or collection is edited, the changes will be directly applied in the store, but not sent to the backend yet. This is what the ServerSyncBuffer classes are handling: they keep patches (which is a list of changes/operations) in memory and send them to the REST api every X (configurable) seconds if necessary.

LotteHofstede and others added some commits Jan 10, 2019

@tdonohue
Copy link
Member

left a comment

@LotteHofstede : My apologies for the long delay in feedback. I gave this a code review today, and the code looks good. I have a few, minor inline improvements to suggest (mostly typedocs -- though you've done a great job of writing the vast majority of typedocs already, thanks!)

I plan to give this a test tomorrow as well. So, this is just the initial feedback.

Show resolved Hide resolved ...lters/search-filter/search-facet-filter/search-facet-filter.component.ts Outdated
Show resolved Hide resolved ...app/+search-page/search-filters/search-filter/search-filter.component.ts Outdated
Show resolved Hide resolved src/app/core/auth/server-auth.service.ts Outdated
Show resolved Hide resolved src/app/core/cache/builders/data-build.service.ts Outdated
Show resolved Hide resolved src/app/core/cache/object-cache.reducer.ts
Show resolved Hide resolved src/app/core/data/update-comparator.ts Outdated
Show resolved Hide resolved src/app/core/dspace-rest-v2/dspace-rest-v2.service.ts
Show resolved Hide resolved src/app/shared/comcol-forms/comcol-form/comcol-form.component.ts
Show resolved Hide resolved src/config/auto-sync-config.interface.ts
Show resolved Hide resolved src/app/shared/testing/utils.ts

@artlowel artlowel referenced this pull request Jan 17, 2019

Merged

Submission implementation #279

@tdonohue
Copy link
Member

left a comment

@LotteHofstede and @artlowel : I gave this a test today, and found the following:

  • All of the new form pages load properly. They look good.
  • When I click Submit (and look at the various REST requests) they all look to be the proper POST/PUT requests. However, several of them result in errors (which as noted in the PR description are not displayed) cause these REST endpoints don't yet exist (at least on the demo site). I suspect this is simply because this PR isn't yet merged & on demo: DSpace/DSpace#2277
  • A tiny thing, I did notice that the Admin sidebar links haven't been updated to align with this PR. For example, the links under "Add" point at /collections/submission and /communities/submission. But, that can always be updated in a later PR.

👍 Overall, I'm satisfied that this PR does what it's supposed to do. Once the backend REST endpoints are in place we can always retest and see if any bugs are found.

All that said, this PR needs two minor things before it can be merged:

  • Needs rebasing on latest master (there are merge conflicts that now exist)
  • I have one (very minor) outstanding question about removing comments related to #217 (see my previous comment on this PR). We may want to quickly clean that up before merger.

LotteHofstede added some commits Jan 22, 2019

Merge branch 'master' into community-and-collection-forms
Conflicts:
	src/app/core/data/item-data.service.spec.ts
	src/app/core/data/item-data.service.ts
	src/app/core/shared/operators.spec.ts
	src/app/core/shared/operators.ts
	src/app/shared/shared.module.ts
@tdonohue

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

@LotteHofstede : I tested this again today against a local copy of the latest code in DSpace/DSpace#2277. Unfortunately, I found several bugs in behavior. I believe this PR isn't synced up with the latest REST API syntax/calls that are in DSpace/DSpace#2277, so I suspect these are all minor changes to the POST/PUT syntax sent to the REST API.

  1. Creating a Collection doesn't work. I get a 500 error. I think the request is incorrect, as it looks to be sending http://localhost:8080/spring-rest/api/core/collections?parentCommunity=<:uuid>, when it should be http://localhost:8081/spring-rest/api/core/collections?parent=<:uuid>
  2. Creating a SubCommunity doesn't work. All Communities are top level communities. I think this is the same as 1, it's likely sending a parentCommunity param instead of parent.
  3. Editing a Community doesn't work. It appears to, as after I hit submit, I see my changes. But, if I leave the page and go back, they are gone. On the backend, I see a 422 error is returned from the PUT.
  4. Editing a Collection doesn't work. Same behavior as Edit Community, and I see a 422 error.

Dismissing my approval until synced with REST API

@LotteHofstede

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

@tdonohue
I updated the parentCommunity parameter to match the parent parameter the REST server is expecting; creating subcommunities and collections seems to work again.

I currently cannot fix the problem with editing both communities/collections. I looked at the requests I sent to the REST server to see what was wrong.
Two things were causing the 422 status:

  1. I was also sending the self link with the PUT request. The REST api does not expect this field and therefore sends back a 422 status.
  2. The REST server expects the object's original name field to be sent with the PUT request. If it's not there, a 422 is also returned.

The first issue was easily fixed and was a mistake on my part.
However, the second issue is more complicated as name is an alias for dc.title, and the user is able to change that field while editing the object. The object's name field that is sent to the server has to be the same as the name that is currently stored in the backend, which would imply the frontend has to keep track of the previous name value. This seems like unnecessary overhead.

I've notified @benbosman, he will look into this.

@Raf-atmire

This comment has been minimized.

Copy link

commented Feb 7, 2019

We have looked into this on the REST side of things and we've disabled the check for name for a PUT on the Community/Collection endpoints which was causing this 422 exception.
This has been disabled in commit: DSpace/DSpace@aa4125c
Which is located within PR: DSpace/DSpace#2290
As soon as this PR is merged, the current Angular code should be valid and should succeed.

@tdonohue
Copy link
Member

left a comment

👍 I've retested this today, and the create/edit forms now work. Thanks @LotteHofstede and @Raf-atmire for the updates (both to the Angular side and REST side) to get this working right.

One minor issue I've noticed is that the "Cancel" buttons on the forms do nothing (except clear all the values from the form, which is bad behavior if you are doing a Edit). I'd expect them to close the form and return to the previous page (or something similar). However, as this PR is very large & mostly works, I'm going to merge it as-is. I'll log the "Cancel" button issue as a bug which can be solved in a follow-up PR.

UPDATE: The "Cancel" button issue was logged here: #358

@tdonohue tdonohue merged commit 13c3e2f into DSpace:master Feb 8, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.1%) to 77.947%
Details

@ghost ghost removed the needs review label Feb 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.