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

Only show the edit buttons if the user has the required permissions #228

Merged
merged 3 commits into from
Dec 13, 2021

Conversation

nbittich
Copy link
Collaborator

@nbittich nbittich commented Dec 9, 2021

No description provided.

@nbittich nbittich requested a review from Windvis December 9, 2021 15:40
Copy link
Contributor

@Windvis Windvis left a comment

Choose a reason for hiding this comment

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

I'm honestly not a big fan of the current implementation. It works, but it doesn't really scale when the application becomes more complex and different users start using the app.

I think https://github.com/Bagaar/ember-permissions would be a nicer way to set this up. It was made for this use case and it's easier to set up more fine grained permissions in the future. I didn't know this was planned this sprint or else I would have documented it in the issue.

The roles can be mapped to a list of permissions (for now only edit I guess) and the helpers can be used instead of the <SecuredArea> component.

The repeated code in the before model hooks would be replaced by setting the route permissions and handling the route-access-denied event.

Now, I assume everything works so it might not be worth redoing everything since we can still refactor it in the future. It would have been nice if you pinged me before starting the implementation though.

app/services/current-session.js Outdated Show resolved Hide resolved
@nbittich
Copy link
Collaborator Author

nbittich commented Dec 9, 2021

I'm honestly not a big fan of the current implementation. It works, but it doesn't really scale when the application becomes more complex and different users start using the app.

I think https://github.com/Bagaar/ember-permissions would be a nicer way to set this up. It was made for this use case and it's easier to set up more fine grained permissions in the future. I didn't know this was planned this sprint or else I would have documented it in the issue.

The roles can be mapped to a list of permissions (for now only edit I guess) and the helpers can be used instead of the <SecuredArea> component.

The repeated code in the before model hooks would be replaced by setting the route permissions and handling the route-access-denied event.

Now, I assume everything works so it might not be worth redoing everything since we can still refactor it in the future. It would have been nice if you pinged me before starting the implementation though.

Well, I set 8 story points for the ticket and did implement it in 1 hour lol. I knew it will take more time than that after the PR would be open. to me it was easier to implement it the way I would, open the discussion through PR and then refactor accordingly. I know you are busy with the change events so didn't want to interrupt. I still have time to try out ember permission tbf.

Copy link
Contributor

@Windvis Windvis left a comment

Choose a reason for hiding this comment

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

Some final minor things!

Copy link
Contributor

@Windvis Windvis left a comment

Choose a reason for hiding this comment

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

Also did a quick test locally and everything seems to work as expected. Nice!

@Windvis Windvis merged commit 56ccbb6 into development Dec 13, 2021
@Windvis Windvis deleted the feature/hide-edit-buttons-for-lezer branch December 13, 2021 09:04
@Windvis Windvis changed the title Hide edit/add buttons for lezer Only show the edit buttons if the user has the required permissions Dec 13, 2021
@Windvis Windvis added the enhancement New feature or request label Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants