-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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: Ensure we show a special readonly page for intentions #11767
Conversation
{{#let api.data as |item|}} | ||
{{#let | ||
api.data | ||
(not (can 'write intention' item=api.data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line here was the bugfix, previously the readonly
variable just below on line 63 was undefined always 😬
Most of the other change here is to test this so it doesn't happen again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe an opportunity to use cannot
?
(not (can 'write intention' item=api.data)) | |
(cannot 'write intention' item=api.data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the codebase we prefer using not (can "thing")
, that way folks always use the same method to flip the conditional no matter if it's using can
or some other flippable helper. So I'll probably leave this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One little suggestion and one clarification regarding a bit of testing code.
{{#let api.data as |item|}} | ||
{{#let | ||
api.data | ||
(not (can 'write intention' item=api.data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe an opportunity to use cannot
?
(not (can 'write intention' item=api.data)) | |
(cannot 'write intention' item=api.data) |
export default function(assert) { | ||
return steps(assert).then('I should find a file', function() { | ||
assert.ok(true, this.step); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you end up using this step? Looks like it always passes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah this is where it's just lack of context into the addon we use for our acceptance tests.
The addon requires these files in order to run (the idea is you write a entire set of steps for every test, which seems like a lot of repetitive work). Instead we have about 25-30 reusable steps that we use for all our tests. Unfortunately the addon requires these files to still exist and generates them exactly like this for each and every acceptance test. If you delete them it breaks. Therefore we have oodles of these files in a nested folder structure that are all exactly the same. It's annoying, but I just pretend they aren't there.
We had a PR a while ago to stop it from doing this which made it much nicer, but unfortunately it didn't go in and has been the root of confusion both previous to that PR and ever since 😹
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes, that isn’t ideal. Still, explanation makes sense for the context of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving as review has taken place with non-approving reviewer.
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/525536. |
🍒✅ Cherry pick of commit be23aab onto |
🍒✅ Cherry pick of commit be23aab onto |
I noticed we were no longer showing our specially designed read only page for intentions due to an undefined handlebars variable, so I fixed that up and added an acceptance test to prevent this happening here again.
It seems like this bug is only/specifically in 1.10.4 (and currently unreleased 1.11 branch)