From e9e29d25035cecf7934db742c1841f00a5e563ec Mon Sep 17 00:00:00 2001 From: Lucho Cannavo Date: Tue, 26 Jan 2021 18:20:35 -0300 Subject: [PATCH 01/10] Use cache_accesor for organization permissions related methods --- lib/mumuki/domain/helpers/user.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/mumuki/domain/helpers/user.rb b/lib/mumuki/domain/helpers/user.rb index 9758fd3dd..b1a0d6362 100644 --- a/lib/mumuki/domain/helpers/user.rb +++ b/lib/mumuki/domain/helpers/user.rb @@ -81,6 +81,8 @@ def to_s result.map { |org| Mumukit::Platform::Organization.find_by_name!(org) rescue nil }.compact end + cache_accessor :any_granted_organizations, :student_granted_organizations + def has_student_granted_organizations? student_granted_organizations.present? end From 4312ab86e780da4bf0795da3c4c9c67796654d4c Mon Sep 17 00:00:00 2001 From: Lucho Cannavo Date: Mon, 22 Feb 2021 11:49:36 -0300 Subject: [PATCH 02/10] Use safe_current --- app/models/user.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index cc486e9d4..0e481aec9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -245,7 +245,7 @@ def age end def current_organic_context - Organization.current? ? Organization.current : main_organization + Organization.safe_current || main_organization end def current_immersive_context_at(path_item) From b5aaae8ad7eba5239158a509911a027cb821c0c4 Mon Sep 17 00:00:00 2001 From: Lucho Cannavo Date: Mon, 22 Feb 2021 11:51:20 -0300 Subject: [PATCH 03/10] Add test for desired behavior --- spec/lib/user_helpers_spec.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/spec/lib/user_helpers_spec.rb b/spec/lib/user_helpers_spec.rb index 0b43dabd7..b1b5ba263 100644 --- a/spec/lib/user_helpers_spec.rb +++ b/spec/lib/user_helpers_spec.rb @@ -162,5 +162,17 @@ before { organization.settings.immersive = true } end end + + context 'is memoized properly' do + let(:user) { create(:user) } + + before { organization.switch! } + before { user.make_student_of!(organization); user.save! } + before { expect(Mumukit::Platform.organization_class).to receive(:find_by_name!).with('foo').exactly(1).times.and_return(organization) } + + it { expect(user.student_granted_organizations.size).to eq 1 } + it { expect(user.student_granted_organizations).to eq [organization] } + end + end end From 453dbc5e9440509fd3bfac26239b9841d1fd17d4 Mon Sep 17 00:00:00 2001 From: Lucho Cannavo Date: Mon, 22 Feb 2021 11:51:58 -0300 Subject: [PATCH 04/10] Memoize granted organizations related methods --- lib/mumuki/domain/helpers/user.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/mumuki/domain/helpers/user.rb b/lib/mumuki/domain/helpers/user.rb index b1a0d6362..a687b2afc 100644 --- a/lib/mumuki/domain/helpers/user.rb +++ b/lib/mumuki/domain/helpers/user.rb @@ -77,12 +77,13 @@ def to_s ## Accessible organizations - revamp_accessor :any_granted_organizations, :student_granted_organizations do |_, _, result| - result.map { |org| Mumukit::Platform::Organization.find_by_name!(org) rescue nil }.compact + revamp :any_granted_organizations, :student_granted_organizations, selector_transformer: proc { |it| "@__#{it}__".to_sym } do |attr_name, this, hyper| + this.instance_variable_get(attr_name) || + hyper.call.map { |org| Mumukit::Platform::Organization.find_by_name!(org) rescue nil }.compact.tap do |organizations| + this.instance_variable_set(attr_name, organizations) + end end - cache_accessor :any_granted_organizations, :student_granted_organizations - def has_student_granted_organizations? student_granted_organizations.present? end From 893515b49a453ac80c3c875533a09038e869e511 Mon Sep 17 00:00:00 2001 From: Lucho Cannavo Date: Mon, 22 Feb 2021 11:52:10 -0300 Subject: [PATCH 05/10] Add faulty test --- spec/lib/user_helpers_spec.rb | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/spec/lib/user_helpers_spec.rb b/spec/lib/user_helpers_spec.rb index b1b5ba263..3586cff55 100644 --- a/spec/lib/user_helpers_spec.rb +++ b/spec/lib/user_helpers_spec.rb @@ -174,5 +174,14 @@ it { expect(user.student_granted_organizations).to eq [organization] } end + context 'memoization may cause issues with uncontextualized users' do + let(:user) { create(:user) } + + before { user.make_student_of!(organization); user.save! } + before { expect(Mumukit::Platform.organization_class).to receive(:find_by_name!).with('foo').exactly(1).times.and_return(organization) } + + it { expect(user.student_granted_organizations.size).to eq 1 } + it { expect(user.student_granted_organizations).to eq [organization] } + end end end From 26a56214a715f2ddba82642003cdfc3f558ff2ae Mon Sep 17 00:00:00 2001 From: Lucho Cannavo Date: Thu, 25 Feb 2021 13:01:42 -0300 Subject: [PATCH 06/10] Use updated auth --- Gemfile | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Gemfile b/Gemfile index 56e32f4fd..4c7c24fa4 100644 --- a/Gemfile +++ b/Gemfile @@ -12,3 +12,6 @@ group :test do end gem 'codeclimate-test-reporter', :group => :test, :require => nil +# +# gem 'mumukit-auth', path: '/home/lucho/mumuki/mumukit-auth' +gem 'mumukit-auth', github: 'mumuki/mumukit-auth', branch: 'chore-granted_organizations_method_rename' From 6d64936cc9488631bb9612c2f837a8490f581b47 Mon Sep 17 00:00:00 2001 From: Lucho Cannavo Date: Thu, 25 Feb 2021 13:02:24 -0300 Subject: [PATCH 07/10] Update method name --- app/models/user.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index 0e481aec9..5e403948a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -293,7 +293,7 @@ def new_accessible_organizations return [] unless saved_change_to_permissions? old, new = saved_change_to_permissions - new_organizations = (new.any_granted_organizations - old.any_granted_organizations).to_a + new_organizations = (new.any_granted_organizations_names - old.any_granted_organizations_names).to_a Organization.where(name: new_organizations) end From fc7b5eedf3f79aa652002918ea7f037f778280ed Mon Sep 17 00:00:00 2001 From: Lucho Cannavo Date: Thu, 25 Feb 2021 13:03:59 -0300 Subject: [PATCH 08/10] Drop unused method --- lib/mumuki/domain/incognito.rb | 4 ---- spec/lib/user_helpers_spec.rb | 2 -- 2 files changed, 6 deletions(-) diff --git a/lib/mumuki/domain/incognito.rb b/lib/mumuki/domain/incognito.rb index 0ee0834b4..435dd808b 100644 --- a/lib/mumuki/domain/incognito.rb +++ b/lib/mumuki/domain/incognito.rb @@ -12,10 +12,6 @@ def incognito? def ensure_enabled! end - def has_student_granted_organizations? - false - end - def teacher_here? false end diff --git a/spec/lib/user_helpers_spec.rb b/spec/lib/user_helpers_spec.rb index 3586cff55..f69473ccc 100644 --- a/spec/lib/user_helpers_spec.rb +++ b/spec/lib/user_helpers_spec.rb @@ -146,7 +146,6 @@ context 'no organization' do it { expect(user.student_granted_organizations).to eq [] } - it { expect(user.has_student_granted_organizations?).to be false } it { expect(user.has_immersive_main_organization?).to be false } end @@ -155,7 +154,6 @@ before { expect(Mumukit::Platform.organization_class).to receive(:find_by_name!).and_return(organization)} it { expect(user.student_granted_organizations).to eq [organization] } - it { expect(user.has_student_granted_organizations?).to be true } it { expect(user.has_immersive_main_organization?).to be false } context 'when immersive' do From 8ff844eb6136cdff443ad1a7eaa95a939b605e67 Mon Sep 17 00:00:00 2001 From: Lucho Cannavo Date: Thu, 25 Feb 2021 13:04:18 -0300 Subject: [PATCH 09/10] Fix test to actually test the wrong case --- spec/lib/user_helpers_spec.rb | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/spec/lib/user_helpers_spec.rb b/spec/lib/user_helpers_spec.rb index f69473ccc..bf4f7045a 100644 --- a/spec/lib/user_helpers_spec.rb +++ b/spec/lib/user_helpers_spec.rb @@ -168,18 +168,15 @@ before { user.make_student_of!(organization); user.save! } before { expect(Mumukit::Platform.organization_class).to receive(:find_by_name!).with('foo').exactly(1).times.and_return(organization) } - it { expect(user.student_granted_organizations.size).to eq 1 } - it { expect(user.student_granted_organizations).to eq [organization] } - end - - context 'memoization may cause issues with uncontextualized users' do - let(:user) { create(:user) } - - before { user.make_student_of!(organization); user.save! } - before { expect(Mumukit::Platform.organization_class).to receive(:find_by_name!).with('foo').exactly(1).times.and_return(organization) } + it do + user.student_granted_organizations + expect(user.student_granted_organizations.size).to eq 1 + end - it { expect(user.student_granted_organizations.size).to eq 1 } - it { expect(user.student_granted_organizations).to eq [organization] } + it do + user.student_granted_organizations + expect(user.student_granted_organizations).to eq [organization] + end end end end From 5092865bf9b1e2af602ad7280be5ed58e72265e9 Mon Sep 17 00:00:00 2001 From: Lucho Cannavo Date: Thu, 25 Feb 2021 13:04:31 -0300 Subject: [PATCH 10/10] Memoize only student_granted_organizations method --- lib/mumuki/domain/helpers/user.rb | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/lib/mumuki/domain/helpers/user.rb b/lib/mumuki/domain/helpers/user.rb index a687b2afc..b5a8b7bf0 100644 --- a/lib/mumuki/domain/helpers/user.rb +++ b/lib/mumuki/domain/helpers/user.rb @@ -14,8 +14,8 @@ module Mumuki::Domain::Helpers::User :protect!, :protect_delegation!, :protect_permissions_assignment!, - :student_granted_organizations, - :any_granted_organizations, + :student_granted_organizations_names, + :any_granted_organizations_names, :any_granted_roles, to: :permissions @@ -75,17 +75,24 @@ def to_s "#{full_name} <#{email}> [#{uid}]" end - ## Accessible organizations + def organizations_for(organizations_names) + organizations_names.map { |org| Mumukit::Platform::Organization.find_by_name!(org) rescue nil } + end - revamp :any_granted_organizations, :student_granted_organizations, selector_transformer: proc { |it| "@__#{it}__".to_sym } do |attr_name, this, hyper| - this.instance_variable_get(attr_name) || - hyper.call.map { |org| Mumukit::Platform::Organization.find_by_name!(org) rescue nil }.compact.tap do |organizations| - this.instance_variable_set(attr_name, organizations) - end + def any_granted_organizations + organizations_for(any_granted_organizations_names) end - def has_student_granted_organizations? - student_granted_organizations.present? + def student_granted_organizations + granted_organizations_names = student_granted_organizations_names + if @student_granted_organizations_names == granted_organizations_names + @student_granted_organizations + else + organizations_for(granted_organizations_names).tap do |granted_organizations| + @student_granted_organizations_names = granted_organizations_names + @student_granted_organizations = granted_organizations + end + end end def main_organization