From 06be3a16be38f431ce075dd57d665d3f40fb4cb2 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Thu, 31 Oct 2024 13:37:37 -0700 Subject: [PATCH 1/2] Organizations Members CRUD actions --- .../organizations/members_controller.rb | 74 +++++++++ app/views/organizations/_subject.html.erb | 2 +- .../organizations/members/index.html.erb | 32 ++++ config/locales/en.yml | 10 ++ config/routes.rb | 1 + .../integration/organizations/members_test.rb | 153 ++++++++++++++++++ 6 files changed, 271 insertions(+), 1 deletion(-) create mode 100644 app/controllers/organizations/members_controller.rb create mode 100644 app/views/organizations/members/index.html.erb create mode 100644 test/integration/organizations/members_test.rb diff --git a/app/controllers/organizations/members_controller.rb b/app/controllers/organizations/members_controller.rb new file mode 100644 index 00000000000..770deb7a979 --- /dev/null +++ b/app/controllers/organizations/members_controller.rb @@ -0,0 +1,74 @@ +class Organizations::MembersController < ApplicationController + before_action :redirect_to_signin, only: :index, unless: :signed_in? + before_action :redirect_to_new_mfa, only: :index, if: :mfa_required_not_yet_enabled? + before_action :redirect_to_settings_strong_mfa_required, only: :index, if: :mfa_required_weak_level_enabled? + + before_action :find_organization, only: %i[create update destroy] + before_action :find_membership, only: %i[update destroy] + + layout "subject" + + def index + @organization = Organization.find_by_handle!(params[:organization_id]) + authorize @organization, :list_memberships? + + @memberships = @organization.memberships.includes(:user) + @memberships_count = @organization.memberships.count + end + + def create + username, role = create_membership_params.require([:username, :role]) + # we can open this up in the future to handle email too via find_by_name, + # but it will need to use an invite process to handle non-existing users. + member = User.find_by(handle: username) + if member + membership = authorize @organization.memberships.build(user: member, role:) + if membership.save + flash[:notice] = t(".success", username: member.name) + else + flash[:error] = t(".failure", error: membership.errors.full_messages.to_sentence) + end + else + flash[:error] = t(".failure", error: t(".user_not_found")) + end + redirect_to organization_members_path(@organization) + end + + def update + @membership.attributes = update_membership_params + authorize @membership + if @membership.save + flash[:notice] = t(".success") + else + flash[:error] = t(".failure", error: membership.errors.full_messages.to_sentence) + end + redirect_to organization_members_path(@organization) + end + + def destroy + authorize @membership + flash[:notice] = t(".success") if @membership.destroy + redirect_to organization_members_path(@organization) + end + + private + + def find_organization + @organization = Organization.find_by_handle!(params[:organization_id]) + authorize @organization, :manage_memberships? + end + + def find_membership + handle = params.permit(:id).require(:id) + @member = User.find_by_slug!(handle) + @membership = @organization.memberships.find_by!(user: @member) + end + + def create_membership_params + params.permit(membership: %i[username role]).require(:membership) + end + + def update_membership_params + params.permit(membership: %i[role]).require(:membership) + end +end diff --git a/app/views/organizations/_subject.html.erb b/app/views/organizations/_subject.html.erb index 78ed96b9266..156e2bab480 100644 --- a/app/views/organizations/_subject.html.erb +++ b/app/views/organizations/_subject.html.erb @@ -18,7 +18,7 @@ <%= nav.link t("layouts.application.header.dashboard"), organization_path(@organization), name: :dashboard, icon: "space-dashboard" %> <%= nav.link t("organizations.show.history"), organization_path(@organization), name: :subscriptions, icon: "notifications" %> <%= nav.link t("organizations.show.gems"), organization_gems_path(@organization), name: :gems, icon: "gems" %> - <%= nav.link t("organizations.show.members"), organization_path(@organization), name: :organizations, icon: "organizations" %> + <%= nav.link t("organizations.show.members"), organization_members_path(@organization), name: :members, icon: "organizations" %> <% if policy(@organization).edit? %> <%= nav.link t("layouts.application.header.settings"), edit_organization_path(@organization), name: :settings, icon: "settings" %> <% end %> diff --git a/app/views/organizations/members/index.html.erb b/app/views/organizations/members/index.html.erb new file mode 100644 index 00000000000..88f4b0f2333 --- /dev/null +++ b/app/views/organizations/members/index.html.erb @@ -0,0 +1,32 @@ +<% + add_breadcrumb t("breadcrumbs.org_name", name: @organization.handle), organization_path(@organization) + add_breadcrumb t("breadcrumbs.members") +%> + +<% content_for :subject do %> + <%= render "organizations/subject", organization: @organization, current: :members %> +<% end %> + +

<%= t("organizations.show.members") %>

+ +<%= render CardComponent.new do |c| %> + <%= c.head do %> + <%= c.title t("organizations.show.members"), icon: :organizations %> + <% end %> + <% if @memberships.empty? %> + <%= prose do %> + <%= t('organizations.show.no_members') %> + <% end %> + <% else %> + <%= c.divided_list do %> + <% @memberships.each do |membership| %> + <%= c.list_item_to(profile_path(membership.user.handle)) do %> +
+

<%= membership.user.name %>

+

<%= membership.role %>

+
+ <% end %> + <% end %> + <% end %> + <% end %> +<% end %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 0a7860a919c..cf0670326b4 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -428,6 +428,16 @@ en: members: Members no_history: No events yet no_gems: No gems yet + members: + create: + failure: "Failed to add member: %{error}" + success: "Member added!" + user_not_found: "User not found" + destroy: + success: "User was removed from the organization" + update: + failure: "Failed to update member: %{error}" + success: "User was updated" pages: about: contributors_amount: "%{count} Rubyists" diff --git a/config/routes.rb b/config/routes.rb index 7207d02133b..aa4852808a6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -290,6 +290,7 @@ end resources :organizations, only: %i[index show edit update], constraints: { id: Patterns::ROUTE_PATTERN } do resources :gems, only: :index, controller: 'organizations/gems' + resources :members, only: %i[index create update destroy], controller: 'organizations/members' end end diff --git a/test/integration/organizations/members_test.rb b/test/integration/organizations/members_test.rb new file mode 100644 index 00000000000..a95fbb98939 --- /dev/null +++ b/test/integration/organizations/members_test.rb @@ -0,0 +1,153 @@ +require "test_helper" + +class Organizations::MembersTest < ActionDispatch::IntegrationTest + setup do + @user = create(:user, remember_token_expires_at: Gemcutter::REMEMBER_FOR.from_now) + post session_path(session: { who: @user.handle, password: PasswordHelpers::SECURE_TEST_PASSWORD }) + end + + test "index should render Not Found org" do + get "/organizations/notfound/members" + + assert_response :not_found + end + + test "index should render Forbidden" do + create(:organization, handle: "chaos") + + get "/organizations/chaos/members" + + assert_response :forbidden + end + + test "should get index" do + create(:organization, owners: [@user], handle: "chaos") + + get "/organizations/chaos/members" + + assert_response :success + assert page.has_content?("Members") + end + + test "create should return Not Found org" do + post "/organizations/notfound/members", params: { membership: { role: "owner" } } + + assert_response :not_found + end + + test "create should return Forbidden when trying to create your own membership" do + create(:organization, handle: "chaos") + + post "/organizations/chaos/members", params: { membership: { username: @user.id, role: "maintainer" } } + + assert_response :forbidden + end + + test "create membership with bad role should not work" do + organization = create(:organization, owners: [@user], handle: "chaos") + bdfl = create(:user, handle: "bdfl") + + post "/organizations/chaos/members", params: { membership: { username: bdfl.handle, role: "bdfl" } } + + assert_redirected_to organization_members_path(organization) + follow_redirect! + + assert page.has_content?("Failed to add member: Role is not included in the list") + assert_nil organization.unconfirmed_memberships.find_by(user_id: bdfl.id) + end + + test "create membership by email should not work (yet)" do + organization = create(:organization, owners: [@user], handle: "chaos") + maintainer = create(:user, handle: "maintainer") + + post "/organizations/chaos/members", params: { membership: { username: maintainer.email, role: "maintainer" } } + + assert_redirected_to organization_members_path(organization) + follow_redirect! + + assert page.has_content?("Failed to add member: User not found") + assert_nil organization.unconfirmed_memberships.find_by(user_id: maintainer.id) + end + + test "should create a membership by handle" do + organization = create(:organization, owners: [@user], handle: "chaos") + maintainer = create(:user, handle: "maintainer") + + post "/organizations/chaos/members", params: { membership: { username: maintainer.handle, role: "maintainer" } } + + assert_redirected_to organization_members_path(organization) + membership = organization.unconfirmed_memberships.find_by(user_id: maintainer.id) + + assert membership + assert_predicate membership, :maintainer? + refute_predicate membership, :confirmed? + end + + test "update should return Not Found org" do + patch "/organizations/notfound/members/notfound", params: { membership: { role: "owner" } } + + assert_response :not_found + end + + test "update should return Not Found membership" do + create(:organization, owners: [@user], handle: "chaos") + + patch "/organizations/chaos/members/notfound", params: { membership: { role: "owner" } } + + assert_response :not_found + end + + test "update should return Forbidden" do + organization = create(:organization, handle: "chaos") + membership = create(:membership, :maintainer, user: @user, organization: organization) + + patch "/organizations/chaos/members/#{@user.handle}", params: { membership: { role: "owner" } } + + assert_response :forbidden + end + + test "should update" do + organization = create(:organization, owners: [@user], handle: "chaos") + maintainer = create(:user, handle: "maintainer") + membership = create(:membership, :maintainer, user: maintainer, organization: organization) + + patch "/organizations/chaos/members/#{maintainer.handle}", params: { membership: { role: "owner" } } + + assert_redirected_to organization_members_path(organization) + assert_predicate membership.reload, :owner? + end + + test "destroy should return Not Found org" do + delete "/organizations/notfound/members/notfound" + + assert_response :not_found + end + + test "destroy should return Not Found membership" do + create(:organization, owners: [@user], handle: "chaos") + + delete "/organizations/chaos/members/notfound" + + assert_response :not_found + end + + test "destroy should return Forbidden" do + organization = create(:organization, handle: "chaos") + membership = create(:membership, :maintainer, user: @user, organization: organization) + + delete "/organizations/chaos/members/#{@user.handle}" + + assert_response :forbidden + end + + test "should destroy a membership" do + organization = create(:organization, handle: "chaos", owners: [@user]) + maintainer = create(:user, handle: "maintainer") + membership = create(:membership, :maintainer, user: maintainer, organization: organization) + + delete "/organizations/chaos/members/#{maintainer.handle}" + + assert_redirected_to organization_members_path(organization) + assert_nil Membership.find_by(id: membership.id) + end +end From 2db498aaa2d797201334d02c1f325ff131e7a644 Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Thu, 21 Nov 2024 19:41:55 -0800 Subject: [PATCH 2/2] use section for card component --- app/views/components/card_component.rb | 2 +- test/views/card_component_test.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/components/card_component.rb b/app/views/components/card_component.rb index 7858d1664bc..236975a1a7f 100644 --- a/app/views/components/card_component.rb +++ b/app/views/components/card_component.rb @@ -6,7 +6,7 @@ class CardComponent < ApplicationComponent def view_template(&) color = "bg-white dark:bg-black border border-neutral-200 dark:border-neutral-800 text-neutral-900 dark:text-white " box = "w-full px-4 py-6 md:p-10 mb-10 rounded-md shadow overflow-hidden" - article(**classes(color, box), &) + section(**classes(color, box), &) end def head(title = nil, icon: nil, count: nil, url: nil, **options, &block) diff --git a/test/views/card_component_test.rb b/test/views/card_component_test.rb index a36daccf46f..20e355e2d7e 100644 --- a/test/views/card_component_test.rb +++ b/test/views/card_component_test.rb @@ -16,7 +16,7 @@ def render(...) end end - assert_selector "article" + assert_selector "section" assert_selector "h3", text: "Gems" assert_selector "svg.fill-orange" assert_selector "span", text: "3" @@ -36,7 +36,7 @@ def render(...) end end - assert_selector "article" + assert_selector "section" assert_selector "h3", text: "History" assert_text "content" refute_text "View all"