►
From YouTube: Code Review Weekly Workshop - July 7, 2023
Description
In this session we talk about some random code review topics.
00:00 - Using the Web IDE to diff email files
07:20 - General tips on reviewing email MR's
14:35 - Discussing approach with rename emailsDisabled to emailsEnabled
A
A
Let
me
share
my
screen
and
we
could
talk
about
that
thing.
So,
every
once
in
a
while,
I
have
to
review
Mrs
touching
emails
and
I
was
gonna.
I
had
a
thought
that
oh
I
might
be
able
to
use
the
web
ID
to
do
this.
One
little
thing
that
you
have
to
make
sure
with
new
emails
and
you
totally
can't
which
is
really
exciting.
So
one
of
the
quirks
with
the
way
we
do
our
emails
is.
A
We
have
camel
email,
templates
and
text,
Erb,
email
templates
and
ideally
these
two
templates
would
say
the
same
thing,
but
in
a
different
format,
and
so
when
someone's
writing
a
new
email,
you
want
to
make
sure
that
these
two
files
are
looking
the
same
to
the
user
and
if
it's
a
long
email,
it's
kind
of
hard
to
like
just
even
visually.
If
you
had
two
renderings
of
the
same
email
side
by
side,
but
let's
just
pick
an
email,
real
fast
I'll
pick
a
one
of
the
text
ones,
let's
do
like.
A
A
A
B
A
A
I
could
do
is
here
we're
saying
hi
username,
but
I
can
see
clearly
just
comparing
these
two
files.
Oh
yeah,
these
aren't
the
same.
This
is
a
much
Starker
difference
than
what
I
was
hoping
to
find
one,
let's
see
if
so
like.
If
I
was
going
to
look
at
these
two
I
see
a
lot.
Some
of
these
are
the
same.
You
see
like
here
this
bit
it
it
determined.
A
B
A
Yeah,
you
can
see
some
slight
differences.
One
is
like
when
we
insert
links
in
the
text.
We
just
do
the
link
directly.
We
don't
we
can't,
because
we're
not
doing
anchor
tags.
We
don't
get
to
wrap
it
in
something
like
change
your
password,
but
basically
what
I
do
and
I
want
to
tell
if
two
things
are
kind
of
the
same
is
I'll
start
diffing
them,
but
then
I'll
just
start
changing
one
of
them
to
look
like
the
other
one
and
see
if
the
changes
make
sense.
So.
B
A
B
A
There's
a
lot
more
information
that
we're,
including,
and
so
if
I
was
reviewing
this.
If
this
was
a
new
email,
I
would
say:
hey
how
come
we're
not
including
hostname,
and
all
this
other
information
in
the
text.
Version
of
the
email
and
I
think
that
would
be
something
that
we'd
want
to
do
more
often
than
not.
A
The
differences
will
look
a
little
more
similar
like
this,
and
so,
if
I
was
going
to
compare
active
file
with
this
one
yeah,
so
I'll
try
I'll
just
start
removing
things
to
see
if
they
kind
of
look
the
same.
So
I
was
like
hello
name,
waiting
to
see
if
these
things
are
kind
of
the
same
yeah
that
one's
the
same
and
then
start.
A
A
That
look
like
they're
slightly
worded
differently,
or
something
like
that
so
yeah
I
was
just
gonna
share.
I
was
I
was
pleasantly
surprised
that
the
web
ID
out
of
the
box,
because
we're
using
open
source
vs
code
has
these
cool
features
like
compare
active
file
with.
So
you
can
actually
compare
the
files
with
each
other
pretty
nicely
in
the
web
ID,
which
could
be
nice
when
two
files
kind
of
have
the
local
link.
So
that's.
A
A
A
E
Don't
even
know
how
to
test
one,
you
know
what
I
mean
like
yeah
I
would
want
to
test
it
I
feel
like
if
I'm
reviewing
it
I
would
want
to
see
it
visually
for
the
for
the
non-text
one,
because
I
like
the
text,
one
like
you
can
probably
just
test
that
pretty
easily,
or
at
least
like
visualize,
what
it's
going
to
look
like,
but
the
from
what
I
remember:
email,
CSS,
Is,
Not,
Great,
like
to
getting
an
email.
Looking
right
is
not
like
a
great
experience.
E
B
E
B
E
B
E
C
I
was
also
mentioned
that
testing
it
in
our
application
is
actually
pretty
straightforward.
I,
don't
remember
it
from
the
top
of
my
head,
but
there's
like
a
route
in
our
in
a
ruby,
app
or
web
app.
That
allows
you
to
render
all
the
emails
that
we
do
have
available
like
in
in
GDK,
like
in
a
testing
purpose.
So
that's
the
good
thing,
the
bad
thing
about
it,
as
you
just
mentioned,
it
will
render
in
your
browser.
C
A
Yeah,
there's
there's
two
things
you
can
use
to
test
the
emails
and
it
rendering
on
the
browser,
isn't
isn't
that
bad?
But
it's
yeah,
there's
quirks
that
the
browser
clearly
can
handle
which
email
clients
can
I
say
it's
not
that
bad,
because
we
have
like
a
preprocessor
that
gets
rid
of
all
CSS
classes
and
stuff
and,
like
inlines
the
styles
of
the
things
we
have
a
preprocessor
to
emails
that
get
sent
out
to
make
them
email,
client
friendly,
but
that
doesn't
necessarily
mean
that
it's
always
email,
client
friendly.
A
The
two
things
ways
of
testing
them
is:
yes,
there's
a
there's
like
the
mail
previewer,
and
so
hopefully,
when
a
new
email
is
written.
We
can
add
this
to
our
big
list
of
email
previews,
but
that
doesn't
always
happen.
The
other
thing
you
can
use
to
test
emails
is
there's
a
route
called
letter
opener
and
if
you
open
this
route,
any
email
that
the
application
is
actually
sending
will
go
into
this
letter,
opener
thing-
and
so
it's
like
you-
have
this
on
a
tab
and
I'm
doing
something
else.
On.
B
A
C
Bonus
question
on
this.
We
do
have
a
browser
metrics
of
browsers
that
we
support.
Like
everybody,
who's
saying
on
the
web,
does
have
do
we
actually
have
a
list
of
email
clients
that
we
support,
so
yeah
haven't
seen
any
any
of
these
things
as
well,
but
I'd
be
I'd,
be
sure
that
you
would,
if
you
would
come
along
with
like
a
a
five-year-old
email
client,
you
probably
wouldn't
have
the
greatest
experience.
A
Yeah
we
it
feels
like
not
too
long
ago,
but
I
guess
maybe
it's
like
a
year
plus
we
updated
that
preprocessor
of
the
emails
significantly
and
emails
became
a
lot
more
styled
when
our
gitlab
Styles
I,
don't
know
if
you
ever
notice.
This
change
happened,
but
there
was
a
number
of
issues
caused
that
happened
in
The,
Fringe,
email,
clients
and
I.
A
Think
the
goal
was
just
to
kind
of
keep
supporting
whatever
we
supported
before,
but
there
were
some
issues
that
were
very
challenging
from
because
the
user
configuration
this
is
difficult
to
anticipate.
I,
say
that
because
maybe
it's
not
a
bad
idea
to
just
be
explicit
with
like
hey
if
you're
receiving
emails.
These
are
the
clients
that
our
emails
render
the
best
with
and
make
sure
you
don't
have
these
things
like.
A
D
A
B
A
B
E
That
was
huge
and
I
have
the
links.
Let
me
put
it
in
here
and
it
was
changing
over
from
using
this
a
field
called
emails,
disabled
to
emails,
enabled
in
both
this
group
and
project
context.
It
was
a
huge
Mr
with
a
ton
of
back-end
changes.
I
think
some
database
changes
some
front-end
changes
and
I
think
Paul.
The
question
that
I
wanted
to
Circle
back
on.
E
You
asked
me:
why
were
they
going
through
the
process
of
changing
everything
from
the
front-end
wording
of
emails
enabled
disabled
to
the
database
when
it
might
be
better
to
maybe
just
change
what
the
front
end
says
and
just
kind
of
like
not
everything
on
the
back
or
something
like
that?
I
did
go
through
and
do
the
due
diligence
of
looking
into
that
when
I
reviewed
the
Mr-
and
there
was
a
really
long
thread.
E
Discussion
between
I
think
git
love
team
members
at
some
point
where
that
was
discussed
and
the
decision
was
made
to
like
do
this
version
of
the
change.
So
I
did
go,
look
it
up
and
yeah
I
guess
I
wanted
to
say!
Thank
you
for
asking
that
question
because
I
like
did
it
didn't
even
like
it
really
even
occur
to
me
just
to
make
the
change
on
the
front
end,
but
I
did
go.
E
Look
it
up
and
if
I'd
like
a
pretty
long
for
our
discussion
with
quite
a
few
folks
about
like
why
they
wanted
to
get
everything
done
from
the
front
to
the
back
and
I
think
it
was
like
it
could
cause
a
lot
of
confusion.
Going
forward
and
they
wanted
everything
to
kind
of
be
the
same.
I
felt
like
the
Mr
was
like
very
confusing
to
review
and
every
time
I
reviewed
it
I
feel
bad
I
kept
like
finding
new
things
or
I'm
like
I.
A
A
B
E
E
So
things
are
a
little
off
right
now,
but
so
just
to
give
the
high
level
I
can
show,
maybe
the
picture
of
before
and
after
and
that
might
be
of
like
a
clear
Yep.
This
is
the
biggest
part
of
the
change
is
they're
changing
from
this
disabled
email
notification
to
it
being
like
enabled
the
wording
enable
and
also
it
like,
floats
all
the
way
down
to
the
database.
E
E
B
E
A
E
The
whole
thing
is
very
confusing,
like
the
code,
the
way
it
was
I
felt
like
it
was
very
odd
because
I
feel
like
this
is
part
of
our
interview
for
backend
there's
like
questions
that
like
they're
like
this
is
an
odd
way
to
do
this.
We
shouldn't
do
this
and
then
we
have
it
in
our
code
base.
So
I'm,
like
okay,
I,
don't
know
if
you
want
to
make
this
recording
private
now,
because
I
said.
E
Don't
know
there's
something
about
like
not
overriding
database
columns.
It's
just
a
strange
thing
to
do.
If
you're,
like
have
a
database
column
that
you
run
this
like
super
or
like
do
something
else,
because
it's
kind
of
like,
if
you
disable
the
old
way,
was
if
you
disabled
in
the
name
space
it
overrides
the
project
setting.
E
B
E
E
Yes,
we
also
ran
into
like
they
removed
stuff
from
the
API
and
I
was
like
oh
I,
don't
I,
don't
think
you
can
do
that.
You're,
just
gonna
have
to
leave
it
and
Mark
it
as
deprecated
yeah.
You
can
add
the
new
one,
but
you
can't
remove
the
old
one.
I,
don't
think
you
can
ever
remove
fields
from
the
API
am
I
wrong.
No.
B
E
So
and
then
you
know
that
was
they
were
like,
but
this
is
what
this
Mr
is
doing
and
I'm
like.
Well,
yes,
it
is
in
most
of
the
areas,
but
for
the
API,
especially
you
have
to
leave
the
old
one
and
then
you
can
add
the
new
one
and
then
hopefully
people
will
just
use
that,
but
for
the
API
it's
like
I,
don't
even
think
from
the
documentation.
It
was
pretty
clear.
You
cannot
introduce,
like
it's
an
exception,
to
get
a
removal
of
a
field
from
the
API
response.
A
E
B
A
A
There's
two
ways:
you
can
approach
changing
the
thing
you
can
change
it
at
its
roots.
You
know
which
would
be
the
database
field
and
then
propagate
from
there
or
we
can
just
re-polish
it
to
make
it
look
like
the
ux
just
solve
that
not
saying
that
we're
ignoring
the
root
we
can
keep
going
like.
We
can
go
from
the
front
to
the
back,
but
because.
A
E
E
Challenging
and
I
think
that
isn't
the
path
that
was
was
taken.
I
actually
worked
with
this
contributor
for
the
database,
migrations
that
backfilled
things
it
took
months,
yeah
for
the
backfills
to
complete
and
I,
didn't
understand.
Why?
Because
I
didn't
really
pay
attention
to
the
names
of
migrations.
I
was
just
like:
oh
okay,
this
thing's
taking
a
while
I
guess
it's
big
and
what
it
was
doing
was
backfilling
emails,
enabled
for
like
all
projects
and
all
these
spaces
yeah.
E
B
B
A
B
E
Oh
I
still
don't
feel
100
about
this
Mr
like
that's.
Why,
as
I,
even
though
I
am
a
maintainer
I
only
did
the
initial
review
and
I
really
want
someone
else
to
look
at
it
as
another
set
of
eyes.
Maintainer
or
not.
There
are
still
sometimes
when
the
Mrs
are
like
I
want
someone
else
to
look
at
this
yeah.
B
B
B
A
The
so
these
are
the
overall
context.
This
is
a
rails
form
that
thing
is
Ajax
submitted.
Do
we
even
say
Ajax
anymore,
I,
don't
know
this
is
a
rails
form
where
you
have
all
the
toggles,
but
then
at
the
bottom
you
have
these
check.
Boxes
of,
for
your
projects
is
in
the
project
settings
so
check
boxes
are
interesting,
I
kind
of
learned
something
because
let
me
go
up
to
the
original
comment
here.
A
B
B
A
B
E
A
A
A
A
B
A
A
A
Here
is
a
hidden
input
for
this
thing.
Here's
a
hidden
input
for
this
thing,
so
like
it's
pretty
straightforward,
but
the
one
thing
I
I
Rec
said
is
like
okay.
If
we
have
an
input
with
the
name,
this
checkbox
doesn't
need
a
name.
I
see
that
it
looks
like
we're
doing
that
below
here.
But
it's
like
the
checkbox
is
just
the
presentation
for
the
hidden
input
and.
D
A
We're
not
gonna
we're
not
going
to
continue
to
add
till
the
pattern
that
doesn't
make
sense,
and
maybe
that
makes
things
a
little
I
don't
know.
Maybe
that
makes
things
strange
for
future
readers
of.
Why
does
this
have
not
have
a
name?
Why
does
this
have
a
name?
Hopefully
it
just
makes
it
obvious
that
the
name
is
and
it
shouldn't.
E
A
C
What
we're
gonna
say
on
it
and
before
you
move
on
to
the
test,
just
a
quick
question
of
understanding
on
this.
Apologies,
if
you,
if
you've
seen
this
before,
but
we
I
get
the
the
hidden
input,
part
and
I
get
what
you
said
regarding
the
Ajax
endpoint
and
all
of
these.
But
what
we're
seeing
here
is
like.
Are
we
firing
a
form
submit
from
a
new
component?
C
Is
that
what
we're
doing,
because,
like
just
from
my
gut
feeling,
that
would
be
the
perfectly
right
thing
to
do
from
a
Hamel
template
as
I
find
like
old,
proper
Dom
land,
but
here
in
redomland
that
seems
odd
and
awesome.
What
are
we
doing
when
this
is
done,
like
page
reload
or
what
is
happening
here?
That's.
A
A
B
C
C
C
B
A
Debt,
but
a
lot
of
Legacy
code
with
the
way
the
controller
receives
this
input.
It's
like
it's
not
ready
for
an
API
Ajax
submission,
but
it's
expected.
This
is
a
form
submission.
The
controller
has
all
these
rules
to
it.
So
we
have
to
do
that
form,
submission
thing
and
it's
part
of
it
is
leaning
into
the
pre-existing
Behavior.
So
for
my
for
my
purpose
was
it's
like
there's
already
a
rails
form,
but
now
we
need
something
really
Advanced.
A
B
C
B
A
Something
to
think
about
I
think
in
the
brought
from
the
from
the
broader
front-end
perspective
is
like
Do
Vanilla
form
submissions
have
a
place
like
do
we
like
them,
and
maybe
we
don't-
and
maybe
we
should
you
know,
maybe
we
should
view
that
as
technical
debt.
This
definitely
becomes
more
challenging
like
when
errors
happen,
like
all
of
that,
that
whole
cycle
of
I
submitted
something
we're
going
to
reload
the
whole
page,
but
there
were
errors,
so
they
showed
that
the
you
know:
that's
not
fun.
A
A
Sorry,
Yannick
I
know
the
only
other
thing
I
was
going
to
say
about
this
is
so
this
is.
This
is
really
really
small.
One
thing
that's
interesting
is
like
here
we
have
these
JS
classes
and
but
that's
pre-existing,
but
it's
being
used
in
these
feature.
Specs
and
the
feature
specs
is
what
I
observed
to
tell
that?
Oh,
what
was
I
thinking.
A
A
B
A
E
B
E
It
may
be
not
a
bad
idea
to
suggested
as
a
non-blocking
change
or
maybe
I
mean
I,
don't
know,
I
feel
like
the
specs
are
hard
enough
to
read,
as
it
is.
Having
things
like
say,
the
right
things
for
me
is
helpful
to
make
sure
that
I
understand
what
the
spec
is
doing
and
so
like,
while
I
want
to
almost
make
it
a
non-blocking,
because
there
has
been
a
lot
of
back
and
forth
and
like
maybe
just
fix
it.
A
Yeah,
that's
yeah,
that's
interesting,
I,
don't
know
now
what
I
was
gonna
say
is
like
so
whenever
we
change
a
unit
but
there's
no
corresponding
change
to
the
unit
test
and
like
this
is
clearly
kind
of
more
than
a
pure
refactor
we're
changing
the
name
of
something
and
even
the
text
that
we're
rendering.
So
it's
like
I
kind
of
expected
to
see
a
unit
test
change,
but
I
see
I
think
when
I
dived
into
the
unit
test.
It's
not
going
to
this
level.
A
So
I
was
a
little
concerned,
but
then
I
could
see
that
okay,
but
we
have
like
multiple
feature
specs
covering
this,
and
so,
if
there
wasn't
already
a
lot
of
fatigue
on
the
Mr
I
would
ask
like
hey.
Can
we
just
add,
you
know
a
test
missing
unit
test
coverage
for
this
thing,
but
technically
we
already
the
thing:
that's
there's
not
a
lot
of
conditional
logic
going
on
either
here.
So
the
most
important
thing
that
goes
on
here
is:
does
it
all
work
in
integration?
A
B
A
B
A
E
I
would
think
that
that
it
might
be
a
nice
thing
to
open
to
leave
the
comment,
make
it
non-blocking
open
up
a
follow-up
ad
like
the
help
needed
or
whatever,
like
the
community
contribution
tag
on.
It
is
because,
if
it's
small
enough
to
like
add
a
feature
spec,
it
is
going
to
be
more
work
on
you
as
the
reviewer
to
like
write
that
up
in
a
way
that
someone
could
pick
it
up.
B
E
A
A
C
C
A
C
C
But
to
yeah
that
might
be,
it
might
be
a
valuable
solution.
But
to
answer
your
question,
I
I'm,
pretty
convinced
that
we
should
come
up
with
a
test
to
come
with
us,
even
though
the
logic
is
kind
of
trivial.
At
this
point,
I
feel
like
this
is
something
if
it
breaks,
we
would
wanna
be
aware
of
and
yeah
yes,
a
feature
test
is
great,
but
feature
test,
also
changes
and
that's
that's
kind
of
okay
right.
Therefore,.
D
B
D
A
Foreign
I
think
that's
a
great
question.
I
I,
like
I
like
ionic's,
point
that
hey,
we
should
I
think
I'm
not
inclined
to
block
the
current
Mr
but
saying
hey.
Let's
add,
let's
add
more
pattern,
breaking
changes
to
the
test,
because
the
tests
aren't
currently
like
supporting
this
kind
of
thing.
So,
especially
given
the
larger
focus
and
fatigue
on
the
Mr
and
the
fact,
it's
already
has
feature
spec
coverage
if
it
didn't
have
features
by
coverage
like
we
got
a
test.
A
So
to
me
it's
like
it's
a
follow-up.
The
task
of
pushing
tests
down
the
pyramid
is
the
pre-existing
test
down.
The
pyramid
just
feels
like
a
follow-up
for
me,
but
I
love
your
idea
of
like
yeah.
Let's
have
a
component
for
this
and
that
will
make
the
test
really
easy.
We
just
have
to
see
hey.
Did
we
render
this
form
check
with
hidden
input
component
with
this
name
like
that
sounds
really
great.
A
C
Plus
I
agree
with
everything
you
just
said,
plus,
maybe
leave
a
comment
on
the
features
back
and
Link
the
issue
like
hey,
it's
absolutely
okay,
to
change
this
file,
but
this
is
not
covered
in
a
unit
based
level.
So,
when
you're
thinking
about
about
changing
this,
please
make
sure
that
this
issue,
Linked
In
Here,
is
hopefully
resolved.
By
now.
B
A
What
you're
saying
yeah
that's
interesting
like
leaving
a
comment,
an
actual
inline
code
comment.
That's
the
interesting
point:
yeah
yeah!
This
is
an
interesting.
This
is
the
interesting
layer
of
the
code
when
it
comes
to
testing
because,
like
the
most
important
thing
here
well,
obviously
it's
all
super
important
but
like
if
the
name
was
different
and.
D
A
It
doesn't
work
like
the
only
one
of
the
things
that
really
really
matters
here
is
that
this
name
of
the
input
matches
what
the
rails
controller
expects.
So
it's
hard
to
test
that
without
it
somehow
being
in
integration.
So
it's
like
that
feature.
Spec
coverage
is
so
important
for
this
kind
of
thing.
A
D
A
B
A
A
A
E
E
Think
maybe
what
would
have
been
good
and
I
didn't
read
through
the
entire
thread,
all
the
way
I
probably
did
I,
don't
remember,
because
I
just
feel
like
the
last
few
weeks
have
been
like
really
just
there's
a
lot
going
on.
It
might
have
been
to
lay
out
a
different
implementation
plan
about
how
to
approach
this.
Knowing
that
someone
from
the
community
is
going
to
be
working
on
it
to
say
like.
E
A
A
Yeah
we're
doing
the
same
thing
for
group
settings.
Yes,
let's:
let's
try
to
do
it
in
a
different
way
and
I
think
that's
the
point
is
we
can
end
up
in
the
same
place?
So
it's
not
saying
the
point.
Isn't
the
the
desired
end?
State!
The
point
is
how
we
get
there
and
I.
Think
if
you
do
it
from
the
front
end
heading
back
it's
safer
than
from
the
root
heading
forward.
We
have
to
change
everything.
A
A
A
Yeah
I
mean
this
is
I.
Think
another
interesting
case
too,
where
I
think
one
thing
that's
relevant
here.
Is
you
see
an
MR
and
you
see
the
approach
and
you're
like
oh
wow
like
this?
Is
this
is
a
risky
approach
to
making
all
these
changes
I
wish
we
weren't
doing
it
this
approach,
but
we've
already
made
the
database
migration.
So
it's
like
we're.
B
A
Number
of
times
I've
reviewed,
NMR,
it's
there's
every
I
would
say.
Maybe
a
quarter
of
the
time,
I'll
see
an
approach
that
isn't
favorable
and
I'll
be
able
to
say
something,
I'm,
saying:
hey:
let's
not
go
down
this
path,
a
lot
of
times,
I
come
to
an
MR,
and
oh
we've
already
been
on
this
path
for
a
couple
of
months.
It's
like
all
right,
but
we
have
to
just
make
the
best
of
the
path
we're
on
and
then
hopefully
the
the
thing
to
then
decide
then
is
well.
A
E
Like
this
yeah,
based
on
the
the
discussion
thread,
it
seems
like
we
got
here
from
a
Lessons,
Learned
From.
A
previous
issue
is
what
I
gathered
from
that,
but
perhaps
that
step
further
would
have
been
to
like
say:
okay,
this
is
the
end
State
we
want.
How
can
we
get
this?
How
can
we
get
there
in
smaller
chunks
to
avoid
monster
risky
change?
Okay,
yeah.
A
Definitely
I'll
try
to
do
that.
None
of
this
is
like
the
contributor.
That's
like
Shepard
Shepard.
Did
this
like
it's
crossed.
It's
been,
you
know
super
great
and
thoughtful,
and
it's
has
been
a
small
change,
but
it's
because
of
the
way
that
we
decided
to
do
it
see
you.
James
has
been
really
involved
and
so
I
think,
like
the
contributor
has
been
fantastic.
It's
just
I
wish
we
had
a
better
iteration
plan
from
the
beginning,
but
all
right.