-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add context to Page #2457
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
base: main
Are you sure you want to change the base?
Add context to Page #2457
Changes from all commits
aba82a5
95c0c54
2606678
c50b74c
4c48cd7
493c9cc
bdbed81
3f28b63
ab67ea2
df2889d
033f2f9
bad6c61
cc3b1e7
2dbb7f3
12ffa10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,8 +31,8 @@ class Admin::FoosController < Admin::ApplicationController | |
| # Foo.find_by!(slug: param) | ||
| # end | ||
|
|
||
| # Override this if you have certain roles that require a subset | ||
| # this will be used to set the records shown on the `index` action. | ||
| # Override this if you have certain roles that require a subset. | ||
| # This will be used in all actions except for the `new` and `create` actions | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as #2457 (comment). Create a separate PR, let me know, and I'll merge it straightaway. |
||
| # | ||
| # def scoped_resource | ||
| # if current_user.super_admin? | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the changes to this file? They seem unrelated.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the review. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -3,10 +3,10 @@ | |||||
| describe Admin::CustomersController, type: :controller do | ||||||
| describe "GET index" do | ||||||
| it "passes all customers to the view" do | ||||||
| customer = create(:customer) | ||||||
| customers = create_list(:customer, 5) | ||||||
|
|
||||||
| locals = capture_view_locals { get :index } | ||||||
| expect(locals[:resources]).to eq([customer]) | ||||||
| expect(locals[:resources]).to eq(customers) | ||||||
| end | ||||||
|
|
||||||
| it "applies any scope overrides" do | ||||||
|
|
@@ -47,6 +47,22 @@ | |||||
| expect(locals[:resources].map(&:id)).to eq customers.map(&:id).sort | ||||||
| end | ||||||
|
|
||||||
| context "when the user is an admin" do | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| controller(Admin::CustomersController) do | ||||||
| def authenticate_admin | ||||||
| @current_user = Customer.find_by!(name: "Current User") | ||||||
| end | ||||||
| end | ||||||
|
|
||||||
| let!(:current_user) { create(:customer, name: "Current User") } | ||||||
| let!(:other_customers) { create_list(:customer, 5) } | ||||||
|
|
||||||
| it "passes one customers to the view" do | ||||||
| locals = capture_view_locals { get :index } | ||||||
| expect(locals[:resources]).to eq([current_user]) | ||||||
| end | ||||||
| end | ||||||
|
|
||||||
| context "with alternate sorting attributes" do | ||||||
| controller(Admin::CustomersController) do | ||||||
| def default_sorting_attribute | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,8 +34,8 @@ def pundit_user | |
| end | ||
|
|
||
| describe "GET new" do | ||
| it "raises a Pundit error" do | ||
| expect { get :new }.to raise_error(Pundit::NotAuthorizedError) | ||
| it "allows access to /new" do | ||
| expect { get :new }.not_to raise_error | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something that I'm not sure about with this is that it removes a place where this type of authorization error is tested. Although the specs are a mess anyway already so things like this are difficult to avoid. Perhaps a case for Also, the context setup at the top (
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The other problem here is that these changes remove the intent of the context block: testing namespaced policies. The So either this context needs a new example that tests the different behaviour of the namespaced policy, or a new context testing namespaced policies has to be added elsewhere (eg: payments controller, like it was done with the previous comment).
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another problem from this: the new context you added below ("when the user is not an admin") doesn't do anything that this one couldn't do. Eg: the "POST create" block below could work up here too. |
||
| end | ||
| end | ||
|
|
||
|
|
@@ -106,5 +106,34 @@ def send_request(order:) | |
| end | ||
| end | ||
| end | ||
|
|
||
| context "when the user is not an admin" do | ||
| controller(Admin::OrdersController) do | ||
| def pundit_user | ||
| Customer.find_by(name: "Current User") | ||
| end | ||
| end | ||
|
|
||
| let!(:user) { create(:customer, name: "Current User") } | ||
|
|
||
| describe "GET new" do | ||
| it "allows access to /new" do | ||
| expect { get :new }.not_to raise_error | ||
| end | ||
| end | ||
|
|
||
| describe "POST create" do | ||
| it "allows creating records with the current customer" do | ||
| post( | ||
| :create, | ||
| params: { | ||
| order: attributes_for(:order) | ||
| } | ||
| ) | ||
| expect(response).to redirect_to([:admin, (order = Order.last)]) | ||
| expect(order.customer).to eq(user) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| # standard:enable Lint/ConstantDefinitionInBlock | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,22 @@ | ||||
| require "rails_helper" | ||||
|
|
||||
| # Ensure we keep explicit coverage for authorization errors on `new` actions. | ||||
| # standard:disable Lint/ConstantDefinitionInBlock | ||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Leftover from a previous implementation, perhaps before using |
||||
| RSpec.describe Admin::PaymentsController, type: :controller do | ||||
| controller(Admin::PaymentsController) do | ||||
| include Administrate::Punditize | ||||
|
|
||||
| def pundit_user | ||||
| Customer.find_by(name: "Current User") | ||||
| end | ||||
| end | ||||
|
|
||||
| let!(:user) { create(:customer, name: "Current User") } | ||||
|
|
||||
| describe "GET new" do | ||||
| it "raises a Pundit error" do | ||||
| expect { get :new }.to raise_error(Pundit::NotAuthorizedError) | ||||
| end | ||||
| end | ||||
| end | ||||
| # standard:enable Lint/ConstantDefinitionInBlock | ||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Same as above. |
||||
Uh oh!
There was an error while loading. Please reload this page.