►
Description
Responding to MR feedback: "User is an omniscient class. Could this logic be encapsulated somewhere else?"
Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/421265
Branch with work in progress changes: https://gitlab.com/gitlab-org/gitlab/-/commit/082e330bc6aaf03eda79300
We also discuss Numbered block Parameters in Ruby https://medium.com/@baweaver/ruby-2-7-numbered-parameters-3f5c06a55fe4
A
B
A
A
This
was
some
Mr
feedback
that
I
got
and
then
the
other.
Maybe
we
can
do
both
we
can
do
reproducing
CI
builds
locally
with
GDK
and
Docker
I'll
do
two
topics,
so
let
me
share
my
screen
and
I
just
found
the
branch
where
I
worked
on
this
the
other
week.
I
actually
started
working
on
this
by
myself
and
I
thought
to
myself
as
I
was
working
on.
C
A
D
A
Also,
if
you
notice
the
customer
Mission
before,
but
it's
also
calling
these
pre-loaded
member
Rose
classes
and
I'm
actually
as
I'm
talking
about
this
I'm
kind
of
remembering
where
I
got
stuck.
So
that's
good,
because
that
before
I
was
like,
why
are
they
stop
working
on
this,
but
now
I'm
remembering
so
we
have
this
method
that
preloads
these
relationships.
A
But
then
this
method
is
always
called
for
one
resource
because
custom
permission
for
is
called
in
policy
classes.
So
basically
say:
you're
listed
looking
at
a
group
of
projects
and
we
want
to
know
if
you
have
the
ability
to
read
all
of
those
projects.
We
call
the
preloader
to
make
sure
that
stuff's
all
cached.
B
D
A
Only
called
in
a
few
places
actually,
but
so
here
we
have
this
nice
little
module
where
we're
doing
a
lot
of
different
pre-loading,
and
this
I
was
trying
to
call
it
in
here.
This
is
actually
I
like
stopped
midward.
For
some
reason,
I
must
have
been
in
a
big
rush,
couldn't
couldn't
finish
whatever
I
was
gonna,
say
here:
resource
ability
or
resources
ability,
so
resources
to
your
projects.
A
Okay,
oh
and
here
yes,
this
is
the
rub.
So
in
this
context
we
might
be
checking
for
a
lot
of
different
abilities
in
the
project.
So,
let's
look
at
where
this
method
is
called
prepare
projects
for
rendering
it's
called
in
various
controllers
that
load
lists
of
projects
where
we
then
make
various
calls
on
them.
So
this
is
a
pre-loader
for
custom
permissions,
there's
also
one
for
Max
member
access,
which
pulls
the
max
access
level.
It's
like
a
list
of
integers
for
the
list
of
projects.
A
C
B
A
Yes
and
I
remember
when
this
logic
was
first
added.
There
were
discussions
about
like
where,
where
else
could
we
do
this
right
because
you
it
needs
to
be
in
a
context
where
you
know
the
preloading
needs
to
happen
so,
like
you,
don't
want
to
call
the
preloader,
for
example,
you're
like,
should
we
call
it
like
a
considered
initializers
or
something
like?
Is
it
like
every
time
a
request
comes
through?
Do
we
pre-load
this?
We
don't
really
need
it
for
every
request,
but
we
need
it
for
many
requests.
A
B
B
A
So
Sam
was
saying
that
calling
preload
here
we're
trying
to
refactor
this
into
like
this
finder
or
into
something
that's
not
on
user,
so
that
we're
not
just
adding
one
more
method
to
the
user
class,
which
has
you
know
thousands
of
methods,
probably
Sam,
was
kind
of
questioning
this
approach
in
general
of.
Why
are
we
calling
a
preloader
in
this
method
that
is
invoked
in
various
controllers
as
a
way
of
caching
and
I
agree,
it's
not
ideal.
C
A
A
A
Yeah,
like
there's,
there's
the
custom
abilities
like
preloader,
which
we
already
have
that
class.
So
this
is
where
I
started,
getting
going
down
a
refactor
a
rabbit
or
like
a
spiral
like
why
I'm
wasting
my
time
spiral
because
it's
like
well,
this
already
encapsulated
in
a
class.
All
that's
happening
here
is
that
we're
wrapping
it
with
safer
cross
loader,
so
does
that?
Does
that
require
its
own
class
or
like?
What
should
the
interface
of
this
be?
Assuming
that
this
method,
in
some
form
needs
to
be
publicly
available
to
be
called
over
here?.
C
B
A
B
C
A
B
A
C
A
D
A
A
A
A
B
A
A
D
C
A
A
A
A
D
D
C
A
A
C
A
D
B
C
Would
always
remember
yeah
for
me:
I
always
use
it,
because
you
get
a
meaningful
error
at
the
point
where
the
error
occurs
rather
than
somewhere
up
the
stack.
The
the
code
saying
nil
does
not
respond
to
LA,
which
is
normally
what
happens
when
you're
left
scratching
your
head
about
where
the
actual
problem
was.
D
C
B
A
Yeah
I,
don't
think
we
do
I,
don't
think.
We
would
expect
that,
based
on
my
memory
of
writing
this
code
a
while
ago,
see
when
there's
it
returns
an
empty
value
right.
So
when
there's
the
membership
has
no
custom
role,
project
ID
would
still
be
there.
It
just
points
to
a
null
value,
which
is
fine,
because
we're
just
calling
fetch
on
we'd
still
get
an
empty
array.
B
A
A
It's
yeah
there's
also
discussion
to
be
had
about
whether
we
could
consolidate,
but
for
now,
let's
just
worry
about
project.
So
this
this
is
cool.
We
have
safe
a
request,
loader
being
called
in
here,
which
is
great
I,
don't
have
any
unit
tests
yet
for
custom
abilities
finder,
but
let
me
just
run:
do
you
mind
if
I
I'm
just
going
to
run
this
pre-loader
unit
test,
really
quick
to
make
sure
we
didn't
like
break
the
interface
of
the
class?
It
should
just
be
a
street
view.
Factor
at
this
point.
B
C
A
C
A
C
A
A
A
This
that's
kind
of
a
mouthful
all
right,
so
many
lines
so
we're
calling
preloader
and
then
we
don't
need
to
pull
out
the
resource
ID
or
we
don't
need
to
call
fetch
even
right,
because
we're
just
trying
to
do
a
preload
here,
so
we're
taking
the
floor
collection
of
projects.
I
think
this
is
current
users.
A
A
C
A
I
feel
like
we
had.
We
have
some
tests
somewhere
I
think
they
were
in
the
method.
So
there
were
this.
This
safe
request,
loader
business,
was
previously
in
this
method.
I
think
the
unit
tests
for
this
method
I
did
delete
those.
Yet
no,
let's
look
for
those,
because
I
think
that
they
do
test
that
the
safe
request,
loader
is
being
called
on
line.
Two
seven,
seven,
two.
A
A
A
B
B
Cool,
it's
just
good
to
remember
too
quit
the
process
before
you
switch
branches
otherwise
like
when
you
pull
master
or
something
there
will
be
so
many
spec
changes.
So
it'll
keep
your
laptop
running
the
whole
day.
Okay,
yeah!
That's
right!
Because
it'll
like
most
of
the
tests,
we
don't
run
again,
but.
A
A
D
A
A
B
A
D
D
A
D
D
D
B
A
A
A
D
A
D
C
A
A
C
B
A
D
D
A
A
B
A
C
A
A
A
A
C
B
A
B
A
A
B
A
D
A
A
D
A
D
A
C
A
C
B
A
A
A
A
C
A
A
A
B
A
D
D
D
A
D
B
D
B
B
B
B
B
A
D
B
C
A
D
A
Yeah,
maybe
this
means
sorry
we're
at
the
end
of
our
time,
so
I
should
make
sure
you
wrap
up
on
time,
but
I
think
what
I'm
going
to
do
moving
forward.
This
is
super
helpful,
is
either
just
update
the
specs
here
or
I.
Guess
I
have
to
think
about
what
I
needed
to
do,
because
I
can't
remember
the
significance
of
why
we
need
a
license
check
to
make
sure
that
data
isn't
returned,
but
that
might
be
important
important
to
check
for
so
I.