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

Modifying metadata registries #355

Merged
merged 40 commits into from Feb 21, 2019

Conversation

Projects
None yet
5 participants
@Atmire-Kristof
Copy link
Contributor

commented Jan 24, 2019

This PR depends on the REST PR DSpace/DSpace#2291

The general purpose of this PR is to expand the currently existing metadata schema and field registry pages allowing admins to create, edit and delete schemas and fields.

URLS
To modify schemas, visit [base-url]/admin/registries/metadata

Here, you can create a new schema by filling in the form at the top of the list and clicking “Submit” (both fields are required).
To edit a schema, click one of the existing ones in the list at the bottom. It will automatically fill in the form with the current values.
To delete schemas, select one or multiple by selecting the checkboxes in the list and clicking “Delete selected”.
Selected schemas will be remembered when navigating further by using the Store.

To modify fields, either click one of the schemas on the page above, or visit [base-url]/admin/registries/metadata/[schema-prefix]

Modifying fields happens in the exact same way as you would modify schemas.
Only the element input is required to add/edit a field, the rest is optional.

Authentication
An AuthenticatedGuard is being used for all the /admin routes.

Notes
This PR is dependent on the same response-cache refactoring as for example #350 and #348, which might make it seem there’s more changes here than necessary.

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

Samuel
* added authenticate guard
* added metadata schema form, with the form model fields & validation and layout
* fixed the pagination for schemas and fields
* added reducer and actions for the registry state

Atmire-Kristof added some commits Jan 24, 2019

Merge branch 'master' into w2p-58789_Metadata-Registry-UI-Part-2
Conflicts:
	src/app/+admin/admin-registries/metadata-schema/metadata-schema.component.ts
	src/app/+community-page/community-page.component.html
	src/app/app.reducer.ts
	src/app/core/auth/server-auth.service.ts
	src/app/core/core.module.ts
	src/app/core/data/item-data.service.spec.ts
	src/app/core/data/item-data.service.ts
	src/app/core/index/index.effects.ts
	src/app/core/index/index.reducer.spec.ts
	src/app/core/index/index.reducer.ts
	src/app/core/shared/operators.spec.ts
	src/app/core/shared/operators.ts
	src/app/header/header.component.spec.ts
@paulo-graca

This comment has been minimized.

Copy link

commented Feb 5, 2019

Does this Pull Request depends on other DSpace PR?
I'm getting an «Http failure response for http://localhost/spring-rest/api/core/metadatafields?schemaId=6: 404 Not Found» Error

@artlowel

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

@paulo-graca Yes, it depends on DSpace/DSpace#2291. I'll add that to the PR description.

@paulo-graca
Copy link

left a comment

This PR achieves what it aims for. But it does it with some usability issues. Like:
. if the user wants to change an existing schema it will be very hard to accomplish this task at first time (we've tried this with 3 different people)
. The content and actions will be accessible even if the user isn't an administrator
. The fields list is visible (without the actions) even when your logged session expires
. if you have an extensive list of fields/schemas, if the user clicks on the a bottom row to change that field/schema, it seems like nothing happened. The user need to figure out that he must scroll up to the form
. the "Delete Selected" button, will not be visible if you have an extensive list of fields/schemas

I've some suggestions like frame the tables to enable the action buttons to be shown and to remove links from schema table, replacing them with action buttons on each row and add an edit action.

/**
* Stop editing the currently selected metadata field
*/
onCancel() {

This comment has been minimized.

Copy link
@paulo-graca

paulo-graca Feb 12, 2019

My comment is more as a report of an usability issue... with the event cancel, the user is lean to think that he can cancel this operation and go back to the previous screen, like the "Return" action in the footer.
Perhaps "Reset" it's best suited for this action.

(cancel)="onCancel()"
(submit)="onSubmit()">

</ds-form>

This comment has been minimized.

Copy link
@paulo-graca

paulo-graca Feb 12, 2019

Suggestion to add to this component the ability to return to the previous page
<button [routerLink]="['/admin/registries/metadata']" class="btn btn-primary">{{'admin.registries.schema.return' | translate}}</button>

>
</label>
</td>
<td class="selectable-row" (click)="editSchema(schema)"><a [routerLink]="[schema.prefix]">{{schema.id}}</a></td>

This comment has been minimized.

Copy link
@paulo-graca

paulo-graca Feb 12, 2019

this is more like an usability issue... for a schema to be active, the user must click on the row (outside of any links).
But the user it's "programmed" to click on links, so it will be hard for him to find the way to change the schema's data (I've already test this with other and none of them could do that action). I would suggest to remove the existing links. Perhaps approaching this interface as it is in JSPUI, with just one link and buttons (one to edit, and other to delete).

constructor(public registryService: RegistryService, private formBuilderService: FormBuilderService, private translateService: TranslateService) {
}

ngOnInit() {

This comment has been minimized.

Copy link
@paulo-graca

paulo-graca Feb 12, 2019

The schema form is visible even if the user doesn't have permissions.

This comment has been minimized.

Copy link
@artlowel

artlowel Feb 14, 2019

Member

This should be fixed now

}

ngOnInit() {
combineLatest(

This comment has been minimized.

Copy link
@paulo-graca

paulo-graca Feb 12, 2019

The field form is visible even if the user doesn't have permissions.

This comment has been minimized.

Copy link
@artlowel

artlowel Feb 14, 2019

Member

This should be fixed now

@paulo-graca

This comment has been minimized.

Copy link

commented Feb 12, 2019

This image https://drive.google.com/file/d/16rym5Vrx1sidYhWZyJvdgLrX7jdE7v5x/view shows what I was referring when said "The fields list is visible (without the actions) even when your logged session expires"

Atmire-Kristof added some commits Feb 13, 2019

Merge remote-tracking branch 'atmire/master' into modifying-metadata-…
…registries

Conflicts:
	src/app/+community-page/community-page.component.ts
	src/app/+item-page/edit-item-page/item-delete/item-delete.component.spec.ts
	src/app/+item-page/edit-item-page/item-delete/item-delete.component.ts
	src/app/+item-page/edit-item-page/item-private/item-private.component.ts
	src/app/+item-page/edit-item-page/item-public/item-public.component.spec.ts
	src/app/+item-page/edit-item-page/item-public/item-public.component.ts
	src/app/+item-page/edit-item-page/item-reinstate/item-reinstate.component.spec.ts
	src/app/+item-page/edit-item-page/item-reinstate/item-reinstate.component.ts
	src/app/+item-page/edit-item-page/item-withdraw/item-withdraw.component.spec.ts
	src/app/+item-page/edit-item-page/item-withdraw/item-withdraw.component.ts
	src/app/+item-page/edit-item-page/simple-item-action/abstract-simple-item-action.component.spec.ts
	src/app/core/auth/server-auth.service.ts
	src/app/core/cache/response.models.ts
	src/app/core/data/item-data.service.spec.ts
	src/app/core/data/item-data.service.ts
	src/app/core/data/request.models.ts
	src/app/core/index/index.effects.ts
	src/app/core/metadata/metadata.service.spec.ts
	src/app/core/shared/operators.spec.ts
	src/app/header/header.component.spec.ts
	src/app/shared/form/form.component.ts
@artlowel

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

@paulo-graca I agree with you on those usability issues. For this first iteration we made something that leans very close to how it worked in dspace 6 xmlui, but of course there was no editing in dspace 6, so what was there was has been modified slightly to allow for editing.

Rather than come up with an approach for this feature separately, I suggest we wait for the Admin Item Edit PR, because the functionality is very similar. We compare what we like and what we don't in each PR, and decide which approach we want to use, and whether or not we change it now, or after the preview release.

@tdonohue
Copy link
Member

left a comment

👍 Overall, I approve of this PR. The code looks good, and I gave it a test, and it does work well.

Overall, the Typedocs are good too (thanks!), but I did find some areas where they were missing. See below.

I agree completely with @paulo-graca and @artlowel that there are usability issues with the way this feature was implemented. However, as it "works", I'm OK with logging those usability issues as bugs and merging this PR (once the TypeDocs are fixed).

I will also note that yarn did return many warnings when I attempted to build this PR. I used the process described here: https://github.com/DSpace/dspace-angular/blob/master/README.md#testing When running yarn start, I saw ~10+ warnings of this sort:

Warning: Can't resolve all parameters for MetadataSchemaDataService 
in /dspace-angular/src/app/core/data/metadata-schema-data.service.ts: ([object Object], [object Object], [object Object], [object Object], [object Object], [object Object], [object Object], [object Object], [object Object], ?). 
This will become an error in Angular v6.x

I know we already are on Angular 6, so I suspect we are doing something that is no longer a "best practice" in this code...if we don't fix it in this PR, we should log a bug to fix it shortly thereafter.

Show resolved Hide resolved .../+admin/admin-registries/metadata-registry/metadata-registry.reducers.ts
Show resolved Hide resolved ...ies/metadata-schema/metadata-field-form/metadata-field-form.component.ts
Show resolved Hide resolved src/app/core/cache/response.models.ts
Show resolved Hide resolved src/app/core/data/metadata-schema-data.service.ts
Show resolved Hide resolved src/app/core/data/metadatafield-parsing.service.ts
return createSelector(selector, (state: IndexState) => this.getUuidsFromHrefSubstring(state, name, href));
}

private getUuidsFromHrefSubstring(state: IndexState, name: string, href: string): string[] {

This comment has been minimized.

Copy link
@tdonohue

tdonohue Feb 20, 2019

Member

Could we add typedocs or inline comments to this private method. It's not entirely clear to me how this is parsing out the UUIDs.

This comment has been minimized.

Copy link
@artlowel

This comment has been minimized.

Copy link
@tdonohue

tdonohue Feb 21, 2019

Member

@artlowel : It looks like these typedocs/comments were not yet added. I see new comments on a different private method here, but not on the getUuidsFromHrefSubstring() method.

Show resolved Hide resolved src/app/core/index/index.reducer.ts
Show resolved Hide resolved src/app/core/metadata/normalized-metadata-schema.model.ts
Show resolved Hide resolved src/app/core/registry/registry.service.ts
Show resolved Hide resolved src/app/core/registry/registry.service.ts
@paulo-graca
Copy link

left a comment

I agree with @tdonohue we shouldn't hold this PR. I've created a new JIRA issue: DS-4170
Modifying metadata registries usability issues (https://jira.duraspace.org/browse/DS-4170)

@LotteHofstede

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

@tdonohue Because my next PR #362 already solves the issue with the warnings you experienced, I simply created an issue #363 to make sure it won't be forgotten about.

@tdonohue
Copy link
Member

left a comment

👍 Looks great to me now. This is at +2, and all outstanding issues now have tickets. Merging. Thanks @Atmire-Kristof (and all other reviewers/contributors)!

@tdonohue tdonohue merged commit eb818e4 into DSpace:master Feb 21, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 78.109%
Details

@wafflebot wafflebot bot removed the needs review label Feb 21, 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.