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

Enhancing UserHasAccess component #472

Merged
merged 3 commits into from
Jul 11, 2022

Conversation

Arjun-Go
Copy link
Contributor

@Arjun-Go Arjun-Go commented Jul 1, 2022

Requirements

  • This PR has a title that briefly describes the work done, including the ticket number if there is a ticket.

For changes to apps

If applicable

  • My work includes tests or is validated by existing tests.
  • I have updated the esm-framework mock to reflect any API changes I have made.

Summary

Existing UserHasAccess component returns null when the user is unauthorised. In this PR, we have enhanced the component such that the user sees an error message / notification or can redirect to a given URL.

Screenshots

image

@Arjun-Go Arjun-Go marked this pull request as draft July 1, 2022 12:34
@Arjun-Go Arjun-Go marked this pull request as ready for review July 1, 2022 12:35
@github-actions
Copy link
Contributor

github-actions bot commented Jul 1, 2022

File size impact

Impact on file sizes when merging user-access-enhancement into master.

@openmrs/esm-devtools-app (+86%)
Files new size
packages/apps/esm-devtools-app/dist/889.js 1.47 MB (+692 kB / +89%) ↗️
Unmodified (4) 24.3 kB (0 B / +0%) 👻
Total (5) 1.5 MB (+692 kB / +86%) ↗️
@openmrs/esm-implementer-tools-app (+27%)
Files new size
packages/apps/esm-implementer-tools-app/dist/889.js 1.47 MB (+692 kB / +89%) ↗️
Unmodified (21) 1.78 MB (0 B / +0%) 👻
Total (22) 3.25 MB (+692 kB / +27%) ↗️
@openmrs/esm-login-app (+33%)
Files new size
packages/apps/esm-login-app/dist/889.js 1.47 MB (+692 kB / +89%) ↗️
Unmodified (27) 1.32 MB (0 B / +0%) 👻
Total (28) 2.79 MB (+692 kB / +33%) ↗️
@openmrs/esm-offline-tools-app (+32%)
Files new size
packages/apps/esm-offline-tools-app/dist/889.js 1.47 MB (+692 kB / +89%) ↗️
Unmodified (29) 1.4 MB (0 B / +0%) 👻
Total (30) 2.87 MB (+692 kB / +32%) ↗️
@openmrs/esm-primary-navigation-app (+32%)
Files new size
packages/apps/esm-primary-navigation-app/dist/889.js 1.47 MB (+692 kB / +89%) ↗️
Unmodified (17) 1.35 MB (0 B / +0%) 👻
Total (18) 2.82 MB (+692 kB / +32%) ↗️
@openmrs/esm-app-shell (+0.002%)
Files new size
packages/shell/esm-app-shell/dist/openmrs.js 1.53 MB (+54 B / +0.004%) ↗️
packages/shell/esm-app-shell/dist/service-worker.js 206 kB (0 B / +0%) 👻
Unmodified (2) 519 kB (0 B / +0%) 👻
Total (4) 2.25 MB (+54 B / +0.002%) ↗️
Generated by @jsenv/file-size-impact during Report bundle size#2647624120 on 6aef14f

@manuelroemer
Copy link
Member

Hey @Arjun-Go, thanks for the PR! I think that this concrete approach can potentially cause unwanted side-effects because it is making the component do too much/make too many assumptions about what the unauthorized fallback should look like. Consider that UserHasAccess can be used anywhere - both for wrapping entire pages, but also only small sections, e.g. little input fields. If one page used that component 10 times (e.g. for various little form fields), you'd get 10 error messages when the page loads (given that you don't have the required privileges).

I think that, to achieve your goal of rendering some kind of fallback when a user doesn't have access, it would be much better to leave that fallback as a placeholder, e.g. like this (treat the following code as a suggestion, not the ultimate way it should look like):

const fallbackWhenUnauthorized = (
    <div className="omrs-inline-notifications-container">
        <InlineNotification
            title="Unauthorised"
            subtitle="You cannot access this page."
            kind="error"
        />
    </div>
);

// Then somewhere else:
<UserHasAccess unauthorizedChildren={fallbackWhenUnauthorized} ...otherProps />

Moving the responsiblity to the user of the component keeps UserHasAccess clean and lean while also allowing each user to determine the best fallback content to render in that specific situation.

@ibacher
Copy link
Member

ibacher commented Jul 1, 2022

I absolutely agree with both @manuelroemer's assessment and suggestion here.

@Arjun-Go
Copy link
Contributor Author

Arjun-Go commented Jul 4, 2022

Hi @manuelroemer, the suggestion really helped. I've refactored the code to incorporate your comments. Thanks!!


export interface UserHasAccessProps {
interface UserHasAccessProps {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should still be exported

privilege: string;
unauthorisedResponse?: React.ReactNode | undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

response doesn't make sense as a name here. It's not a response, it's a thing to render. I suggest fallbackChildren. Or just fallback. Note that | undefined is in most cases redundant with ?:. Also, as an aside, we normatively use US spelling.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
unauthorisedResponse?: React.ReactNode | undefined;
fallback?: React.ReactNode;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure @brandones , changes have been made accordingly.

@brandones brandones merged commit 800fe7e into openmrs:master Jul 11, 2022
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