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

editor: check permission before showing the editor #43

Merged
merged 1 commit into from
Nov 15, 2019
Merged

editor: check permission before showing the editor #43

merged 1 commit into from
Nov 15, 2019

Conversation

sebdeleze
Copy link

  • Checks permission to update the record in component init and redirects to previous page if user has no access.
  • Fixes Securize update route #39

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

@sebdeleze sebdeleze requested review from Garfield-fr and jma November 6, 2019 16:19
@AoNoOokami AoNoOokami self-requested a review November 8, 2019 08:03
@@ -15,12 +15,9 @@
 along with this program. If not, see <http://www.gnu.org/licenses/>.
-->
<div class="alert alert-danger" *ngIf="error">{{ error | translate }}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line still valid?

Copy link
Author

Choose a reason for hiding this comment

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

Yes it's still valid, because it displays an error when the user can access to the page, but the API is not properly responding.

config.canUpdate(this.record).subscribe((result: ActionStatus) => {
this.updateStatus = result;
});
if (typeof this.route.snapshot.data.adminMode !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

typeof?

Copy link
Author

Choose a reason for hiding this comment

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

You're right, as I read here, the best way to do this is :

if (this.route.snapshot.data.adminMode != null) {
    this.adminMode = this.route.snapshot.data.adminMode;
}

Copy link
Author

Choose a reason for hiding this comment

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

done in other PR

_('You cannot update this record'),
_(this.recordType)
);
this.location.back();
Copy link
Contributor

Choose a reason for hiding this comment

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

What's happens with a direct link?

Copy link
Author

Choose a reason for hiding this comment

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

The user will be redirected to the previous website or on the browser welcome page if the direct link was the first seen page.

<ng-core-record-search [adminMode]="true" [currentType]="currentType" [types]="types" [detailUrl]="detailUrl"
[showSearchInput]="showSearchInput" [aggFilters]="aggFilters" [q]="q" [page]="page" [size]="size" [inRouting]="true"
(parametersChanged)="updateUrl($event)">
</ng-core-record-search>
Copy link
Contributor

Choose a reason for hiding this comment

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

please add an empty line.

Copy link
Author

Choose a reason for hiding this comment

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

done in other PR

* Component initialisation.
*/
ngOnInit() {
const queryParams = this.route.snapshot.queryParams;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add an observable to detect queryParams changes?

if (['q', 'page', 'size'].includes(key)) {
this[key] = queryParams[key];
} else {
if (Array.isArray(queryParams[key]) === false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if not?

Copy link
Author

Choose a reason for hiding this comment

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

I don't understand this comment, there's a "else" with this condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

I propose here:
if (!Array.isArray(queryParams[key]))

Comment on lines 172 to 184
return this.router.url.split('?')[0].replace(/(^.*\/)(.*)$/, `$1${type}`);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use Angular to parse the URL: https://angular.io/api/router/Router#parseUrl

Copy link
Author

Choose a reason for hiding this comment

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

done in other PR

* Checks permission to update the record in component init and redirects to previous page if user has no access.
* Fixes #39

Co-Authored-by: Sébastien Délèze <sebastien.deleze@rero.ch>
@sebdeleze sebdeleze merged commit d5c2914 into rero:dev Nov 15, 2019
@sebdeleze sebdeleze deleted the sed-check-rights-on-editor branch November 15, 2019 12:51
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.

4 participants