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

resource: check permissions on detail view #37

Merged
merged 1 commit into from
Nov 6, 2019
Merged

resource: check permissions on detail view #37

merged 1 commit into from
Nov 6, 2019

Conversation

sebdeleze
Copy link

  • Checks permissions for deleting or updating a resource on detail view.
  • Returns an observable for getting the record instead of the record data directly.
  • Converts returns of functions canAdd and canEdit into observable.
  • Creates a service for managing records in user interface.

Co-Authored-by: Bertrand Zuchuat bertrand.zuchuat@rero.ch
Co-Authored-by: Sébastien Délèze sebastien.deleze@rero.ch

@sebdeleze sebdeleze requested review from Garfield-fr and jma November 6, 2019 08:21
@sebdeleze
Copy link
Author

Warning, when this PR will be merged, canUpdate and canAdd checks have to return an observable in rero-ils-ui instead of a value directly.

<a href="javascript:void(0);" class="btn btn-sm btn-secondary" routerLink="../../edit/{{ record.metadata.pid }}">
<div class="col-3 text-right" *ngIf="record">
<a href class="btn btn-sm btn-primary"
routerLink="../../edit/{{ record.metadata.pid }}" *ngIf="canUpdateResult">
Copy link
Contributor

Choose a reason for hiding this comment

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

canUpdateResult.can? or better canUpdateResult && canUpdateResult.can

Copy link
Author

Choose a reason for hiding this comment

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

canUpdateResult is not an object, but directly resolved as a boolean. It's different in regard to canDeleteResult because the latter returns an object with the boolean and eventually a message.

Copy link
Author

Choose a reason for hiding this comment

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

ok, I saw your comment later on, which is relevent. Il will create an interface ActionStatus which returns always an object for all actions

</div>
</div>
<hr class="my-4">
<a href="#" (click)="goBack($event)">&laquo; {{ 'Back' | translate }}</a>
<a href="#" (click)="goBack($event)">&laquo; {{ 'Back' | translate }}</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of href you can use routerLink="" or type="button" or use button

Copy link
Author

Choose a reason for hiding this comment

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

I converted link to button when there's no link to follow, but an action on click

/**
* Record can be updated ?
*/
canUpdateResult = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we uniformize by returning a ActionStatus?

Copy link
Author

Choose a reason for hiding this comment

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

done

* @param message - message to display into modal
*/
public showDeleteMessage(event: Event, message: string) {
event.preventDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

is it really required?

Copy link
Author

Choose a reason for hiding this comment

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

It's required when a link is clicked. I converted links to buttons.


@Component({
template: `
<h1>Record of type "{{ type }}" #{{ record.metadata.pid }}</h1>
{{ record|json }}
<div *ngIf="record">
Copy link
Contributor

Choose a reason for hiding this comment

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

ng-container?

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -79,6 +79,7 @@ export class MainFieldsManagerComponent implements OnInit {
// disable FormControls for hidden fields
for (const fieldSet of Object.values(this.layoutRefField)) {
if (this.isHidden(fieldSet)) {
/* tslint:disable:no-string-literal */
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this and use: fieldSet.items

Copy link
Author

Choose a reason for hiding this comment

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

done

ignoreBackdropClick: true,
initialState: {
title: this.translate.instant('Confirmation'),
body: this.translate.instant('Do you really want to delete this record ?'),
Copy link
Contributor

Choose a reason for hiding this comment

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

record ? => record?

Copy link
Author

Choose a reason for hiding this comment

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

done


/**
* Delete a record by its PID.
* @param pid - string, PID to delete
Copy link
Contributor

Choose a reason for hiding this comment

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

@param type is missing, you can also add @returns see: https://jsdoc.app/tags-returns.html

Copy link
Author

Choose a reason for hiding this comment

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

done, but the @returns must be set to all functions/methods in project

@sebdeleze sebdeleze requested a review from jma November 6, 2019 13:33
* Checks permissions for deleting or updating a resource on detail view.
* Returns an observable for getting the record instead of the record data directly.
* Converts returns of functions canAdd and canEdit into observable.
* Creates a service for managing records in user interface.
* Fixes issue #2

Co-Authored-by: Bertrand Zuchuat <bertrand.zuchuat@rero.ch>
Co-Authored-by: Sébastien Délèze <sebastien.deleze@rero.ch>
@sebdeleze sebdeleze merged commit d5215cd into rero:dev Nov 6, 2019
@sebdeleze sebdeleze deleted the sed-detail-permission-checks branch November 6, 2019 15:06
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.

3 participants