GitLab Code Review Weekly Workshop, 16 Jun 2023

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: Code Review Weekly Workshop - Jun 16, 2023

Description

In this session we talk about some code reviews!

00:00 - Reviewing a change to high traffic modules with technical debt
08:55 - Reviewing an MR that transforms with `render_ce` vs `render`
30:12 - Pair reviewing on a HAML Pajamas Component refactor

A

Thanks everybody for hopping on the code review, weekly Workshop uh I've got just one small topic um to bring up, but if you all had any other things, you wanted to show and tell other questions about code review. If you want to add it to the agenda and then we can parent a code review afterwards.

A

um So yeah I'll go ahead and kick this off. I'll share my screen, so we're looking at the same thing.

A

So I had um I got painting to review this Mr, which is great um but made me nervous immediately, because I saw oh, no, we're touching notes uh and uh and deprecated notes uh as well and notes are just like notoriously um flaky and there's lots, there's duplication and there's some technical debt here.

A

So I was a little nervous about some of the changes and just to highlight what what some of the changes were was I. Guess we had this non-reactive property that we were setting inside of a method before submitting things and we're removing it now into the data of the components and setting it as soon as or here's the update. ah Why is it keep going down uh yeah we're moving into the data of the component and then we're also adding it we're kind of changing this line so I'm?

A

My ears are really perked up wanting to be extra sensitive to what are hidden side effects to this thing, and so this is the main thing I wanted to communicate is when I come across some bit of code. That's super high traffic, because this is notes and but is also he's crazy, large, with lots of technical debt all around and everything I've I've gotta ask myself what is the worst thing that could happen and try to mitigate that as much as possible.

A

So like the worst thing that could happen as I look at well, hopefully, I can still submit notes and create threads on Mrs issues and all the different types of issueable types. I, don't see anything with these changes that would be different based on different issueable types.

A

So, when I'm testing things I, don't feel like I need to check out incidents and I need to check out uh commit issues, although commits and Snippets are handled in the deprecated notes, but um but one thing that I did know like okay, if we're changing the way we initialize this thing at different times, but it does have like a side effect of rather than setting it here if we're setting it when we've created the thing or what, if this thing has changed since then, and what is this old content even supposed to do in the first place?

A

And uh so I did try to recreate a scenario where, like okay, the note has been created, but it's changed after this bit has been done and then we submit and like is all of that expected to work as possible, but I couldn't find anything. So then I ended up just asking a question of like hey, so like there's a small side effect of of making this change uh and then mainly just asking like okay. Is it any possible that this side effect can cause any damage?

A

And you know was aware of this and just said like, but that they couldn't think of a actual scenario where it will play out so um so that was kind of good enough for me, I tested it out locally um and then we neither of us, could think of a scenario um it was brought up of like oh well.

A

Do we want to just add a watch to this, but like at this point, I'm just so afraid of touching this thing and unintended side effects that was like okay, if it's working as it is, let's, let's just keep, keep it um yeah. So it's it's challenging like so we did have um tests to update, but here with deprecated notes, um so deprecate analysis still used to render com comments from commence and Snippets. However, deprecated notes, spec is skipped because it's flaky and so I was like. Oh my.

A

The whole unit test for this deprecated thing is skipped.

B

It is to pull the statement. I've heard this week.

A

I know um so like I was testing. These things out and I was trying to like reverse engineer. Okay, what was the actual changes we're trying to make here and um but yeah I just wanted to share like sometimes we get code, reviews or high traffic things touching. You know touching the power of sticks and and just moving something very slightly, and this can cause you know catastrophic failures.

A

um Hopefully we have like feature tests that can catch those things, but it's really really worth asking at a high level from a black box perspective without looking at the code. What's the worst thing that can happen and then, after looking at the code, what's the worst thing that can happen and then trying to mitigate all mitigate that from all the s ones until like okay, we're talking about edge cases here, I think we gotta just move forward so yeah.

B

um Big question: if I made like how do you plan on carrying on with this because, like if I, if I think about the changes that this Mr introduces and deprecated notes and the fact that it actually doesn't have any tests when they were skipped like this is kind of equal to not have any tests?

B

um What do we do? Is that like I would be I would maybe maybe that's over the top, but I would lean like okay as long as we don't have any tests for this, and it's important just don't touch it at this point or find another way or go fix, make the tests not flaky and get some coverage first.

A

I think those are great ideas: um I I, lean towards I tested it out manually so um and it was broken, but it was pre-existingly broken. So the actual notes themselves were like really wacky and so I was like I, don't know, I think we just have to migrate. Deprecating notes to other notes, uh but I think it's a great idea to like okay. Why are we skipping all of the unit tests? Why don't we?

A

Why don't we just skip the ones that are actually flaky um or maybe we need to create deprecated notes, not flaky spec or just these changes um but yeah. That's a good question. I agree with what you're saying is like yeah. If we don't have tests for it, it's likely, we probably shouldn't touch it.

A

um For some reason, this was like rated label like P1, because it's related to some other thing that they're trying to do. um Oh, do you hear my alarm going off.

B

um Didn't hear anything, oh.

A

Okay, my house alarm's going off and I guess this means the kids are out.

B

As long as there's no fire yeah.

A

Yeah, okay, they're they're, pretty good. They know how to uh they've read books about volcanoes. They know how to take care of the fire. um Let me turn off the alarm because it looks like my wife is not waking up to do it either. So, uh but that was all I wanted to share. I'll, be right right back.

B

Awesome sweet um in the meantime, James I know. You've got something on your plane. um Should we just um try and start with it and see when Paul comes back.

C

Sorry yep, that's good!

C

um Let me open it up.

C

C

Yeah so with this Mr I on first glance, I had a look at it. So, okay, like it's, it's just changing something: straightforward, there's nothing complex here. So like I'm sure it's fine, you know like if someone's doing this on purpose, it's fair enough um like it's Community, but it's like it's not really Community, um so I figured. It must be a good reason.

C

um So, okay, fine with me and then I, saw a note from Kasha here saying like um she, she said: I, don't know what this does so I'm, not gonna approve it, but probably James knows because there's more front and focused, but I was like I, don't know what it does. I'm sure it's fine! So then I approved it and then I was like and then I saw this and I was like oh oops, um yeah I shouldn't have done that I should have.

B

C

Little bit more thought um so yeah trying to work out like yeah like yeah, well, I, I, guess, there's a few things I want to know. One is like what is my responsibility as a reviewer like?

C

Why is this being done like who's asking for it to be done like how much am I? uh Is it my responsibility to dig into this or because I sort of figured like with something like this? My responsibility is like? Are you introducing any bugs, or you know what I mean like with something like this? It seems like pretty straightforward, so I'm.

B

C

B

Yeah I'll um I'll give this a shot, so um I I quickly, skimmed through the SMR, so I I partially know what I'm speaking um about here, but how I am reading this thing? If you go to the Italy change the step, one more time um to me, this fear is like a little we're. Looking at all those those pages like this is these are the ones we care about, like uh merch requests, merge method, settings options.

B

What not um I think the auto has a point as if in these pages are CE only at that point and are like it is kind of unnecessary um for us to to use this function, and if I read it correctly, it will save them like some some override or some work around or whatnot. The problem that that I see with that is that this pattern that we're using rendering everything in CE by default and then we're gonna, also have an ee page just in case we need it.

B

That is I would argue like an architectural question um in our code base. So that's where things getting a little tricky as if in like rendering things CE is our default and then, when we need an EPA page, we'll just add and render ee method, and this will just work and if we introduce this change, this will not be the case anymore.

A

C

Want something to be immune, CE is Community Edition right. What what's the point of having a community edition only like is this like Community Edition is the base right well.

A

Yeah right so this this uh Echoes, how we do um ee overrides in the front end where uh the ee override will need to.

A

um Oh, we have ELC, that's right, that we have that prefix I haven't used that in a long time uh the eelce prefix, but then ee often needs to import specifically CE to overwrite it. So that's when we would use render C otherwise you'd end up with a loop. If we had, if I had ee override Hamels you and then that helps you rendered itself trying to reference the CE version of The View, it would end up in a loop.

A

So that's the only time I think we want to use, render CE or render ee we wouldn't use, render ee that's something that jihu would probably just use, um but in this case that, like there's a there's, a small side effect that could happen by changing these from render CE to render, uh which would be if these had previously ee specific pages um or ee views and if they don't, then then we're probably fine but um yeah.

A

The only time I think we want to use render CE by itself is: if we have a specific page, that's targeting a specific CE page, we would need to break out of our automatic render overriding thing so that we don't end up in a forever Loop. Does that make sense.

C

It doesn't make sense to me at all: okay.

A

I'm, probably a bad job explaining.

C

It so no that it's it's a complicated part that I've never touched, but my question is like at what point should I be like? Oh, this is not my area I'm out hand it over to someone else and at what point should I be like? Okay, I should uh understand how this works properly. You know what I mean yeah.

B

To answer that question I would argue whenever you feel uncomfortable like if, if you get an essential Mr and you don't really feel it and you'll be like okay, it'll, probably take me a long long time to actually figure it out, I was like I would always recommend to keep on following it to actually check out the resolution on this. But if you're feeling, if you, if you're assigned to anything that is not in your ballpark, for whatever reason it might be, don't be ashamed or afraid to just Loop in anybody else.

B

Just like no, no shame in it.

A

Yeah but I think like looping it in to and then anticipating on gaining like the understanding, because, hopefully growing the knowledge of the code basis is a something that um we could do collaboratively.

C

And then like on top of the I know, this is pushing it too far pushing the example too far but like at what are there? Are there parts of the code base where you you are like nah I'm out and and how do you recognize them? Sorry say that again, Yannick.

B

um I I just said: Geo I, don't know if you had any touch points with it, but in the very short stories like when you need to have multiple GitHub instances that communicate with each other and it's very, very hard to reproduce locally.

B

um So that is definitely and I learned that through pain, um I learned that since I tried it to set it up locally, and it was really painful.

A

Yeah I um there's many times, I feel like. Oh, no, no, no, no I, don't want to do this, and um but I don't think I've unless it's like clearly inefficient for me and it'd be super efficient for someone else. Then I might say like oh hey, do you have capacity? Do this because you're very familiar with all this? Can you do that? If it's a case where some somebody's got to do it, we've all got a chip in it's like okay, I've got a and emails is the bit that will come up there.

A

For me, it's like anything, that's changing! Oh, we didn't do emails. I I had to deep dive into that into that topic, um like maybe a year plus ago, uh which is normally something I avoid because it's a lot gnarlier than other views, um but yeah that that comes up but I, think I.

A

Think you're looking at this one like so, the jihu context is interesting if you're not familiar with it, um but I think you can do a search for render CE and I would recommend just doing a project-wide search if like what in the world is this doing? How do we use this thing? We do project-wide search for a thing of like okay. How do we use it in other places and is it making sense there, and um if we do that real quick? Do you happen to have the gitlab project open.

A

Yeah, if you just do a project-wide search for render CE.

A

You'll see a whole bunch of so there's like looks like documentation on it. You'll see a whole bunch of these views that do it just once, because it's doing that override thing that I was talking about um because it's rendering itself but the CE version of itself, uh but the parts that stand alone are like the merge request. Settings is being updated here, where it doesn't need to be doing that. That's breaking this pattern and I think doing a search for sometimes just looking through the code base of other times.

A

We do a thing that can that can shine a different perspective on the thing you're looking for at.

C

Sorry, so why is this different.

A

If you look at um like take the second example there, the email settings and you look at the path that we're rendering it's the same as the path of this View.

C

A

And if you look at the documentation, I'm pretty sure, that's that's what it's describing as well. It.

C

A

And it's saying this is yeah could be referring to the C, partial and C, while e partial is e. This way we could show different things and see in ee. However, sometimes we would also want to reuse the CE, partial and ee partial, and that's that's the use case for this function. So.

C

Yeah would be like uh I, don't know what this is or how it works. So I'm going to spend a little bit of time, trying to understand it. If I have capacity, I'll spend more time and.

A

That's so so that, and it's so so valuable like having I, am completely on the outside I'm not familiar with this, and this doesn't make any sense. It's really great feedback to get I mean try to spend five minutes sense making of it, but then after you've spent that time- and you still can't make sense of it- give that feedback of I'm trying to understand this, and these are the questions I have about it. Let me Loop in someone else, uh that's a totally great feedback to get.

A

um If you just back down of like I, don't want to review that! Oh man, that's! That's me! Oh, what was I doing there yeah I would um I would I would put in the effort to try to review, even if it's really really outside your territory, because that outside perspective is really really valuable.

C

Okay, cool makes sense thanks.

A

Yeah, what do you think Johnny? Do you think it's.

B

um I highly agree: um I um I, don't know. If the with my very first statement, what I said would um be the same, what you just said, but that was basically what I meant so um yeah absolutely I do think that there is uh a lot of value um in anybody trying to get their their head around it and especially providing feedback, um because maybe maybe the dogs are not yet where they should be or whatever it might be.

B

That is kind of um making this easier for um Outsiders to understand this, so um yeah highly agree um still don't feel like um that. You would have to bring this over the Finish Line. If.

A

B

You're comfortable with that, but um yeah, even even like the like, a quick, quick statement about the current state or the level of understanding, and especially the problems with it um as a statement and then getting getting in some help. Absolutely the best way to deal with that.

A

Yeah- um and this comes up for me too- of like we're using a we're using a um uh we're using a technology or like a third party library that I'm not familiar with um I may realize I have no familiarity with this I guess: I'm gonna, you know, excuse myself or I'll open up, docs and other examples using this third party library and compare it to what we're doing and just recently I saw like oh we're, not using. You know one of our.

A

We weren't using one of our Sentry methods, as it kind of seemed, to be anticipated to being used um based on the docs, and so just doing that outside perspective can can go really really long way of uh so I I would recommend trying to not excuse yourself, but definitely time blocking yourself. Is it's a wise thing? If you're you.

C

A

To do investigation.

B

What'd, you say on it: um can I just quickly come across with with a technical question on this, because I, maybe I'm, currently at a dangerous level. There, like I, have understood powers of it, but not a complete thing. So, um regarding the the render CE function, do I get this right that when we would replace all our renders by render CE, we should kind of get away with it as if in like just just wouldn't change anything for us.

A

If we replace render with render CE exactly it, it would mean that we won't try to look for ee version of a view. We would ignore.

B

The version yeah.

A

And would possibly break to you.

B

Okay, because that is if you could open the Mr once more gems, there's like one slight thing that I would worry about when, when I'm accepting these changes,.

C

Yeah one sec, all right.

B

So how I, how I read this or how I get this currently so, basically, the author's saying like this is currently done with render CE, um but there is no actual ee page um forward, so we can just use the render and it won't have any effect.

B

um What I would worry in terms of like this is a small little more soft rule than actually, but if I would come to the code base and I would need to introduce like an ee partial for one of those pages.

B

But then I would worry that we might be in trouble as if in like that. Might you could argue that that would be my problem, but I would maybe assume that this would just work out of the box and as if and I would go in and be like render EE and potentially break something. So what I'm? What I'm trying to say is um if there is no harm currently in the render CE like does. This is like a trade-off, but it'll make us easy.

B

It'll make it easier for us to um to add on ee partials. Then why not just keep it.

A

Are you saying that there's no harm with this there's no harm with the CE, we're being more explicit and so future changes won't accidentally bump into anything 100 yeah that that does make a lot of sense. The the situation with jihu is that it causes them some harm. So if you're G who's Forks get lab, so we have one tier of CE changes.

A

Ee then provides some overriding. Sometimes on top of those CE changes, then jihu provides some overwriting on top of all of those, that's external from our code base, but every once in a while, we have to merge in extension, points for their project and that's the situation here is I. Imagine if you, if you dive into one of the issues or context, is they're wanting to actually have a JH version of one of these views or something.

B

Yes, that's very what if we go see the overview tip once more, um if I recall that correctly, um there was like a um a statement regarding a workaround for them.

B

No, that's not what we're looking for.

C

um Let me quickly.

A

So, do you see like.

B

What I'm, what I'm, trying to the point I'm trying to make is if there is like an acceptable work around for jihu this. This boils down to a little bit of a. There is none yeah.

A

No and I I I think this is just how they're in anticipate I'm like there's likely other hacks, that you can go around it, but the reader function is how I think the most likely anticipate being able to extend.

A

um But with that being said, um like you're right, adding an ee view can have unintended side effects. um So we're not every time we add an ee view, you have to be mindful of the naming Collision that could possibly happen and see. uh So that's one thing to be aware of. As reviewing and authoring funding code is new. Hammel views could conflict in names and it's a it's a thing. Yeah.

B

And, on the other hand, we are using the vendor method a lot of times in our code base, so.

A

It's not like it's like deprecated.

B

Or whatever yeah.

A

B

A

The intention of the render c e and ee is meant only for when we are doing an explicit override. We can call the super class.

B

So DC render CE is not the default. No.

A

Yeah, so you got to be really careful with adding new handle views and make sure that the names don't conflict. So that's something that I when I think about. What's the worst thing that could happen, that's one thing: I think about.

B

A

Yeah lots of little uh what lots of little little traps.

A

Cool yeah, that's good! That's a good question and it's good to be think. Thinking from an outside perspective is like those. Maybe there is some another way of doing things and, like you, don't get that when you're, when you're really used to this- and it's like oh, this is just the way we do it. You just you, don't get those thoughts as often and so I love when people from without familiarity, review and marks, because that's really really sweet collaboration and ideas come from sometimes.

B

Was that helpful to you James, like do you know if you're prepared to to bring this uh to the next step.

C

Yeah awesome yeah makes sense, yeah yeah. It really does thanks. Okay,.

A

uh Michael did uh do we have anything else on the agenda or do we want to maybe pair up on a merch.

C

Question anything else on the agenda. I have another one that I'm a bit stuck on um sure reviewing. Could we bring that one up.

A

Yeah: let's do that that.

C

A

Oh wikis I'm out.

A

uh Dimitri has been, um let's say on: fire has got to be the the word I have received so many Community contributions from Dimitri, um so Props to the contributor for being uh extra busy.

C

um So I I'm really not sure if, like like, if this is in view, I can read it and be like yeah. Here are the problems, but in Hamel I'm like okay, no I need to like line by line, spend a lot of time. Understanding this I don't know what is like. I'm not familiar. I still can't get my head around the um like the syntax or like I, don't know when I can't see errors, I can't see them.

A

C

A

Find it usually hard to read the diff from the MR view, because I think sometimes the code editor is able to figure out white space changes and other things better than our um marriage request field. uh So do you wanna? Do you want to review this one together.

C

A

Can we maybe check out the diff locally? Let me can I show you my favorite way of doing this yeah please. um So if you have a terminal open and if you can check out master.

A

But if you have changes that you get it okay, great all right um and you can update if you want to, but you don't have to um request.

C

A

Then copy the URL yeah and then go back to the terminal and then do curl and then the URL space, but then get rid of the slash divs part and replace it with a DOT diff.

A

Yeah and then space pipe git apply.

A

This is my favorite way to review Mrs locally, um because I don't have to worry about changing branches and I, see all the changes.

A

uh So then, what I'll do as a very last step is I, usually stage all the changes, because if I find myself making changes on top of it since their difference between what's staged in my my head changes, then uh I can keep those so yeah cool all right. So let's check it out, so we got Wiki's main links.

A

So it looks like we're: switching the order of.

A

Something so I see this page. History thing is getting removed uh right, but is it being replaced with.

C

I don't know honestly I try to look at this several times and every time I did I was just like I, don't know. What's going on, I can't handle this I'm out yeah.

A

One thing we could possibly do is um like uh to understand: what's actually changed between two lines that aren't right next to each other: um I'll, actually like change up, I'll move the lines and the code to compare it. But since we staged the change, we won't get that view we'll have to unstage it and then move the lines around. uh If we want to do, try that out so I'll like that, if can that's on line six and seven.

A

um Yeah I'll move that up to the line that we removed, and so we can see what's actually changed. Oh you go up. One more I think yeah. Okay,.

C

A

C

So then, this line is the same.

A

C

That Line's kind of the same.

A

So what I find really really interesting is all of a sudden for some reason we're adding more conditions to the page history button.

A

So I want to understand the context of like. Why are we adding the conditions line? Two, the condition seems really long. It's like okay. Maybe this needs, rather than doing all this logic and the presentation Hammer layer, maybe we, the logic should be I'm such a bad rails backender. Maybe it should be in the helper uh I, don't know, let's keep looking at the rest of the button. The button is really long if we scroll to the right on line three so yeah, oh yeah, so look at this, so that's edit.

A

We've totally lost the history button altogether.

A

um Okay, so so that's why it's in on it's wrapped up in ah how interesting.

C

So one of the things that was a bit overwhelming to look at is in the differences particularly like this makes sense like if we do. Okay, that's what it was now. It's here.

A

Okay, I'm cool ah and that's where page history went I, see I, see.

C

But on mobile this looks real weird oops.

A

Hey look at that cool little pencil button.

C

C

I, don't know what's happening here.

B

um We got dear on how I tend to approach like Hummel um changes that I, like you, also tend to not understand or just having having a hard time getting into it, um executing it and actually looking at the the output of HTML helps me a lot, and that is. That is what I kind of care about, maybe even like from the chicken chicken master back out and looking onto what this was initial was and then the actual yeah outcome of the changes introduced.

B

So that is then, when I know whether I, like it what's coming out of it or if I, do see any issues and can then go search for them at a hammer. That makes it for me way easier than um trying to make sense out of the summer, which.

B

A

Don't like camel.

B

It's too close to Yellow. For me, I, say: I, see.

A

uh It has a significant lack of curly, braces and and pointy brackets.

A

um I started teaching my daughter. She really wanted to code like we she's done scratch before, but she was one to do like actual coding, and so part of me is always like. Okay do I do python. I was like it's gonna, be a while before she's. Actually using a GUI is like okay, we're just jumping right into HTML JavaScript, because that's the most immediate feedback you get from doing stuff, but every time I teach HTML to someone. They always laugh at me when I talk about open, pointy, the pointy brackets and they're like.

A

Is that what you call them, because, like oh I, thought you were smart? Okay,.

A

Like yeah, you got your pointy, your curlies.

B

A

C

B

Still getting sweaty hands for me, like I, remember when I, when I joined, gitlip and people your Superman, with sports from all over the world. Try to explain me stuff and I was just typing and everybody had a different name for a different bracket, and I was just sitting on my keyboard being like. If nobody wait what um so yeah it can get quite messy. Well,.

A

Is there another name besides the point scene, for these things, the pointy.

C

Bracket I've heard a good Arrow brackets. Oh.

A

Geez that looks so confused.

C

Well, alligator I have heard that.

A

um The open, Chompers yeah, all right, um yeah, I, uh I, think your your point Yannick about actually running. It is a big big deal because um sometimes you'll find that, like oh we're, setting an attribute, that's not even being read it's just being written as an HTML attribute to this thing. um So that's checking it out. These Hammel things is is a really wise idea.

A

um I, like that I.

C

Would and I wouldn't so do you mean like inspecting it and.

A

Yeah I, wouldn't I would inspect it um to see how these things are actually being structured, because sometimes it's like oh well, the way it's being rendered is all this we're putting divs inside of a span now or something and.

C

A

Okay, yeah, so there's our main links and looks like yeah, we end up, which is the a button, and then we've got the wiki actions drop down cool yeah. These are just eight traps for it. The.

C

How much are you also getting involved in like some like design or usage like um yeah.

A

That's that's a really good how opinionated.

C

Are you or if you're like well, this seems weird, do you just pull in a ux or.

A

This is a great question, so I'll I'll get involved if I feel like if I see ux has looked at this, but what I'm saying still seems very questionable.

A

I'll leave a ux question um of repeating back to ux. Like hey. Are we really sure about this? Have you looked at this? This side effect of that and I'll do that um sometimes ux has made a decision and I'm saying hey speaking as a user. This is what it all communicates to me, and none of this is helpful. You can please not do this I'll do that, um but in this situation since ux is currently assigned to it, I'm not going to be super concerned about ux things.

A

At this moment, I know after ux has given all their feedback. The Mr is probably going to look different, so I'm really looking at this moment for um are we are we approaching? Are we doing anything from a front-end perspective? That's a really off approach, or have we not set a test for this thing? Whatever that is and test is a big test is a big deal. If you actually go back to the Mr Dev I, say Tess is a big deal and I merged a whole Mr that totally skipped tests for deprecated notes.

A

um We have Hamel tests and we have you know non-trivial conditions that we're adding here like. Can we create a Wiki page latest in dialogue, encoding and those um it's worth uh I think it's worth figuring out um yeah. We probably should add test to these, and so there's a whole thing called Hamilton specs, um and if this, since this is a community contributor, they may feel a little uncomfortable doing that so I will probably reach to a back end. I'll. Ask them hey, let's try to add some tasks for this.

A

Some handle tests for this here's an example: I'll do a project by search for Hamilton spec. um If but then I'll also leave is like hey. If you need some help doing this, let me know, and then I'm, anticipating on reaching out to a back-end engineer, to help the community contributor right some handle tests.

C

Okay, yeah, that makes sense.

A

C

A

C

Helpful I don't know why I was just stuck on it just well.

A

I mean it is, it is it is. It is harder to follow because of how long these lines are I think um but yeah, uh so I've I think if the ux is cool with this change. At that moment, I was concerned that, like oh we're, getting rid of one of our buttons but I see it goes here into the drop down. So I'd want to just double check if you, if you scroll down um I'd, want to double check that we're not losing any of our conditions.

A

As we've moved some of these things around, um oh and so yeah you see on line 27 that we removed on the show HTML Hamel. That's where that funky condition comes from so it makes sense like okay I see we're just moving stuff around. We maybe don't need to.

A

um Maybe we don't need to uh what I'm trying to say refactor.

C

The tests, if there is a test.

A

Well would suggest testing it, especially because we're kind of I don't know.

C

Sorry I just use handle tests at all. How would I find that that's.

A

A good question: if you go ahead and go back to your um your vs code and then I would do a um fuzzy file find.

A

For uh Hamill underscore spec I think.

C

A

So these are all these, like Hamel tests.

C

A

um So you see how, in the description, we're specifying a Hammel, View and.

C

Then there's the description. Sorry, oh yeah.

A

This yeah yeah yep, and so then, and so you gotta you gotta, just embrace the R spec rails, magic yeah that automatically Associates this whole test. With that view, so when you run on line 20 render um rendered will be the HTML of the things.

A

A

But you see like beforehand we're specifying that hey these views can receive current user and other. We have to specify all the collaborating writers, so I would add just one test for the show camel spec that the buttons show up, and maybe that test already exists um and since we're just moving things around it all works.

C

To find that I'm just going to do a search for the name Rin, maybe with that yeah.

A

Except I would look for, um I would look for the show One the wiki show.

C

A

This one yeah that one and I think you might have to do the the fuzzy file named search for it.

A

And if we do underscores backpack, maybe if we add a Wiki at the front of this.

A

A

I, don't yeah I, don't think we do have it.

C

It would also have to have the name of the file in it right. Oh.

A

That's true: that's.

C

A

Didn't realize oh yeah, you're totally right I wasn't thinking it would, but it totally does sorry about that.

C

A

um It'll have the whole path to it too, so it should be Wiki. Slash, show.

A

Yeah, you see, we don't do animal specs a lot because they're not necessarily loads of fun, but it's also worth looking at. Maybe we have feature specs for these, and so we can well see we have like QA, selectors and stuff, so we definitely have end-to-end tests for some of these I guess. But maybe if we have other test IDs, we can search there's a test ID the Wiki page. Oh that's just the page title though.

A

Yeah, if we go back to yeah, if we don't need to do a handle spec, if we have a feature tests for this, uh if we have a test ID, if you go to the far right, there's a QA selector for the new page button.

A

There we go. The wiki edit button. I bet a feature, spec references that just says: half selector. Okay, all right! This is like doing a handle spec at the feature level, which is fine-ish, I, guess.

A

C

Yeah I mean it's just checking to see that there is an edit button. It's not testing the edit button.

A

Yeah, that's that's what I'm uh that's what I'm observing but oh but look! We can also click on page history like it's showing that oh does this test even work. Is this working I, don't think it would on line 127 I bet the pipeline's failing.

C

uh Yeah pipeline is failing.

A

C

Did not even run.

A

It probably didn't run because.

C

C

What else would it like this looks like it ran right, but there's no.

A

The large number, the eight eight ninety seven.

A

You know what this is: a really unhelpful, View.

C

I'm sure, probably like earlier today, it had stages listed out really.

A

C

A

uh One one thing that can happen is the um Community contributor runs out of minutes, contributing oh yeah, um uh CD on it yeah and we'll hop off.

A

The community contributor can run into issues um running the pipeline and then we somehow need to think maybe it won't run on our end. Unless someone approves it or we add a label or something like that, um I'm just oh yeah or click run pipe run pipeline might be a good idea. There's no bad there's no malicious code in it. Is there now.

A

C

All right, yeah, that makes a lot of sense. Okay,.

A

Look yeah, it looks like we have feature tests pretty much covering so I'll just take a glance through those, um but it does seem like those might need to be updated because we'll need to open the drop down before hitting the page history, button and stuff like that. So it's um but yeah. uh Some of these handle ones get difficult to follow.

A

C

Well, yeah, that makes sense cool thanks, great.

A

Thanks for bringing us, though, it's very interesting, James, well uh I'll, let you go and uh thanks for hopping on right, talk later bye, see you bye.