-
Notifications
You must be signed in to change notification settings - Fork 70
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
Fix ambassador dashboard exception #843
Conversation
expect(response).to render_template(:show) | ||
end | ||
end | ||
|
||
describe "resources" do | ||
it "renders the ambassador resources" do | ||
get "#{base_url}/resources" | ||
get resources_organization_ambassador_dashboard_path(organization) |
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.
Why switch to routes instead of explicit paths?
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.
The benefits I see to using explicit paths is that it adds one extra bit of test coverage.
I can see an argument for using paths instead, in that we generally use them in the code.
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 preexisting tests, for consistency with the new tests
- In the new tests, because url helpers provide a layer of indirection so that we're not propagating knowledge of the routes throughout the app. (Suppose, for example, we want to refactor underscores to dashes in routes for the sake of SEO.)
I'm not sure I see where the added test coverage would come from, but I'll go ahead ship and iterate since this is a bug fix. Happy to standardize on something and refactor for uniformity.
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.
The "extra test coverage" is a little bit of a stretch, because I'm not sure that it actually is useful test coverage (like shoulda matchers) - but by writing out the route as a URL string, we get routing specs.
There are two routing specs in Bike Index, both for complicated routing parameters, both were used as unit tests when I was verifying things worked as expected; if we switched fully over to request specs and wrote out URLs, these tests wouldn't be necessary.
Merging this was definitely the right choice. I'm adding additional explanation to the request spec migration issue #845
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 to me.
I'm going to make a plea for not using draper, but that can be dealt with separately
Fixes a UninferrableDecoratorError / NoMethodError exception that occurs when accessing the organized ambassador dashboard as a super admin who is not in the ambassador organization.
Fixes #842
https://app.honeybadger.io/projects/35931/faults/50329990#notice-comments