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

[Admin] Edit/Update roles via new admin UI #5828

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<%= turbo_frame_tag :edit_role_modal do %>
<%= render component("ui/modal").new(title: t(".title")) do |modal| %>
<%= form_for @role, url: solidus_admin.role_path(@role), html: { id: form_id } do |f| %>
<div class="flex flex-col gap-6 pb-4">
<%= render component("ui/forms/field").text_field(f, :name, class: "required") %>
</div>
<% modal.with_actions do %>
<form method="dialog">
<%= render component("ui/button").new(scheme: :secondary, text: t('.cancel')) %>
</form>
<%= render component("ui/button").new(form: form_id, type: :submit, text: t('.submit')) %>
<% end %>
<% end %>
<% end %>
<% end %>
<%= render component("roles/index").new(page: @page) %>
12 changes: 12 additions & 0 deletions admin/app/components/solidus_admin/roles/edit/component.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# frozen_string_literal: true

class SolidusAdmin::Roles::Edit::Component < SolidusAdmin::BaseComponent
def initialize(page:, role:)
@page = page
@role = role
end

def form_id
dom_id(@role, "#{stimulus_id}_edit_role_form")
end
end
6 changes: 6 additions & 0 deletions admin/app/components/solidus_admin/roles/edit/component.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# Add your component translations here.
# Use the translation in the example in your template with `t(".hello")`.
en:
title: "Edit Role"
cancel: "Cancel"
submit: "Update Role"
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def search_url
end

def row_url(role)
solidus_admin.roles_path(role)
solidus_admin.edit_role_path(role, _turbo_frame: :edit_role_modal)
end

def page_actions
Expand All @@ -29,6 +29,7 @@ def page_actions
def turbo_frames
%w[
new_role_modal
edit_role_modal
]
end

Expand Down
39 changes: 39 additions & 0 deletions admin/app/controllers/solidus_admin/roles_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ module SolidusAdmin
class RolesController < SolidusAdmin::BaseController
include SolidusAdmin::ControllerHelpers::Search

before_action :find_role, only: %i[edit update]

search_scope(:all)
search_scope(:admin) { _1.where(name: "admin") }

Expand Down Expand Up @@ -52,6 +54,39 @@ def create
end
end

def edit
set_index_page

respond_to do |format|
format.html { render component('roles/edit').new(page: @page, role: @role) }
end
end

def update
if @role.update(role_params)
respond_to do |format|
flash[:notice] = t('.success')

format.html do
redirect_to solidus_admin.roles_path, status: :see_other
end

format.turbo_stream do
render turbo_stream: '<turbo-stream action="refresh" />'
end
end
else
set_index_page

respond_to do |format|
format.html do
page_component = component('roles/edit').new(page: @page, role: @role)
render page_component, status: :unprocessable_entity
end
end
end
end

def destroy
@roles = Spree::Role.where(id: params[:id])

Expand All @@ -63,6 +98,10 @@ def destroy

private

def find_role
Copy link
Member

Choose a reason for hiding this comment

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

nit: thoughts on sticking to the convention used in rails scaffold code and use set_role here?

from rails g scaffold

  private
    # Use callbacks to share common setup or constraints between actions.
    def set_post
      @post = Post.find(params[:id])
    end

    # Only allow a list of trusted parameters through.
    def post_params
      params.require(:post).permit(:title, :body)
    end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh for sure, I'd be happy to switch to that! And yes, I feel strongly that we should validate the presence of names on Roles given they are the only defining characteristic at the moment. It would be great if you could take a look at my draft PR: #5833 and let me know of anything else you think I should consider as part of my modifications. Part of that PR is working to pull in some of the most essential pieces from the solidus_user_roles gem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually on second thought, @elia I might leave this as find_role for now, as you can see that this pattern of find_<resource_name> persists across many of our admin controllers. I think if we do change it I should change it for all of them.

Screenshot 2024-08-29 at 11 22 54 AM

Copy link
Member

Choose a reason for hiding this comment

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

Great call, yes, let's do it in one pass for everything 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I can put that bulk rename change up today.

@role = Spree::Role.find(params[:id])
end

def set_index_page
roles = apply_search_to(
Spree::Role.unscoped.order(id: :desc),
Expand Down
2 changes: 2 additions & 0 deletions admin/config/locales/roles.en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,5 @@ en:
success: "Roles were successfully removed."
create:
success: "Role was successfully created."
update:
success: "Role was successfully updated."
2 changes: 1 addition & 1 deletion admin/config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
admin_resources :refund_reasons, except: [:show]
admin_resources :reimbursement_types, only: [:index]
admin_resources :return_reasons, except: [:show]
admin_resources :roles, only: [:index, :new, :create, :destroy]
admin_resources :roles, except: [:show]
admin_resources :adjustment_reasons, except: [:show]
admin_resources :store_credit_reasons, except: [:show]
end
30 changes: 30 additions & 0 deletions admin/spec/features/roles_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,34 @@
end
end
end

context "when editing an existing role" do
let(:query) { "?page=1&q%5Bname_cont%5D=er" }

before do
Spree::Role.create(name: "Reviewer")
visit "/admin/roles#{query}"
find_row("Reviewer").click
expect(page).to have_content("Edit Role")
expect(page).to be_axe_clean
end

it "opens a modal" do
expect(page).to have_selector("dialog")
within("dialog") { click_on "Cancel" }
expect(page).not_to have_selector("dialog")
expect(page.current_url).to include(query)
end

it "successfully updates the existing role" do
fill_in "Name", with: "Publisher"

click_on "Update Role"
expect(page).to have_content("Role was successfully updated.")
expect(page).to have_content("Publisher")
expect(page).not_to have_content("Reviewer")
expect(Spree::Role.find_by(name: "Publisher")).to be_present
expect(page.current_url).to include(query)
end
end
end
47 changes: 47 additions & 0 deletions admin/spec/requests/solidus_admin/roles_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,53 @@
end
end

describe "GET /edit" do
it "renders the edit template with a 200 OK status" do
get solidus_admin.edit_role_path(role)
expect(response).to have_http_status(:ok)
end
end

describe "PATCH /update" do
context "with valid parameters" do
let(:valid_attributes) { { name: "Publisher" } }

it "updates the role" do
patch solidus_admin.role_path(role), params: { role: valid_attributes }
role.reload
expect(role.name).to eq("Publisher")
end

it "redirects to the index page with a 303 See Other status" do
patch solidus_admin.role_path(role), params: { role: valid_attributes }
expect(response).to redirect_to(solidus_admin.roles_path)
expect(response).to have_http_status(:see_other)
end

it "displays a success flash message" do
patch solidus_admin.role_path(role), params: { role: valid_attributes }
follow_redirect!
expect(response.body).to include("Role was successfully updated.")
end
end

context "with invalid parameters" do
let(:invalid_attributes) { { name: "admin" } }

it "does not update the role" do
original_name = role.name
patch solidus_admin.role_path(role), params: { role: invalid_attributes }
role.reload
expect(role.name).to eq(original_name)
end

it "renders the edit template with unprocessable_entity status" do
patch solidus_admin.role_path(role), params: { role: invalid_attributes }
expect(response).to have_http_status(:unprocessable_entity)
end
end
end

describe "DELETE /destroy" do
let!(:role_to_delete) { create(:role) }

Expand Down
Loading