From 1709b4d4597f154479ef835ec8deabec083c9e53 Mon Sep 17 00:00:00 2001 From: Denilson Velasquez <66847768+DeeTheDev@users.noreply.github.com> Date: Fri, 29 Nov 2024 13:45:08 -0500 Subject: [PATCH 1/5] Build access to programs by member roles Allow roles to have program enrollments. Members with the same role should have access to the enrollment program resources. This is similar to the enrollments via organization memberships. * Add migration for roles in program enrollments * Associate the member, enrollment and role models * Refactor dataset methods to also access role enrollments * Expose :roles in program enrollments entity --- .../056_role_based_program_access.rb | 47 +++++++++++++++++++ lib/suma/admin_api/entities.rb | 1 + lib/suma/admin_api/program_enrollments.rb | 3 +- lib/suma/fixtures/program_enrollments.rb | 2 +- lib/suma/member.rb | 15 +++++- lib/suma/program.rb | 24 ++++++---- lib/suma/program/enrollment.rb | 12 ++++- lib/suma/role.rb | 2 + .../admin_api/program_enrollments_spec.rb | 11 +++++ spec/suma/member_spec.rb | 24 ++++++++-- spec/suma/program/enrollment_spec.rb | 9 +++- 11 files changed, 129 insertions(+), 21 deletions(-) create mode 100644 db/migrations/056_role_based_program_access.rb diff --git a/db/migrations/056_role_based_program_access.rb b/db/migrations/056_role_based_program_access.rb new file mode 100644 index 00000000..7799d991 --- /dev/null +++ b/db/migrations/056_role_based_program_access.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require "sequel/unambiguous_constraint" + +Sequel.migration do + up do + alter_table(:program_enrollments) do + add_foreign_key :role_id, :roles, on_delete: :cascade, index: true + + drop_constraint(:one_enrollee_set) + add_constraint( + :one_enrollee_set, + Sequel.unambiguous_constraint([:member_id, :organization_id, :role_id]), + ) + + drop_index(:program_id, name: :unique_enrollee_in_program_idx) + add_index [ + Sequel.function(:coalesce, :member_id, 0), + Sequel.function(:coalesce, :organization_id, 0), + Sequel.function(:coalesce, :role_id, 0), + :program_id, + ], + name: :unique_enrollee_in_program_idx, + unique: true + end + end + + down do + alter_table(:program_enrollments) do + drop_index(:program_id, name: :unique_enrollee_in_program_idx) + add_index [ + Sequel.function(:coalesce, :member_id, 0), + Sequel.function(:coalesce, :organization_id, 0), + :program_id, + ], + name: :unique_enrollee_in_program_idx, + unique: true + + drop_constraint(:one_enrollee_set) + add_constraint( + :one_enrollee_set, + Sequel.unambiguous_constraint([:member_id, :organization_id]), + ) + drop_column(:role_id) + end + end +end diff --git a/lib/suma/admin_api/entities.rb b/lib/suma/admin_api/entities.rb index 1252293c..ea68a0aa 100644 --- a/lib/suma/admin_api/entities.rb +++ b/lib/suma/admin_api/entities.rb @@ -136,6 +136,7 @@ class ProgramEnrollmentEntity < BaseEntity expose :program, with: ProgramEntity expose :member, with: MemberEntity expose :organization, with: OrganizationEntity + expose :role, with: RoleEntity expose :approved_at expose :unenrolled_at expose :program_active do |pe| diff --git a/lib/suma/admin_api/program_enrollments.rb b/lib/suma/admin_api/program_enrollments.rb index d8711858..76d27aaa 100644 --- a/lib/suma/admin_api/program_enrollments.rb +++ b/lib/suma/admin_api/program_enrollments.rb @@ -30,7 +30,8 @@ class DetailedProgramEnrollmentEntity < ProgramEnrollmentEntity requires(:program, type: JSON) { use :model_with_id } optional(:member, type: JSON) { use :model_with_id } optional(:organization, type: JSON) { use :model_with_id } - exactly_one_of :member, :organization + optional(:role, type: JSON) { use :model_with_id } + exactly_one_of :member, :organization, :role end end diff --git a/lib/suma/fixtures/program_enrollments.rb b/lib/suma/fixtures/program_enrollments.rb index 101b46e7..83e83be1 100644 --- a/lib/suma/fixtures/program_enrollments.rb +++ b/lib/suma/fixtures/program_enrollments.rb @@ -12,7 +12,7 @@ module Suma::Fixtures::ProgramEnrollments end before_saving do |instance| - instance.member ||= Suma::Fixtures.member.create if instance.organization_id.nil? + instance.member ||= Suma::Fixtures.member.create if instance.organization_id.nil? && instance.role_id.nil? instance.program ||= Suma::Fixtures.program.create instance end diff --git a/lib/suma/member.rb b/lib/suma/member.rb index 0512a458..5611d4dd 100644 --- a/lib/suma/member.rb +++ b/lib/suma/member.rb @@ -103,6 +103,15 @@ class ReadOnlyMode < RuntimeError; end right_primary_key: :organization_id, read_only: true + many_through_many :program_enrollments_via_roles, + [ + [:roles_members, :member_id, :role_id], + ], + class: "Suma::Program::Enrollment", + left_primary_key: :id, + right_primary_key: :role_id, + read_only: true + one_to_many :combined_program_enrollments, class: "Suma::Program::Enrollment", read_only: true, @@ -114,8 +123,10 @@ class ReadOnlyMode < RuntimeError; end self.direct_program_enrollments_dataset.union( self.program_enrollments_via_organizations_dataset, alias: :program_enrollments, - ).order(:program_id, :member_id). - distinct(:program_id) + ).union( + self.program_enrollments_via_roles_dataset, + alias: :program_enrollments, + ).order(:program_id, :member_id).distinct(:program_id) }, eager_loader: (proc do |eo| eo[:rows].each { |p| p.associations[:combined_program_enrollments] = [] } diff --git a/lib/suma/program.rb b/lib/suma/program.rb index 733150c1..27300e2c 100644 --- a/lib/suma/program.rb +++ b/lib/suma/program.rb @@ -46,16 +46,20 @@ def active(as_of:) def enrollment_for(o, as_of:, include: :active) # Use datasets for these checks, since otherwise we'd need to load a bunch of organization memberships, # which could be very memory-intensive. - ds = if o.is_a?(Suma::Member) - self.enrollments_dataset. - where( - Sequel[member: o] | - Sequel[organization_id: o.organization_memberships_dataset.verified.select(:verified_organization_id)], - ) - elsif o.is_a?(Suma::Organization) - self.enrollments_dataset.where(organization: o) - else - raise TypeError, "unhandled type: #{o.class}" + ds = case o + when Suma::Member + self.enrollments_dataset. + where( + Sequel[member: o] | + Sequel[organization_id: o.organization_memberships_dataset.verified.select(:verified_organization_id)] | + Sequel[role_id: o.roles_dataset.select(:id)], + ) + when Suma::Organization + self.enrollments_dataset.where(organization: o) + when Suma::Role + self.enrollments_dataset.where(role: o) + else + raise TypeError, "unhandled type: #{o.class}" end ds = ds.active(as_of:) unless include == :all return ds.first diff --git a/lib/suma/program/enrollment.rb b/lib/suma/program/enrollment.rb index af2c747e..ac521742 100644 --- a/lib/suma/program/enrollment.rb +++ b/lib/suma/program/enrollment.rb @@ -8,6 +8,7 @@ class Suma::Program::Enrollment < Suma::Postgres::Model(:program_enrollments) many_to_one :program, class: "Suma::Program" many_to_one :member, class: "Suma::Member" + many_to_one :role, class: "Suma::Role" many_to_one :organization, class: "Suma::Organization" many_to_one :approved_by, class: "Suma::Member" many_to_one :unenrolled_by, class: "Suma::Member" @@ -28,18 +29,25 @@ def for_members(member) verified. where(member:). select(:verified_organization_id) - ds = self.where(Sequel[member:] | Sequel[organization_id: verified_org_ids]) + ds = self.where( + Sequel[member:] | + Sequel[organization_id: verified_org_ids] | + Sequel[role: Suma::Role.dataset.where(members: member)], + ) ds = ds. left_join( :organization_memberships, {verified_organization_id: Sequel[:program_enrollments][:organization_id]}, - qualify: :deep, + ).left_join( + :roles_members, + {role_id: Sequel[:program_enrollments][:role_id]}, ).select( Sequel[:program_enrollments][Sequel.lit("*")], Sequel.function( :coalesce, Sequel[:program_enrollments][:member_id], Sequel[:organization_memberships][:member_id], + Sequel[:roles_members][:member_id], ).as(:annotated_member_id), ) return ds diff --git a/lib/suma/role.rb b/lib/suma/role.rb index f08e60e0..b12474b1 100644 --- a/lib/suma/role.rb +++ b/lib/suma/role.rb @@ -44,6 +44,8 @@ def cache = @cache ||= Cache.new class: "Suma::Member", join_table: :roles_members + one_to_many :program_enrollments, class: "Suma::Program::Enrollment" + def rel_admin_link = "/role/#{self.id}" def label = self.name.titleize diff --git a/spec/suma/admin_api/program_enrollments_spec.rb b/spec/suma/admin_api/program_enrollments_spec.rb index 4c831442..29b37de5 100644 --- a/spec/suma/admin_api/program_enrollments_spec.rb +++ b/spec/suma/admin_api/program_enrollments_spec.rb @@ -46,6 +46,17 @@ expect(last_response.headers).to include("Created-Resource-Admin") expect(Suma::Program::Enrollment.all).to have_length(1) end + + it "creates the program enrollment for role" do + role = Suma::Role.create(name: "test") + program = Suma::Fixtures.program.create + + post "/v1/program_enrollments/create", program: {id: program.id}, role: {id: role.id} + + expect(last_response).to have_status(200) + expect(last_response.headers).to include("Created-Resource-Admin") + expect(Suma::Program::Enrollment.all).to have_length(1) + end end describe "GET /v1/program_enrollments/:id" do diff --git a/spec/suma/member_spec.rb b/spec/suma/member_spec.rb index 95a456a8..a8a08f83 100644 --- a/spec/suma/member_spec.rb +++ b/spec/suma/member_spec.rb @@ -335,6 +335,7 @@ def skip_verification?(c, list=nil) describe "enrollments" do let(:member) { Suma::Fixtures.member.create } let(:organization) { Suma::Fixtures.organization.create } + let(:role) { Suma::Role.create(name: "role access") } it "can fetch direct enrollments" do e = Suma::Fixtures.program_enrollment(member:).create @@ -347,20 +348,35 @@ def skip_verification?(c, list=nil) expect(member.program_enrollments_via_organizations_dataset.all).to have_same_ids_as(e) end - it "can fetch direct and organizational enrollments" do + it "can fetch role enrollments" do + role = Suma::Role.create(name: "test") + member.add_role(role) + e = Suma::Fixtures.program_enrollment(role:).create + expect(member.program_enrollments_via_roles_dataset.all).to have_same_ids_as(e) + end + + it "can fetch direct, role-based and organizational enrollments" do Suma::Fixtures.organization_membership(member:).verified(organization).create + member.add_role(role) active_via_member = Suma::Fixtures.program_enrollment(member:).create active_via_org = Suma::Fixtures.program_enrollment(organization:).create - expect(member.combined_program_enrollments_dataset.all).to have_same_ids_as(active_via_member, active_via_org) + active_via_role = Suma::Fixtures.program_enrollment(role:).create + expect(member.combined_program_enrollments_dataset.all).to have_same_ids_as(active_via_member, active_via_org, + active_via_role,) eagered_member = Suma::Member.all.first - expect(eagered_member.combined_program_enrollments).to have_same_ids_as(active_via_member, active_via_org) + expect(eagered_member.combined_program_enrollments).to have_same_ids_as(active_via_member, active_via_org, + active_via_role,) end it "filters combined enrollments having the same program" do + r = Suma::Role.create(name: "role access") + member.add_role(r) o = Suma::Fixtures.organization.with_membership_of(member).create program = Suma::Fixtures.program.create # Create the enrollments in a random order to ensure we don't depend on random/chance ordering - [{member:}, {organization: o}].shuffle.each { |p| Suma::Fixtures.program_enrollment.create(program:, **p) } + [{member:}, {organization: o}, {role: r}].shuffle.each do |p| + Suma::Fixtures.program_enrollment.create(program:, **p) + end member_enrollment = member.direct_program_enrollments.first # Prefer the member/direct enrollment over the org/indirect enrollment expect(member.combined_program_enrollments_dataset.all).to have_same_ids_as(member_enrollment) diff --git a/spec/suma/program/enrollment_spec.rb b/spec/suma/program/enrollment_spec.rb index aca38a69..86a6402a 100644 --- a/spec/suma/program/enrollment_spec.rb +++ b/spec/suma/program/enrollment_spec.rb @@ -23,7 +23,7 @@ end end - it "can enroll members and organizations" do + it "can enroll members, organizations and roles" do org1 = Suma::Fixtures.organization.create org1_mem1 = Suma::Fixtures.organization_membership.verified(org1).create org1_mem2 = Suma::Fixtures.organization_membership.verified(org1).create @@ -31,9 +31,13 @@ org2 = Suma::Fixtures.organization.create org2_mem1 = Suma::Fixtures.organization_membership.verified(org2).create + role = Suma::Role.create(name: "test") + member_with_role = Suma::Fixtures.member.with_role(role).create + program = Suma::Fixtures.program.create member_enrollment = Suma::Fixtures.program_enrollment(program:, member: org1_mem1.member).create org_enrollment = Suma::Fixtures.program_enrollment(program:, organization: org2).create + role_enrollment = Suma::Fixtures.program_enrollment(program:, role:).create as_of = Time.now # This member should find the direct enrollment expect(program.enrollment_for(org1_mem1.member, as_of:)).to be === member_enrollment @@ -44,6 +48,9 @@ # The organization and its members should all find the enrollment expect(program.enrollment_for(org2_mem1.member, as_of:)).to be === org_enrollment expect(program.enrollment_for(org2, as_of:)).to be === org_enrollment + + expect(program.enrollment_for(member_with_role, as_of:)).to be === role_enrollment + expect(program.enrollment_for(role, as_of:)).to be === role_enrollment end describe "enrollment_for" do From e1ca075ce78a6f0fc3fec5b9d42e998f7a6b2d53 Mon Sep 17 00:00:00 2001 From: Denilson Velasquez <66847768+DeeTheDev@users.noreply.github.com> Date: Mon, 2 Dec 2024 12:53:33 -0500 Subject: [PATCH 2/5] Implement admin pages for roles update Refactor how 'enrollee' are exposed and listed. Ensure that roles are reflected in programs and enrollments CRUD pages. --- adminapp/src/api.js | 1 + adminapp/src/pages/ProgramDetailPage.jsx | 8 ++-- .../src/pages/ProgramEnrollmentCreatePage.jsx | 1 + .../src/pages/ProgramEnrollmentDetailPage.jsx | 17 ++------ adminapp/src/pages/ProgramEnrollmentForm.jsx | 30 +++++++++++--- .../src/pages/ProgramEnrollmentListPage.jsx | 14 +++---- lib/suma/admin_api/entities.rb | 12 ++++-- lib/suma/admin_api/search.rb | 22 ++++++++++ lib/suma/program/enrollment.rb | 6 ++- spec/suma/admin_api/search_spec.rb | 41 +++++++++++++++++++ 10 files changed, 116 insertions(+), 36 deletions(-) diff --git a/adminapp/src/api.js b/adminapp/src/api.js index 1dd1e3af..b4e96e21 100644 --- a/adminapp/src/api.js +++ b/adminapp/src/api.js @@ -219,6 +219,7 @@ export default { searchVendors: (data) => post(`/adminapi/v1/search/vendors`, data), searchMembers: (data) => post(`/adminapi/v1/search/members`, data), searchOrganizations: (data) => post(`/adminapi/v1/search/organizations`, data), + searchRoles: (data) => post(`/adminapi/v1/search/roles`, data), searchVendorServices: (data) => post(`/adminapi/v1/search/vendor_services`, data), searchCommerceOffering: (data) => post(`/adminapi/v1/search/commerce_offerings`, data), searchPrograms: (data) => post(`/adminapi/v1/search/programs`, data), diff --git a/adminapp/src/pages/ProgramDetailPage.jsx b/adminapp/src/pages/ProgramDetailPage.jsx index 8ffde948..6bedf00d 100644 --- a/adminapp/src/pages/ProgramDetailPage.jsx +++ b/adminapp/src/pages/ProgramDetailPage.jsx @@ -113,9 +113,9 @@ export default function ProgramDetailPage() { /> [ , - {row.member?.name}, - {row.organization?.name}, + {row.enrollee?.name}, + row.enrolleeType, dayjsOrNull(row.approvedAt)?.format("lll"), dayjsOrNull(row.unenrolledAt)?.format("lll"), ]} diff --git a/adminapp/src/pages/ProgramEnrollmentCreatePage.jsx b/adminapp/src/pages/ProgramEnrollmentCreatePage.jsx index b92c343d..252c5b58 100644 --- a/adminapp/src/pages/ProgramEnrollmentCreatePage.jsx +++ b/adminapp/src/pages/ProgramEnrollmentCreatePage.jsx @@ -8,6 +8,7 @@ export default function ProgramEnrollmentCreatePage() { program: {}, member: {}, organization: {}, + role: {}, }; return ( {model.program.name.es}, }, - model.member - ? { - label: "Enrolled Member", - value: {model.member?.name}, - } - : { - label: "Enrolled Organization", - value: ( - - {model.organization?.name} - - ), - }, + { + label: "Enrolled " + model.enrolleeType, + value: {model.enrollee?.name}, + }, { label: "Approved", children: ( diff --git a/adminapp/src/pages/ProgramEnrollmentForm.jsx b/adminapp/src/pages/ProgramEnrollmentForm.jsx index 67a17092..89335da6 100644 --- a/adminapp/src/pages/ProgramEnrollmentForm.jsx +++ b/adminapp/src/pages/ProgramEnrollmentForm.jsx @@ -53,9 +53,9 @@ export default function ProgramEnrollmentForm({ return ( @@ -82,15 +82,16 @@ export default function ProgramEnrollmentForm({ control={} label="Organization" /> + } label="Role" /> - {enrolleeType === "member" ? ( + {enrolleeType === "member" && ( setField("member", mem)} onTextChange={() => setField("member", {})} /> - ) : ( + )} + {enrolleeType === "organization" && ( setField("organization", {})} /> )} + {enrolleeType === "role" && ( + setField("role", role)} + onTextChange={() => setField("role", {})} + /> + )} ); diff --git a/adminapp/src/pages/ProgramEnrollmentListPage.jsx b/adminapp/src/pages/ProgramEnrollmentListPage.jsx index 11f4639f..2681f7e0 100644 --- a/adminapp/src/pages/ProgramEnrollmentListPage.jsx +++ b/adminapp/src/pages/ProgramEnrollmentListPage.jsx @@ -26,19 +26,17 @@ export default function ProgramEnrollmentListPage() { render: (c) => {c.program.name.en}, }, { - id: "member", - label: "Member", + id: "enrollee", + label: "Enrollee", align: "left", - render: (c) => {c.member?.name}, + render: (c) => {c.enrollee?.name}, hideEmpty: true, }, { - id: "organization", - label: "Organization", + id: "enrollee_type", + label: "Enrollee Type", align: "left", - render: (c) => ( - {c.organization?.name} - ), + render: (c) => c.enrolleeType, hideEmpty: true, }, { diff --git a/lib/suma/admin_api/entities.rb b/lib/suma/admin_api/entities.rb index ea68a0aa..8349ebc5 100644 --- a/lib/suma/admin_api/entities.rb +++ b/lib/suma/admin_api/entities.rb @@ -130,13 +130,19 @@ class ProgramEntity < BaseEntity expose :app_link_text, with: TranslatedTextEntity end + class ProgramEnrolleeEntity < BaseEntity + include AutoExposeBase + expose :name do |inst| + inst.is_a?(Suma::Role) ? inst.name.titleize : inst.name + end + end + class ProgramEnrollmentEntity < BaseEntity include AutoExposeBase expose :admin_link expose :program, with: ProgramEntity - expose :member, with: MemberEntity - expose :organization, with: OrganizationEntity - expose :role, with: RoleEntity + expose :enrollee, with: ProgramEnrolleeEntity + expose :enrollee_type expose :approved_at expose :unenrolled_at expose :program_active do |pe| diff --git a/lib/suma/admin_api/search.rb b/lib/suma/admin_api/search.rb index c8ae8fdc..73a8c34f 100644 --- a/lib/suma/admin_api/search.rb +++ b/lib/suma/admin_api/search.rb @@ -224,6 +224,20 @@ def ds_search_or_order_by(column_sym, ds, params) present_collection ds, with: SearchOrganizationEntity end + params do + optional :q, type: String + end + post :roles do + check_role_access!(admin_member, :read, :admin_members) + # role names are sluggified by default. + params[:q] = Suma.to_slug(params[:q]) if params[:q].present? + ds = Suma::Role.dataset + ds = ds_search_or_order_by(:name, ds, params) + ds = ds.limit(15) + status 200 + present_collection ds, with: SearchRoleEntity + end + params do optional :q, type: String end @@ -335,6 +349,14 @@ class SearchOrganizationEntity < BaseEntity expose :name, as: :label end + class SearchRoleEntity < BaseEntity + expose :key, &self.delegate_to(:id, :to_s) + expose :id + expose :admin_link + expose :name + expose :label + end + class SearchVendorServiceEntity < BaseEntity expose :key, &self.delegate_to(:id, :to_s) expose :id diff --git a/lib/suma/program/enrollment.rb b/lib/suma/program/enrollment.rb index ac521742..d35dabc3 100644 --- a/lib/suma/program/enrollment.rb +++ b/lib/suma/program/enrollment.rb @@ -74,8 +74,10 @@ def unenrolled=(v) self.unenrolled_at = v ? Time.now : nil end - # @return [Suma::Member,Suma::Organization] - def enrollee = self.member || self.organization + # @return [Suma::Member,Suma::Organization,Suma::Role] + def enrollee = self.member || self.organization || self.role + + def enrollee_type = self.enrollee.class.name.demodulize def rel_admin_link = "/program-enrollment/#{self.id}" end diff --git a/spec/suma/admin_api/search_spec.rb b/spec/suma/admin_api/search_spec.rb index bc7e1f98..4dacf6b3 100644 --- a/spec/suma/admin_api/search_spec.rb +++ b/spec/suma/admin_api/search_spec.rb @@ -343,6 +343,47 @@ end end + describe "POST /v1/search/roles" do + it "errors without role access" do + replace_roles(admin, Suma::Role.cache.noop_admin) + + post "/v1/search/roles" + + expect(last_response).to have_status(403) + expect(last_response).to have_json_body.that_includes(error: include(code: "role_check")) + end + + it "returns matching roles" do + r1 = Suma::Role.create(name: "spongebob") + r2 = Suma::Role.create(name: "patrick") + + post "/v1/search/roles", q: "bob" + + expect(last_response).to have_status(200) + expect(last_response).to have_json_body.that_includes(items: have_same_ids_as(r1)) + end + + it "returns matching roles label" do + Suma::Role.create(name: "hard worker") + + post "/v1/search/roles", q: "hard" + + expect(last_response).to have_status(200) + expect(last_response).to have_json_body.that_includes(items: [include(label: "Hard Worker")]) + end + + it "returns all results in descending order if no query" do + r1 = Suma::Role.create(name: "x role") + r2 = Suma::Role.create(name: "addmin") + + post "/v1/search/roles" + + expect(last_response).to have_status(200) + expect(last_response_json_body[:items].first).to include(name: r2.name) + expect(last_response_json_body[:items].last).to include(name: r1.name) + end + end + describe "POST /v1/search/vendor_services" do it "errors without role access" do replace_roles(admin, Suma::Role.cache.noop_admin) From e6d368503d962a6a2caf97b97c9f6c3dc1511bda Mon Sep 17 00:00:00 2001 From: Denilson Velasquez <66847768+DeeTheDev@users.noreply.github.com> Date: Mon, 2 Dec 2024 13:12:03 -0500 Subject: [PATCH 3/5] Fix failing test --- spec/suma/admin_api/program_enrollments_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/suma/admin_api/program_enrollments_spec.rb b/spec/suma/admin_api/program_enrollments_spec.rb index 29b37de5..5a1a090a 100644 --- a/spec/suma/admin_api/program_enrollments_spec.rb +++ b/spec/suma/admin_api/program_enrollments_spec.rb @@ -68,7 +68,7 @@ expect(last_response).to have_status(200) expect(last_response).to have_json_body.that_includes( id: enrollment.id, - member: include(id: enrollment.member.id), + enrollee: include(id: enrollment.member.id), ) end From 86c070535afe773bf11c1d47d0e60f59b3a87074 Mon Sep 17 00:00:00 2001 From: Rob Galanakis Date: Tue, 3 Dec 2024 13:40:25 -0800 Subject: [PATCH 4/5] disambiguate enrollment sorting --- lib/suma/member.rb | 4 +-- spec/suma/admin_api/search_spec.rb | 17 +++-------- spec/suma/member_spec.rb | 42 ++++++++++++++++++++-------- spec/suma/program/enrollment_spec.rb | 15 ++++++++++ 4 files changed, 51 insertions(+), 27 deletions(-) diff --git a/lib/suma/member.rb b/lib/suma/member.rb index 5611d4dd..99f2b393 100644 --- a/lib/suma/member.rb +++ b/lib/suma/member.rb @@ -126,7 +126,7 @@ class ReadOnlyMode < RuntimeError; end ).union( self.program_enrollments_via_roles_dataset, alias: :program_enrollments, - ).order(:program_id, :member_id).distinct(:program_id) + ).order(:program_id, :member_id, :organization_id).distinct(:program_id) }, eager_loader: (proc do |eo| eo[:rows].each { |p| p.associations[:combined_program_enrollments] = [] } @@ -135,7 +135,7 @@ class ReadOnlyMode < RuntimeError; end # Get unique enrollments for a program. Prefer direct/member enrollments, # so sort the rows by member_id so NULL member_id rows (indirect/org enrollments) # sort last and are eliminated by the DISTINCT. - order(:program_id, :member_id). + order(:program_id, :member_id, :organization_id). distinct(:program_id) ds.all do |en| m = eo[:id_map][en.member_id || en.values.fetch(:annotated_member_id)].first diff --git a/spec/suma/admin_api/search_spec.rb b/spec/suma/admin_api/search_spec.rb index 4dacf6b3..025b08d4 100644 --- a/spec/suma/admin_api/search_spec.rb +++ b/spec/suma/admin_api/search_spec.rb @@ -353,23 +353,14 @@ expect(last_response).to have_json_body.that_includes(error: include(code: "role_check")) end - it "returns matching roles" do - r1 = Suma::Role.create(name: "spongebob") + it "returns matching roles, using slug naming" do + r1 = Suma::Role.create(name: "sponge_bob") r2 = Suma::Role.create(name: "patrick") - post "/v1/search/roles", q: "bob" + post "/v1/search/roles", q: "sponge bob" expect(last_response).to have_status(200) - expect(last_response).to have_json_body.that_includes(items: have_same_ids_as(r1)) - end - - it "returns matching roles label" do - Suma::Role.create(name: "hard worker") - - post "/v1/search/roles", q: "hard" - - expect(last_response).to have_status(200) - expect(last_response).to have_json_body.that_includes(items: [include(label: "Hard Worker")]) + expect(last_response).to have_json_body.that_includes(items: [include(id: r1.id, label: "Sponge Bob")]) end it "returns all results in descending order if no query" do diff --git a/spec/suma/member_spec.rb b/spec/suma/member_spec.rb index a8a08f83..a6871f47 100644 --- a/spec/suma/member_spec.rb +++ b/spec/suma/member_spec.rb @@ -368,19 +368,37 @@ def skip_verification?(c, list=nil) active_via_role,) end - it "filters combined enrollments having the same program" do - r = Suma::Role.create(name: "role access") - member.add_role(r) - o = Suma::Fixtures.organization.with_membership_of(member).create - program = Suma::Fixtures.program.create - # Create the enrollments in a random order to ensure we don't depend on random/chance ordering - [{member:}, {organization: o}, {role: r}].shuffle.each do |p| - Suma::Fixtures.program_enrollment.create(program:, **p) + describe "with redundant enrollment directly, and through org and role" do + it "returns the direct enrollment" do + r = Suma::Role.create(name: "role access") + member.add_role(r) + o = Suma::Fixtures.organization.with_membership_of(member).create + program = Suma::Fixtures.program.create + # Create the enrollments in a random order to ensure we don't depend on random/chance ordering + [{member:}, {organization: o}, {role: r}].shuffle.each do |p| + Suma::Fixtures.program_enrollment.create(program:, **p) + end + member_enrollment = member.direct_program_enrollments.first + # Prefer the member/direct enrollment over the org/indirect enrollment + expect(member.combined_program_enrollments_dataset.all).to have_same_ids_as(member_enrollment) + expect(Suma::Member.all.last.combined_program_enrollments).to have_same_ids_as(member_enrollment) + end + end + + describe "with redudant enrollment through org and role" do + it "returns the org enrollment" do + r = Suma::Role.create(name: "role access") + member.add_role(r) + o = Suma::Fixtures.organization.with_membership_of(member).create + program = Suma::Fixtures.program.create + # Create the enrollments in a random order to ensure we don't depend on random/chance ordering + [{organization: o}, {role: r}].shuffle.each do |p| + Suma::Fixtures.program_enrollment.create(program:, **p) + end + org_enrollment = member.program_enrollments_via_organizations.first + expect(member.combined_program_enrollments_dataset.all).to have_same_ids_as(org_enrollment) + expect(Suma::Member.all.last.combined_program_enrollments).to have_same_ids_as(org_enrollment) end - member_enrollment = member.direct_program_enrollments.first - # Prefer the member/direct enrollment over the org/indirect enrollment - expect(member.combined_program_enrollments_dataset.all).to have_same_ids_as(member_enrollment) - expect(Suma::Member.all.last.combined_program_enrollments).to have_same_ids_as(member_enrollment) end end end diff --git a/spec/suma/program/enrollment_spec.rb b/spec/suma/program/enrollment_spec.rb index 86a6402a..54a93fa2 100644 --- a/spec/suma/program/enrollment_spec.rb +++ b/spec/suma/program/enrollment_spec.rb @@ -78,4 +78,19 @@ expect(pe.program.enrollment_for(m, as_of:, include: :all)).to be === pe end end + + it "can describe its enrollees" do + pe = Suma::Fixtures.program_enrollment.instance + member = Suma::Fixtures.member.create + organization = Suma::Fixtures.organization.create + role = Suma::Role.create(name: "test role") + pe.set(member:) + expect(pe).to have_attributes(enrollee_type: "Member", enrollee: be === member) + pe.set(member: nil, organization:) + expect(pe).to have_attributes(enrollee_type: "Organization", enrollee: be === organization) + pe.set(organization: nil, role:) + expect(pe).to have_attributes(enrollee_type: "Role", enrollee: be === role) + pe.set(role: nil) + expect(pe).to have_attributes(enrollee_type: "NilClass", enrollee: nil) + end end From 9547a7f2d82f883e9db355956697903affb47e17 Mon Sep 17 00:00:00 2001 From: Rob Galanakis Date: Tue, 3 Dec 2024 13:41:52 -0800 Subject: [PATCH 5/5] fix warning --- spec/suma/fixtures_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/suma/fixtures_spec.rb b/spec/suma/fixtures_spec.rb index 6cd7fba9..76e73a55 100644 --- a/spec/suma/fixtures_spec.rb +++ b/spec/suma/fixtures_spec.rb @@ -15,7 +15,7 @@ Suma::Fixtures.bank_account.with_legal_entity.create Suma::Fixtures.card.with_legal_entity.create Suma::Fixtures.funding_transaction.with_fake_strategy.member(member).create - Suma::Fixtures.geolocation.latlng(10, 2).create + Suma::Fixtures.geolocation.latlng(10, 2).instance Suma::Fixtures.legal_entity.with_contact_info.with_address.create Suma::Fixtures.member_activity.create Suma::Fixtures.member.password.plus_sign.with_email.with_phone.terms_agreed.create