GitLab Backend Pairing, 7 Jun 2023

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: BE Pairing -- 20230607 - Add service and mutation to set user namespace commit email

Description

Today we paired on https://gitlab.com/gitlab-org/gitlab/-/merge_requests/108236

A

B

I'm, just um I, don't know if it's Marco or Raymond or someone that taught me about the multiple desktops thing so because I have so many windows open and I wouldn't share my screen, I'm just going to try and move dumping over to my other screen. So I can just share that um in a minute, maybe Marco while I'm messing around. Do you think you could share your screen just with the Mr open and just talk, give us a bit of background and remind us whatever we're at.

C

Yeah I also have to remind myself, but yes, so, let's share my screen.

C

Okay and I think you can see my screen now: yep, okay, so basically five months, wow five months ago, I opened this Mr to basically add a new mutation to be able to set the uh was it the email.

B

Yes, so we've introduced this New Concept called namespace commit emails um or we're sort of halfway through introducing it. Some of the tables are there, but where we're lacking a bit of the the graphql functionality, some of the back ends there, um but we're lacking a few bits and pieces, and then we need to start building the front end. The idea is that um if you contribute to multiple projects, maybe your companies on gitlab.com, um but maybe you contribute to a couple of Open Source projects on gitlab.

B

You might want your company contributions to come from your company email. You might want your open source ones to come from your private email. You might even have either some other kind of business contracts, side, hustle, whatever you want to call it email. So um it's it's to be able to kind of configure a a default. If that's the right term say when you're using the web IDE or using when you're merging through the UI or using a single file editor. All of those things will check what your namespace commit.

B

Email is that's configured, and this Mr specifically was to allow us to build the front end. So you can actually configure these things, so we've got the backend tables to store it, um but we need a graphql endpoint to be able to to send some data to and say right, okay um for me or for this user and for this namespace use. This email address does.

D

B

Sound about right, yep awesome!

B

So where did you get to.

C

The last one who worked on this was actually you, oh.

B

Thanks for the Fire Conference Terry.

A

I was like I hate I'm like who touched this last.

D

A

It's always like. Oh it was me.

B

Has anyone got freaked out by the additional spacing on this page recently, by the way, oh.

C

A

um I, don't ever notice space differences when people call things out as being misaligned like I, don't know, I think I just kind of like don't it doesn't. I have no idea what you're talking about. Sometimes.

B

It's just weird when your brains, like something's, changed.

C

The OCD kicks in yeah.

D

A

Is why I'm not a ux person, because I'm like eh, looks fine still.

D

A

um I guess I took a quick look at this. It looks like there was some work on the model to add a new method that grabs the email.

A

B

All right, it's almost ready to.

C

uh I'm almost certain that the implementation was there, we were still only missing some specs. Oh.

B

So so it's an opportunity for us to maybe just um review the Mr, almost and and kind of just yeah see if there's just some nice finishing steps that we can.

D

A

I see a note also about the policy, maybe needing an update from.

C

A

um And this is stuff: I don't get to work with on a daily basis, so that's kind of fun. Awesome, I,.

B

Like the sound of that um I'm just trying the rebay so um should I share my screen. Now: ah conflicts yay.

C

C

Okay, now I remember why I left that comment.

C

Well, it was four months ago: I, don't remember what it did yesterday. So it's tough.

C

Okay, so just to give a little bit of context about this comment. Basically, we changed the.

C

Written name space did we change the policy policies user policy? Oh!

C

A

Oh, the new space service is I. Think it's getting a new policy called admin. I guess it's like administrate user email address. Is that what that means.

C

Hope policies, admin, user, email address, and this is enabled when.

B

So you're it's, however yourself, so you can admin your email addresses or if it's an admin, yeah.

A

Yeah, what is the little tilde subject? Ghost means that means it's, not a ghost user, exactly.

B

A

User and it's you or an admin, then you can change your email address.

C

Yeah but we are not checking that.

A

I saw it somewhere, uh maybe in the uh service, under the yeah, under service users,.

C

Oh yeah we're checking written namespace, but where are we line.

B

17 in that file that you had opened.

C

Yes and that's true, but where are we oh read, namespace was already there that's a permission that already has existed.

A

C

Oh and I think that um I have to remind myself. Where is this.

A

Let's hope that search gives you what you want.

C

Okay, so this makes sense, because reading namespace is coming from the username space and from the group project name. Space, shared policy.

C

A

So you should be able to use it on both types of namespaces, which is what you want right, because.

C

B

Exactly yeah I think so.

C

And we are providing this one from the user policy.

C

Oh, my God I, don't remember what we did and why we did it.

C

So the user doesn't exist or you don't have so only if the current user can admin the user email address of the target user. Okay, yes,.

B

We almost don't care about the namespace until we've checked that the user's actually got permission for them to do. The email changed in the first place.

C

Yes, and we also check if the the target user can read the namespace. So basically, you can't set the email or the for a user to a namespace that that user can't access exactly.

B

C

Okay, then, why did I left that comment.

A

C

Basically want to allow this mutation only when the written namespace permission is granted. So that's granted when you have access to the namespace.

C

C

Okay, because, if you, when you're using the mutation, if you can't yourself read the target the namespace that you want to manage, then you can't use limitation yeah.

D

C

Should we add it projects.

B

Let me just reread I missed always just rebasing.

D

B

We edit this permission, so so that's project, namespace policy um and have you still got your you've got your search results? Up still, I mean it was Project namespace right, so project names.

C

I wonder if this got added after a comment: I.

B

Yeah, you can actually do blame through this UI. Now can't you oh.

C

B

um Yeah, maybe not on here, yeah, look in the left-hand.

A

Model like if you hover by the numbers, I think it'll give you a blame link. One.

E

A

Those icons, oh.

C

Yeah two months ago, yeah.

A

That's nice, okay,.

C

B

Maybe copy a link to this and just pop it in there and say it looks like since we, since we did this, this has been added yeah.

A

C

C

B

B

D

B

Any other outstanding.

B

B

So this might just be like a Mr review.

B

Should we go through from from the top and and kind of review what we've got sure.

C

Yeah, that's that sounds like a plan. Do you want to do it or do I go from here.

B

um I'm happy for you to start you've got your screen up already and yeah all good.

C

So, okay, uh this is the new mutation and we have two arguments: the namespace ID and the email ID that we want to set for the user and the user is coming from.

B

So at the moment it.

C

Looks only the current user can.

B

Manage itself I think we, we sort of suggested that we would add the ability in a future iteration to be able to to.

C

B

Other users, but but there's a minimum viable or whatever we want to call it. That's just the current user. For now.

C

Exactly and this mutation is returning the uh the new link basically to between the namespace and the email.

D

C

We check the the current user is actually set, otherwise we can basically do anything. Then, oh and here we try to find the namespace from the namespace ID, and this is an authorized file. So it's applying this policy yeah.

B

C

Sure that the user can actually read the namespace and we do not check if the user can read the email, though.

B

A

Oh yeah I wonder.

C

We that's one thing that I'm sure of is that we can't use unauthorized fine for that as well, because we can't have more.

B

Than one authorized absolutely, um can you have a look in well how far.

A

Can we change that record not found exception to check if the user can do it.

B

So my thinking is so we we're not checking or our way um we're not checking what was it yeah I was going to say we're checking something else in here. Aren't we right we're checking admin user email address, but at the moment, because you can only hit the mutation as the current user I guess we don't need to check this. Do we.

C

uh No at the moment, no because current user and Target user is the same and we have this so users help so.

B

But in the service I notice, when we grab the email address, we're grabbing it um we're doing. Email is targetuser.emails.findbuy. So, okay, if it's an email, address, that's attached to.

C

Yeah, because if the email is null nil,.

C

On this, create namespace comment, email and the create one will fail if the email doesn't exist. So if you can't find that email well, basically, you can only add the emails that you can manage because their the email Associated to your user yeah.

A

Yeah, so it must be an email that the target user has.

D

A

With their yeah.

B

A

C

Yeah for now, yes, okay,.

C

Okay, so that's that's good, and so we get the we actually don't get the email. We only get the email, ID.

B

Yeah I think that's kind of deliberate to say this loud in loading it twice. If that makes sense,.

C

B

So arguably I suppose we could load it here and then pass the object into the service. But yeah.

C

That, then, we wouldn't be able to check uh to use the target user emails fine by.

B

uh Yeah, because that's um there's cop, to prevent you from doing that here is there.

C

uh Yeah cool cool, uh so we call the service and then return the reload, nothing, nothing big in here and the find object so that we uh can find the namespace from the name. Space ID, so I'll draw is fine, basically uses find object, go okay, mutation type only exposes the new mutation. Nothing big in here. uh uh This one is used from the service because we try to find okay.

D

C

If the um basically, if the user has already set a custom email for that specific namespace, and this one is getting that basically looking at the table and finding, if there's already a record for that namespace cool, uh so that's cool here we are enabling the Paul the the right to admin, the user email address, that's good and here's the service.

C

C

E

One something I noticed when searching through the emails sync, you don't make sure that the email is also confirmed so that the user actually confirms the email.

B

C

B

I'm just gonna I I, wonder if there's like a standard for that. If you know I mean if that's something that we have to do everywhere or if there's some kind of abstraction level layer or something that that we can use.

A

To avoid them, is there a scope? That's like give me confirmed, emails, I, don't know, question.

B

E

You can use that.

B

No, um so this is immediately meaning we're going to need to write another test. um Yes,.

A

The only other thing I would note here is: um is this service because we're almost doing uh like a fine and then a create or an update? If the record is found, there might be like an edge case where it's like service run multiple times like too close together, where it might get into a funky State um I, just like reviewed a database Mr where they talk about using upsert, and that might be something we want to look into here.

C

We actually have also a third case, because you can use the same service to remove the email. Okay.

A

But that's only I guess: if the email ID was not provided, then it would be removable um I guess maybe that could be the first check and then, if it's the email, ID, um okay, uh I, see yourself. Okay, I see what you're saying, but we like, because I, let me see if I can find an example for the upsert in the code. There's nothing in our Docs and there seems to be some rubocov rule about safe, binder, create by like that, not being good to do all the time. So let me see.

B

C

And at this point, how are you with setting up the code the because otherwise we're gonna at least I'm, going to forget.

B

uh Yeah I'm I'm good at what I've done I've slipped in the dot confirmed into the um line 27.

B

um So once we've finished and depending on how how you Dim for time once once, we kind of finish reviewing it I'll have that change and I just need to to kind of add a spec for that I guess.

C

Okay, okay, so I guess we can move on then, with this so yeah uh we get the current user. We already prepare to support other additional user plan and we get the namespace and the email ID okay. So we check for the right permissions and if the namespace is new, we can do anything. So that's good.

C

So we find the email from the email ID. If it's provided- and this has just changed slightly yeah- uh then we try to see if the user has already set an email for the specific namespace and if it doesn't, we try to create one.

C

Otherwise, if the uh email for name species set and provided email ID is nil, will remove that. So basically, this that removes the association. Otherwise, if that exists, and you provide a different email ID, uh we update the email.

C

Okay, so the remove namespace comment. Email just removes the record, uh the create namespace checks for the email. So basically, if you uh don't provide a valid email, ID or you can't read the email, ID or.

B

It's not confirmed or.

C

It's not confirmed this won't do anything, otherwise. This will create the new record and save that okay.

C

And this will just update. uh We actually don't check if the email is different.

C

Should we add that to save us from doing a save on the database or will.

B

Be Terry is probably a lot better position to answer that, but I feel like that. What.

A

Was the question.

B

C

If we already have a namespace email Association and we try to change the email, we're not checking if the email is actually different,.

A

B

um Tell me that rail's active records clever enough to.

A

I, don't know what it does. There um I was trying to write a comment about upsert, because I still think that might be something we want to do to create like to avoid a weird case where you might get like a duplicate record error or something um we could try it I, don't remember I work in elasticsearch, most of the time. That's why I do database reviews to try to get more practice about using database and another Factor records smart enough to just ignore it. My.

B

My sort of thinking was, but you know, I'm not in quite a good position to to say this, as you might be Terry that, even if it isn't that clever, like the additional code to check like I.

D

B

Is you know, is it really worth yeah.

C

This is really a one line that has to prevent one single save from a mutation. That's going to be used once every.

B

Yeah yeah exactly.

C

Yeah I wouldn't be too fussy about that.

A

C

uh So, having said that, these are only helper methods to return the right response to the uh to the mutation itself. So nothing big in here.

C

uh Graphql documentation- and here are the specs and I'm pretty sure that we have to update these.

C

So this is the set name, commit email. So this is the mutation itself.

C

And we check we check what do we check.

C

If the current user doesn't have the right permissions, how do we check that resultmentation object context, field.

B

So we haven't the the user, isn't a member of the namespace.

B

So if you look online 32, when the user does have permission, we're doing a group AD.

C

Okay, yeah, that makes sense uh yeah, also because that's the only thing that we're tracking on the mutation side so.

B

C

That's good. uh We checked for the parameters to be valid.

C

Are invalid, uh create email to Global, ID returns of validation, error image must be provided we are providing it.

C

C

We are providing an email ID over here.

B

Is it because the email is.

E

B

To the is that a bit of a that's actually testing the service, not the mutation.

B

D

B

You look in the service. Oh.

C

Yeah, that's the service yeah, because that message comes from here. Yeah.

B

So it's not this, do you call it an integration test? I, don't know what it's not not really a unit test. Is it.

C

No, but to Global ID amen. Oh basically, the user doesn't have access to this email, so it can't find it. It will be new, so this message will be returned.

A

B

C

Testing the service, rather than the.

B

C

B

We do do tests like that. Don't we so.

C

Oh yeah, true, yes,.

B

C

D

B

The only the actual an actual invalid one yeah it returned the same error, probably wouldn't it um or would it no.

D

It wouldn't I think you know.

B

You know: we've got that non-existent, ID written, non-existent record ID helper thing. We could possibly set the namespace ID to something invalid, because that validation message should come back from the mutation, but.

C

If you provide an invalid ID for the namespace or the email, the mutation itself will try to validate the fields on rather the arguments.

C

So if it's not a valid ID for an email that will return a completely different error, yeah.

B

So what you're saying again is what kind of testing um testing the I don't know what you want to call it like the gem or the library or whatever we don't.

C

B

That yeah good point.

C

Yeah and also um no yeah even providing null to the namespace. That's that will fail because it's required and if we provide node to an email, it's actually valid in.

B

Some cases yeah so.

C

Yeah I I think this can probably.

B

I think it could be removed, but I also don't think it's doing any harm. If you know what I mean I.

C

B

Valid use a valid test case, even though it's testing the service more than that mutation.

C

Yeah there's one thing that we probably are missing here, because we are only testing for the group. We're not testing for a project as a namespace I mean yeah.

C

And do we also want to test for a username space.

B

C

Yeah, that's still a namespace, so yeah I think that all of this could be a shared example and then only provide a different namespace for the three cases.

A

You could probably just do the shared example in there to at least let it be used easily.

C

A

I think it's if you're gonna do that it might be easier to review and read.

C

Oh yeah totally.

A

um I left some comments: I don't know the upstart stuff, I, don't use very much so I'm, not um so sure about in the service. What I've written is not fully functional but like at least kind of show.

A

The idea of um if we could somehow do the removal trick first and then like create a new record, um maybe like a validate that it's good and then use this up cert that would at least prevent having, like maybe a clash if, for some reason this to serve two times, someone quickly is like changed it twice and then two things run at the same time, because.

C

Absurd is basically trying to do an insert otherwise an update, yeah.

A

It's kind of like it updates or inserts a record in a single statement, but the problem is it doesn't do any callbacks or validations. So I, don't know if you like this needs that um like if this there are validations that like need to be.

B

Run handling one risk, but introducing another one is yeah.

A

I mean I, put the validate call in there, which you can say like if it's not valid return, the error you can handle that and if it is valid, then you can just do the upsert um trying to think uh uh there is like safe finder, create, buy and I keep getting feedback that they don't want us to use that anymore. So let me see if I can find.

B

A

Don't remember.

A

C

I'm going on with the rest of the specs in the meantime.

D

C

So this is just checking for the right uh permission to get applied to the user in different cases and time pre diction, oh I, would need to when user is not an admin, uh their own to be allowed okay, requesting a different user is not an admin and they're requesting a different user not to be allowed. Okay,.

D

C

And when admin mode is enabled, so basically, when the user is an admin and the I I always confuse myself about admin mode, how do you disable admin mode.

B

Well, it's not about deciding it's more about enabling it so the default! You oh no, do yourself. You use SAS in the office or.

C

uh No, we are self-hosted perfect.

B

So if you've got it, come for your button in the default configuration you don't have admin mode enabled. So it means that if you're an admin, you can do anything and everything. If you enable admin mode, then you have to actually switch to like press a button to go to into admin mode um in order to be able to do these administrative type functions. If that makes sense, oh.

C

So I guess I'm always in admin mode, because I am an.

B

Administrator I, don't have admin mode enabled.

C

How do I manage the admin mode? On our instance, you.

B

Would have to you just go into the sort of admin console or not console, but the admin UI and just enable admin mode, and that would just mean that you have to explicitly switch to admin mode. Oh okay, I want to admin. It's a bit confusing because.

C

Yeah because I am an admin, because I am the admineral our instance, but I can do anything so yeah I always assume that I was in admin mode and.

B

No, you haven't, got admin mode, enabled admin mode or maybe I'm getting the terminology wrong, but I think admin mode when you've got admin mode, enabled that basically means that, in order to do admin stuff, you need to switch yeah.

E

B

Need to press an admin button, whereas if admin Mode's disabled and you're logged in as an admin, then you can just admin.

C

A

Oh rgdk instances must have admin mode disabled because when I'm an admin I can do whatever I want.

B

Exactly exactly that makes a.

A

Lot of sense, thank you for explaining that I've.

B

Probably, given you the completely wrong explanation, but that's let.

A

Me know it sounds valid and it sounds that's why they have the trait right. The enable admin mode, train yeah. So.

C

Yeah, that's something different. Let's move on so when you're on admin and admin mode is enabled you can admin every email basically, and when that mode is disabled, you can't admin every email.

A

That makes sense.

C

A

C

Policy tests are relatively easy, uh so set namespace. Okay, this is testing this sir. These.

B

No, this is the request spec for the mutations we favor these ones over the mutation specs themselves.

D

C

And so this is setting up the code to the graphql mutation itself: okay, variables, name, space, ID, email, ID, okay,.

C

This is getting limitation response, so the user has access by default to the namespace because we are setting this outside. So every spec yeah.

D

C

Have that when the current user is nil? Okay, so General General case we prepare this, but then we.

B

um Current news: it's a nail there.

C

Yeah we post limitation with the current user Neil and we X do expat some graphql errors and that makes sense we're not checking what error.

C

Do we have to oh because that's not uh part of the Json response like it's not. Actually, uh you know that there are two different types of errors.

B

And data errors, yeah.

C

This is a top level error.

A

B

C

We do get the message, but it's different.

C

I I, don't remember exactly what the difference is in the response itself, but they do have different Json form if I remember right.

C

uh So when the user is nil, it's a top level error and that's coming from.

C

See this is a data.

B

It will come from the mutation because the user.

C

ah Come on yeah, it's this exception. Yeah.

C

A

We could check it if we really wanted to, but.

C

A

The exact error, yeah I, see some expectation examples in other specs, where they say like expect, graphql error to to include a hash, including message and then some value after whatever is in the message, but I, don't know what I don't know what that message would be for these two items.

C

B

Be easy enough to check Clara yeah.

C

It would be definitely easier now, but okay.

B

So let me um let me put the potential to do in so that's requests.

D

A

Can add a comment unless you, oh you're, okay, go ahead! That's fine!.

C

B

So like when it's time to do it, I might be like oh well, that's why I was too lazy to do it because of this problem, or maybe maybe it'll be simple, I, don't know, I.

A

Hope it's simple.

C

Okay, uh so when the user cannot, so we are overriding the namespace ID in this instance- and this is another error that should- and it's a different one, because it what was it in here, we would get.

C

uh In in theory, the authorized find should fail, so we would get uh 403 and that's a top level error yeah, because it's not an error in the data.

B

Itself, it wouldn't be a four I free with graphql.

C

Yeah like it would be a 403 on our rest, API and like yeah, that's what they return for top level errors like 403 404 and those kind of things should be no, why 404 shouldn't be oh I, I top level errors and that they are data? Errors are complex.

C

I never get my head around that so.

B

What one we looking at.

C

uh The when the user cannot access the namespace, it returns a top level error, because it's.

B

Because the authorized find fouls, basically yep.

C

B

C

What kind of iron should be, but yeah no.

B

I I've put it to do against that as well. To I, don't see any reason why we shouldn't be able to check for the specific error, yeah.

C

Exactly and when the service returns an error, this is uh this is not a top level error, so we get both errors and the namespace committee mail. So the field on the graphql response- and here we can- we can check uh we. We know what the error should be. Yep and.

C

Create a namespace comment, email this case. No, this is the successful one yeah, so yeah we check well. Actually this one should be for every one of the other tests, because everyone should every test should return. The success.

B

Yeah, maybe a better test here would be expect graphql errors to be empty. Yes,.

C

Yeah definitely I'll.

B

C

Yeah I'm going to have that coming in.

D

B

D

B

Very, very rarely with 500. Would it like 99 of the time it returns success but yeah it's the payload that you have to examine to see whether it was actually successful or not.

B

Is anyone been watching following the the Apple event at all.

C

B

uh Apple event, I think it's like that. Yeah.

C

uh I did uh see these in the Stream.

C

I was one I was thinking about trying to get a Mac. um How did they call it? The Mac Pro right only seven thousand dollars.

B

Well, they just released well, they haven't released, it I, don't think actually sorry they've just announced um or a couple of days ago that VR headset. That is rather insane looking at a price point of about 4 000 US dollars. So.

C

Yeah but well I think that, with the the Mac Pro, GDK should run very smoothly.

B

I can't imagine it'd be that much better than a MacBook Pro. Would it I mean it.

C

C

A

I know I might begrudging. Mac User I have no idea. I'm gonna follow it. I saw a lot of people were messaging about it, though yeah.

B

That's that's. To be honest, all right, I, like I've, not been following it myself, but in the yeah apple um slack, Channel I saw like everyone going a bit crazy, so requested what was going on. I. Think the one that amused me the most was um a picture of one of them with like a cracked screen on it, because everyone I know like iPhones, just notoriously everyone I know of an iPhone. The screen is cracked.

B

C

My my iPhone screen, never cracked.

B

C

That was true: I I actually never dropped. My iPhone.

B

Impressive yeah right, so this is the service spec and.

C

Yeah uh this is the service pack like before we by default. We add the user to the group. Oh yeah, uh oh by the way in this one as well I think it would be a good.

B

Opportunity yeah for.

C

The three kinds of new spaces.

C

And here well, obviously, then, so we check the execute when the keyword user does not provider. uh Well, it should oh yeah. We don't have to suppose that we are coming from the mutation. So uh if someone's someone uses this service from not from the mutation this, this is making sure that the execute yeah uh when the current user is not provided. This is going to make sure that the service itself Returns the error and that's good.

C

The current users do not have permissions to change. That's in here.

D

C

Yeah, so if they, the current user being new.

C

It should okay, it would uh go inside here, because no checking the permission for a null user will always be false. Yeah.

D

C

uh Yeah, in fact, that's the same message: okay yep, when the target user does not have permission to access the namespace. Okay, we're checking that as well uh when the name Trace is not provided. We're checking that as well email, ID.

B

To the description of that one, because specifically when one doesn't when a namespace commit email doesn't already exist exactly because if one did already exists, then we wouldn't need to.

D

D

B

uh In fact, the context block below I'm just going to move it down into that because there's a context block that starts on line 64., so I'm just going to move it in there.

C

C

D

C

uh Namespace comment: email: it's a mouthful yeah.

C

Okay, this is coming to me. It does not exist and that's the other case down there. Okay uh does not exist, it's it creates by default. Okay. So it's making sure that the new record is created and persisted uh when the target user is not the current user. Okay, so we are going to yeah at this point. If we already setting everything up, should we also update the mutation to allow.

C

uh Users to change other users, namespace committee, mail.

B

uh And I I think um I definitely remember that there were a couple of authorization checks and it just wasn't worth okay. You.

A

Know we all like advocate for keeping this as small as possible. Yeah.

B

It's already I can't exactly tell, but our 10 files, I I, think it's. You know hundreds of lines, um yeah 400 lines of code already so I think yeah um yeah.

C

So when the target is not when the current user is an.

D

C

Admin mode is enabled it can. Oh sorry, Nicholas, where you.

C

Was someone talking.

B

I mean maybe it's me touch the case: making noise.

C

Okay, uh so when admin mode is enabled and the user is not the current user, the user can actually create the the record.

B

Is not current user yeah? Does this stuff doesn't need to be in inside the context block that it's in? Does it.

C

You mean this context, yeah. uh Well, it's.

B

It's not really relevant. Is it.

C

Well, the thing that it's checking it is relevant because when the namespace committee mail does not exist, there are different cases where you can create the the record or not.

B

Right, okay, but the fact that the target user is not the current user is kind of irrelevant. It's the success that we're really testing and the success is relevant to this use case, because it's creating a new record yeah. Okay, okay,.

B

But then the like, when admin mode is not enabled, for example, yeah, is kind of not relevant. That could be outside the context almost because.

C

Yes, yes, but when the namespace does not exist and you don't have the permission, we should check that the um the service is written in this RR.

B

um So you mean we should check it in both branches. We did check that we get the same error, regardless of whether a namespace commit email exists,.

C

B

That's what I'm getting at the the permissions, like the the successful scenario here kind of it does matter that a namespace commit email doesn't already exist because we're checking in a new one essentially, but the failure cases will be the same regardless of whether a namespace commit email does already exists.

C

Oh I see what you mean.

C

B

Argue that we should check the failures the same in both scenarios.

B

Yeah, let me just see if we're actually doing that, so when namespace commit email already exists, no we're not we're not doing any permissions checks in the specs. In that context,.

C

B

Me I'm tempted to move these permissions. You want outside of that context, block.

C

Yeah because even if the namespace already exists, that permission check is going to happen. Yes, yes, okay, yeah, I, agree.

C

C

uh So this is basically everything that it's going to get moved out of the context.

C

And in here, when we basically make sure that it's removing or updating the the yeah, we definitely, we are definitely missing some. The the permission checks on these cases, yeah yeah.

B

So if I just move them outside the context, it sort of yeah.

C

Yeah, that makes sense. That's the end.

B

Texturize the heart bit right, yeah.

C

It's harder to write the tests that will make the changes.

A

I actually find it more harder to review too, because you're trying to like look at what's being tested, which is I, think we spend a lot of time reviewing the tests, which is good. Do.

B

You know what that's the one reason: um can you go to the service quickly, Marco.

C

Yes, the service, okay, so.

B

I think it's because this Mr is so: oh no, it's actually also because I canceled the pipeline, um but that's why I love our code coverage test coverage, visualization um I! Think that I know that. Doesn't you know it's not infallible? No right! It's! It's a really, really really good indication of whether we've got code coverage for all the different branches. So um I find that really helpful. Awesome uh we're already over time, Marco. Obviously you managed to uh survive longer. Then you must be starving.

C

uh Oh no, actually, we already have dinner. It's.

B

No way not in your country in Iowa, oh.

C

Yeah we are in the north, we don't have dinner, 11 p.m. Oh.

A

Yeah we ate dinner at like 5 p.m, local time so yeah.

B

Generally I'm gonna say any of us with younger kids, probably uh yeah.

A

It all started when I became pregnant. The first time I like rant back my thing, I'm, like I'm hungry at seven nope I'm hungry at six and I, want to go to bed at 6 15, so I need to like be eating at an earlier time.

C

um No way now we had dinner at.

C

Seven p.m: yeah.

A

I, don't know my kids, hopefully at least one of them is going to Venice. Yeah I can stop the recording now yeah.

D