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

UI: fixes link from to show page from entity policies list #17950

Merged
merged 4 commits into from
Nov 16, 2022

Conversation

hellobontempo
Copy link
Contributor

@hellobontempo hellobontempo commented Nov 15, 2022

this PR previously addressed a capabilities issue in the show template, but we decided to approach the solution another day and get to the root of the problem (rather than supply a bandaid fix)

@hellobontempo hellobontempo added this to the 1.13.0-rc1 milestone Nov 15, 2022
zofskeez
zofskeez previously approved these changes Nov 15, 2022
Copy link
Contributor

@zofskeez zofskeez left a comment

Choose a reason for hiding this comment

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

Thanks! I wonder how this worked in the first place if the model was being unloaded on route change by that mixin? Maybe something changed in the run loop timing or something in the new version.

@zofskeez
Copy link
Contributor

A few policy tests are failing but I'm guessing they just need to be updated to have this.capabilities.isLoaded returning true.

@hellobontempo
Copy link
Contributor Author

hellobontempo commented Nov 16, 2022

A few policy tests are failing but I'm guessing they just need to be updated to have this.capabilities.isLoaded returning true.

It ended up being more complicated than that...updated the template so that tests pass.

I'm not sure if changing the capabilities check from this.capabilities.canDelete to this.model.canDelete etc has far-reaching consequences? The capability model seemed to be what was causing the problem...

@zofskeez zofskeez self-requested a review November 16, 2022 01:47
@zofskeez zofskeez dismissed their stale review November 16, 2022 01:47

Reviewing new changes

Copy link
Contributor

@zofskeez zofskeez 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 not familiar with the capabilities model but it looks to me like using lazyCapabilities on the policy model itself is more appropriate. It might be a good idea to get another set of eyes on this just to be sure.

@hellobontempo hellobontempo modified the milestones: 1.13.0-rc1, 1.10.9 Nov 16, 2022
@hellobontempo hellobontempo changed the title UI: fixes accessing updatePath in template before model is loaded UI: fixes link from to show page from entity policies list Nov 16, 2022
@hellobontempo hellobontempo enabled auto-merge (squash) November 16, 2022 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants