favor active sub-account for branding

fixes FOO-2312
flag  = none

when the user has only 1 active enrollment in 1 specific sub-account in
the associated account hierarchy, choose the branding for that
sub-account

:~::::::::~~:
::test plan::
;;;;;;;;;;;;;

- create two sub-accounts with courses and a student
- create a course in your root account and enroll that student too
- brand your root and two sub-acounts differently
- with 3 active enrollments, your student's dashboard should display the
  root account theme
- conclude the enrollments until you end up with only 1 enrollment and
  then verify:

  1. with root account course enrollment ACTIVE, the branding should
     follow the root account's
  2. with sub-account 1 course enrollment ACTIVE, the branding should
     follow that sub-account's
  3. with sub-account 2 course enrollment ACTIVE, the branding should
     follow that sub-account's

Change-Id: I909b1ad20106926deb7dc9b2b54e4ca1ce4fc8a9
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/273188
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Charley Kline <ckline@instructure.com>
QA-Review: Charley Kline <ckline@instructure.com>
Product-Review: Ahmad Amireh <ahmad@instructure.com>
This commit is contained in:
Ahmad Amireh 2021-09-08 16:16:22 -06:00
parent c94ec0a964
commit 924d378431
5 changed files with 527 additions and 111 deletions

View File

@ -840,7 +840,12 @@ module ApplicationHelper
# user profile, show the branding for the lowest account where all my enrollments are. eg:
# if I'm at saltlakeschooldistrict.instructure.com, but I'm only enrolled in classes at
# Highland High, show Highland's branding even on the dashboard.
@brand_account = @current_user.common_account_chain(@domain_root_account).last
GuardRail.activate(:secondary) do
@brand_account = BrandAccountChainResolver.new(
user: @current_user,
root_account: @domain_root_account
).resolve
end
end
# If we're not logged in, or we have no enrollments anywhere in domain_root_account,

View File

@ -1825,47 +1825,6 @@ class User < ActiveRecord::Base
pseudonym.account rescue Account.default
end
# this finds the reverse account chain starting at in_root_account and ending
# at the lowest account such that all of the accounts to which the user is
# associated with, which descend from in_root_account, descend from one of the
# accounts in the chain. In other words, if the users associated accounts
# made a tree, it would be the chain between the root and the first branching
# point.
def common_account_chain(in_root_account)
GuardRail.activate(:secondary) do
rid = in_root_account.id
accts = associated_accounts.where("accounts.id = ? OR accounts.root_account_id = ?", rid, rid)
return [] if accts.blank?
children = accts.each_with_object({}) do |acct, hash|
pid = acct.parent_account_id
if pid.present?
hash[pid] ||= []
hash[pid] << acct
end
end
enrollment_account_ids = in_root_account
.all_enrollments
.current_and_concluded
.where(user_id: self)
.joins(:course)
.distinct
.pluck(:account_id)
longest_chain = [in_root_account]
while true
break if enrollment_account_ids.include?(longest_chain.last.id)
next_children = children[longest_chain.last.id]
break unless next_children.present? && next_children.count == 1
longest_chain << next_children.first
end
longest_chain
end
end
def courses_with_primary_enrollment(association = :current_and_invited_courses, enrollment_uuid = nil, options = {})
cache_key = [association, enrollment_uuid, options].cache_key
@courses_with_primary_enrollment ||= {}

View File

@ -0,0 +1,111 @@
# frozen_string_literal: true
#
# Copyright (C) 2021 - present Instructure, Inc.
#
# This file is part of Canvas.
#
# Canvas is free software: you can redistribute it and/or modify it under
# the terms of the GNU Affero General Public License as published by the Free
# Software Foundation, version 3 of the License.
#
# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
# details.
#
# You should have received a copy of the GNU Affero General Public License along
# with this program. If not, see <http://www.gnu.org/licenses/>.
# Find the reverse account chain starting at the specified root account and
# ending at the lowest sub-account such that all of the accounts with which
# the user is associated (e.g. through an enrollment), and which descend from
# that root account, also descend from one of the accounts in the chain.
#
# In other words, if the users associated accounts made a tree, it would be the
# chain between the root and the first branching point:
#
# /-E
# A-B-
# ^ ^ \-C-
# \-D
#
# The only exception to this is when the user has active enrollments exclusively
# in one sub-account, then the chain is the path from the root (A) all the way
# to that sub-account (D):
#
# /-E
# A-B-
# ^ ^ \-C-
# ^ \-D*
# ^
#
# This is used in the context of deciding which theme the user should be using
# when it comes to non-contextual pages, like the dashboard. See the specs for
# a more visual representation of the expected behavior.
#
class BrandAccountChainResolver
attr_reader :root_account, :user
def initialize(root_account:, user:)
@root_account = root_account
@user = user
end
def resolve
return nil if associated_accounts.blank?
# if there's only one account that has an active enrollment, just use that
actives = enrollment_account_ids & associated_accounts.map(&:id)
if actives.count == 1
return associated_accounts.detect { |x| x.id == actives[0] }
end
longest_chain = [root_account]
loop do
break if enrollment_account_ids.include?(longest_chain.last.id)
next_children = sub_accounts_of(longest_chain.last)
break unless next_children.present? && next_children.count == 1
longest_chain << next_children.first
end
longest_chain.last
end
private
def associated_accounts
@associated_accounts ||= user.associated_accounts.where(
"accounts.id = ? OR accounts.root_account_id = ?",
root_account.id,
root_account.id
)
end
def sub_accounts_of(account)
@sub_account_index ||= associate_accounts_to_parents
@sub_account_index[account.id]
end
def associate_accounts_to_parents
associated_accounts.each_with_object({}) do |account, hash|
hash[account.id] ||= []
if account.parent_account_id.present?
hash[account.parent_account_id] ||= []
hash[account.parent_account_id] << account
end
end
end
def enrollment_account_ids
@enrollment_account_ids ||= root_account
.all_enrollments
.current # don't want concluded enrollments to count, only active ones
.where(user_id: user)
.joins(:course)
.distinct
.pluck(:account_id)
end
end

View File

@ -0,0 +1,410 @@
# frozen_string_literal: true
#
# Copyright (C) 2021 - present Instructure, Inc.
#
# This file is part of Canvas.
#
# Canvas is free software: you can redistribute it and/or modify it under
# the terms of the GNU Affero General Public License as published by the Free
# Software Foundation, version 3 of the License.
#
# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
# details.
#
# You should have received a copy of the GNU Affero General Public License along
# with this program. If not, see <http://www.gnu.org/licenses/>.
#
require_relative "../spec_helper"
describe BrandAccountChainResolver do
let(:user) { user_with_pseudonym }
context "with no associated accounts" do
it "returns an empty chain" do
expect(subject(Account.create!(name: "A"))).to eql(nil)
end
end
#
# A
# ^
#
context "with no active enrollments and no sub-accounts..." do
it "chooses the root account" do
accounts = create_accounts_and_associate(
[
[:A]
]
)
expect(subject(accounts[:A]).try(&:name)).to eql("A")
end
it "ignores accounts the user is not associated with" do
a = Account.create!(name: "A")
b = Account.create!(name: "B")
user.user_account_associations.create!(account: a, depth: 0)
expect(subject(a).try(&:name)).to eql("A")
expect(subject(b).try(&:name)).to eql(nil)
end
end
#
# A-B
# ^
#
context "with no active enrollments and no branches..." do
it "chooses the node farther from the root" do
accounts = create_accounts_and_associate(
[
[:A],
[:B, :A]
]
)
expect(subject(accounts[:A]).try(&:name)).to eql("B")
end
end
#
# /-C
# A-B-
# ^ \-D
#
#
context "with no active enrollments and a branch..." do
it "chooses the branch" do
accounts = create_accounts_and_associate(
[
[:A],
[:B, :A],
[:C, :B],
[:D, :B],
]
)
expect(subject(accounts[:A]).try(&:name)).to eql("B")
end
end
#
# /-D
# /-C-
# A-B- \-E
# ^ \-F
#
context "with no active enrollments and multiple branches..." do
it "chooses the branch closer to the root" do
accounts = create_accounts_and_associate(
[
[:A],
[:B, :A],
[:C, :B],
[:D, :C],
[:E, :C],
[:F, :B]
]
)
expect(subject(accounts[:A]).try(&:name)).to eql("B")
end
end
# /-D
# /-B-
# A- \-E
# ^ \-C
#
context "with no active enrollments and an immediate branch..." do
it "chooses the branch closer to the root" do
accounts = create_accounts_and_associate(
[
[:A],
[:B, :A],
[:D, :B],
[:E, :B],
[:C, :A]
]
)
expect(subject(accounts[:A]).try(&:name)).to eql("A")
end
end
#
# A-B-C*-D
# ^
#
context "with one active enrollment..." do
it "chooses that node" do
accounts = create_accounts_and_associate(
[
[:A],
[:B, :A],
[:C, :B, enroll: true],
[:D, :C],
]
)
expect(subject(accounts[:A]).try(&:name)).to eql("C")
end
end
#
# /-C-E
# A-B-
# \-D*
# ^
#
context "with one active enrollment in a branch..." do
it "chooses that node" do
accounts = create_accounts_and_associate(
[
[:A],
[:B, :A],
[:C, :B],
[:D, :B, enroll: true],
[:E, :C],
]
)
expect(subject(accounts[:A]).try(&:name)).to eql("D")
end
end
#
# /-E /-C
# /-B-
# A- \-D
# \-F-G*
# ^
#
context "with one active enrollment in one of many branches..." do
it "chooses the path to that point" do
accounts = create_accounts_and_associate(
[
[:A],
[:B, :A],
[:C, :B],
[:D, :B],
[:E, :A],
[:F, :A],
[:G, :F, { enroll: true }],
]
)
expect(subject(accounts[:A]).try(&:name)).to eql("G")
end
end
#
# A-B*-C*-D*
# ^
#
context "with multiple active enrollments..." do
it "chooses the enrollment node closer to the root" do
accounts = create_accounts_and_associate(
[
[:A],
[:B, :A, { enroll: true }],
[:C, :B, { enroll: true }],
[:D, :C, { enroll: true }],
]
)
expect(subject(accounts[:A]).try(&:name)).to eql("B")
end
end
#
# A*-B*-C*-D*
# ^
#
context "with multiple active enrollments even at the root..." do
it "chooses the root" do
accounts = create_accounts_and_associate(
[
[:A, nil, { enroll: true }],
[:B, :A, { enroll: true }],
[:C, :B, { enroll: true }],
[:D, :C, { enroll: true }],
]
)
expect(subject(accounts[:A]).try(&:name)).to eql("A")
end
end
# ideally we want it to be D, but it's hard, so we'll stick to "B" since it's
# a branching point:
#
# /-C-E
# A-B-
# ^ \-D*-F*
#
#
context "with multiple active enrollments in a specific branch..." do
it "chooses the enrollment branching point closer to the root" do
accounts = create_accounts_and_associate(
[
[:A],
[:B, :A],
[:C, :B],
[:E, :C],
[:D, :B, { enroll: true }],
[:F, :D, { enroll: true }],
]
)
expect(subject(accounts[:A]).try(&:name)).to eql("B")
end
end
#
# /-C
# A-B*-
# ^ \-D*
#
context "with multiple active enrollments starting at a branching point..." do
it "chooses the branching point" do
accounts = create_accounts_and_associate(
[
[:A],
[:B, :A, { enroll: true }],
[:C, :B],
[:D, :B, { enroll: true }],
]
)
expect(subject(accounts[:A]).try(&:name)).to eql("B")
end
end
#
# /-B-D*
# A-
# ^ \-C-E*
#
context "with multiple active enrollments across different sides of an immediate branch..." do
it "chooses the branching point" do
accounts = create_accounts_and_associate(
[
[:A],
[:B, :A],
[:C, :A],
[:D, :B, { enroll: true }],
[:E, :C, { enroll: true }],
]
)
expect(subject(accounts[:A]).try(&:name)).to eql("A")
end
end
#
# /-C-D*
# A-B-
# ^ \-E-F-G*
#
context "with multiple active enrollments across different sides of a branch..." do
it "chooses the branching point closer to the root" do
accounts = create_accounts_and_associate(
[
[:A],
[:B, :A],
[:C, :B],
[:D, :C, { enroll: true }],
[:E, :B],
[:F, :E],
[:G, :F, { enroll: true }]
]
)
expect(subject(accounts[:A]).try(&:name)).to eql("B")
end
end
#
# /-C-D*
# A-B-
# ^ \-E*-F-G*
#
context "with multiple active enrollments across unevenly-weighted sides of a branch..." do
it "chooses the branching point closer to the root" do
accounts = create_accounts_and_associate(
[
[:A],
[:B, :A],
[:C, :B],
[:D, :C, { enroll: true }],
[:E, :B, { enroll: true }],
[:F, :E],
[:G, :F, { enroll: true }]
]
)
expect(subject(accounts[:A]).try(&:name)).to eql("B")
end
end
# again, ideally it should be D, but it's hard, so we'll stick to the first
# branching point "B":
#
# /-C
# A-B- /-E
# ^ \-D*-
# \-F-G*
#
context "with multiple active enrollments within a single branch..." do
it "chooses the branching point closer to the root that is also an enrollment node" do
accounts = create_accounts_and_associate(
[
[:A],
[:B, :A],
[:C, :B],
[:D, :B, { enroll: true }],
[:E, :D],
[:F, :D],
[:G, :F, { enroll: true }]
]
)
expect(subject(accounts[:A]).try(&:name)).to eql("B")
end
end
def subject(account)
described_class.new(
user: user,
root_account: account
).resolve
end
def create_accounts_and_associate(accounts_and_parents)
accounts_and_parents.each_with_object({}) do |(name, parent, *opts), acc|
opts = opts.first || {}
acc[name] = Account.create!(
name: name,
parent_account: parent ? acc.fetch(parent) : nil
)
user.user_account_associations.create!(
account: acc[name],
depth: acc[name].account_chain.length - 1
)
next unless opts[:enroll] == true
course_with_student(
user: user,
account: acc[name],
active_all: true
)
end
end
end

View File

@ -2397,75 +2397,6 @@ describe User do
expect(user.profile).to eq profile
end
describe "common_account_chain" do
before :once do
user_with_pseudonym
end
let_once(:root_acct1) { Account.create! }
let_once(:root_acct2) { Account.create! }
it "work for just root accounts" do
@user.user_account_associations.create!(account_id: root_acct2.id)
@user.reload
expect(@user.common_account_chain(root_acct1)).to eq []
expect(@user.common_account_chain(root_acct2)).to eql [root_acct2]
end
it "works for one level of sub accounts" do
root_acct = root_acct1
sub_acct1 = Account.create!(parent_account: root_acct)
sub_acct2 = Account.create!(parent_account: root_acct)
@user.user_account_associations.create!(account_id: root_acct.id)
expect(@user.reload.common_account_chain(root_acct)).to eql [root_acct]
@user.user_account_associations.create!(account_id: sub_acct1.id)
expect(@user.reload.common_account_chain(root_acct)).to eql [root_acct, sub_acct1]
@user.user_account_associations.create!(account_id: sub_acct2.id)
expect(@user.reload.common_account_chain(root_acct)).to eql [root_acct]
end
context "two levels of sub accounts" do
let_once(:root_acct) { root_acct1 }
let_once(:sub_acct1) { Account.create!(parent_account: root_acct) }
let_once(:sub_sub_acct1) { Account.create!(parent_account: sub_acct1) }
let_once(:sub_sub_acct2) { Account.create!(parent_account: sub_acct1) }
let_once(:sub_acct2) { Account.create!(parent_account: root_acct) }
it "finds the correct branch point" do
@user.user_account_associations.create!(account_id: root_acct.id)
expect(@user.reload.common_account_chain(root_acct)).to eql [root_acct]
@user.user_account_associations.create!(account_id: sub_acct1.id)
expect(@user.reload.common_account_chain(root_acct)).to eql [root_acct, sub_acct1]
@user.user_account_associations.create!(account_id: sub_sub_acct1.id)
expect(@user.reload.common_account_chain(root_acct)).to eql [root_acct, sub_acct1, sub_sub_acct1]
@user.user_account_associations.create!(account_id: sub_sub_acct2.id)
expect(@user.reload.common_account_chain(root_acct)).to eql [root_acct, sub_acct1]
@user.user_account_associations.create!(account_id: sub_acct2.id)
expect(@user.reload.common_account_chain(root_acct)).to eql [root_acct]
end
it "breaks early if a user has an enrollment partway down the chain" do
course_with_student(user: @user, account: sub_acct1, active_all: true)
@user.user_account_associations.create!(account_id: sub_sub_acct1.id)
@user.reload
full_chain = [root_acct, sub_acct1, sub_sub_acct1]
overlap = @user.user_account_associations.map(&:account_id) & full_chain.map(&:id)
expect(overlap.sort).to eql full_chain.map(&:id)
expect(@user.common_account_chain(root_acct)).to(
eql([root_acct, sub_acct1])
)
end
end
end
describe "mfa_settings" do
let_once(:user) { User.create! }