GitLab Backend Pairing, 24 Feb 2023

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: BE Pairing - 20230224 - Discussing permission testing

Description

In this session we discuss testing permission changes. We also talk about a contribution that looks to adding an anchor to an email link.

00:00 - Permission discussions and brainstorm
36:01 - Review adding system note anchor to email

A

uh All right, uh thanks for hopping.

B

A

End pairing um Jesse has an issue sounds like figuring out how to how to make refactoring permissions more lovable from the developer experience.

C

So let me share you think I'll do my full desktop and then make this full screen. So it's big okay. So this is the issue. um I wrote it up, so there might be a missed context here, but we'll talk through it. So this is the summary. Sometimes we want to make permissions check changes in the gitlab code base. We don't want a behavioral change right.

C

We just want to refactor, and currently um we pretty much just rely on people being very familiar with how our permissions work and hoping we have really good tests covered when we do this, um which it can be challenging to do right, because we do permissions checks in web controllers. Api controllers views they're everywhere. So, if you're missing testing at any level, uh you could cause a regression which, in the world of permissions, is not ideal, because permissions are a key security feature.

C

B

I will say this is um very relevant to a maintainer review that I got recently. That I was like. Please explain to me. Help me understand so I can know this way because I don't know anything yeah I'm, like I, don't know what this permission check does: I'm, not sure what these two you're changing. You know, I I, remember reviewing them being like this thing is changing to this one. They don't seem related.

B

The names are like kind of the same, but like yeah, it was really made me a little nervous to prove and merge it. Yeah.

C

Yeah and like you know, as a maintainer, how could you possibly know right what every permission like you kind of are relying on the.

B

Author to know,.

C

What they're doing yeah.

B

To ask the questions to be like: please help me make sure that someone has vetted this. That knows what I mean besides you, because obviously like I'm assuming they're, making a change, they know the permissions, but I just want to, like you know, yeah I think I wanted another set of eyes that weren't just mine, yeah.

C

Absolutely and we have so like and then in our Docs and our rules and permissions dogs like I as a member of the off team heavily on this table, um but this table is human generated. So we don't have any guarantees that, like you know, we don't guarantees in terms of. We believe that our documentation is accurate, but um there are cases where this does not 100 reflect reality or there's an edge case that that is not reflected in this simple table, and so um like Terry was saying it's not always straightforward.

B

um And the other thing is I've never seen that table before. Oh.

C

It's huge it's so big, but.

B

Now I feel bad I'm, like I, probably should.

C

Not a lot and I I can't say that I learn a lot from it, because it's so much information and then there's also I, believe I think there are 23 footnotes now. um Yes, 23.23.

C

um they're, 23 footnotes on like and this and this. So it's a lot um oh and the other thing I was going to mention- is that, like there aren't necessarily permissions that are named like there's, not a permission called manage, terraform State, necessarily so. Mapping like the code Concepts and the like conceptual pieces is not a one-to-one. So it's like super complicated.

C

It's a big old mess, uh so uh I have some ideas for how to go about making this easier um and I can talk about two of them and and I also would love more ideas from the two of you um cool. So the first is like I mentioned earlier, like an R spec shared example.

C

So our permissions engine is declarative policy, which is a gem that we maintain and declarative policy basically takes in two objects: they'll take in like user and a project, and then you can call policy debug on a specific permission like read Pipeline, and it will tell you all of the step, the checks that it runs and the outcome enabled true or prevented, uh enabled true or prevented true. uh So in this case you could really go line by line and say like. Why was this enabled and you can like walk yourself through the code?

C

So we have this under the hood. So what we could do say we wanted to do a refactor and I. Think I have an example down here: um yeah, okay, so say we wanted to change. Read build to read pipeline, for whatever reason we're doing. A refactor like Terry is in an MR review and someone's doing this refactor, and we want to compare like the outcome of these two permissions checks.

C

um We could build something like this. This is this little proof of concept. Where you basically say what is the project, because this is a permission check on a project? So what is the project visibility? What is the role of the user and like is this allowed for this user um I only am using Anonymous user guest and reporter here you could also pass in developer or owner admin uh and then basically, this Compares and through each.

C

It runs each case in this table and says like yes, this would value for True both for read, build and read Pipeline, and if they were all the same, then you would get to the end like okay, well, these evaluate to the same in every case, so I feel comfortable switching instances of read, build to read, Pipeline and not causing any behavioral changes.

C

um So that was the initial idea. um The downside or like the problem with this that I see is just like. These are project visibility. Enroll are two factors. There are obviously a lot of other uh things to take into account with these permissions checks.

C

um Let me see if I even thought of any oh yeah, like I, I, didn't think of any specific edge cases here, but I know they're there. That's why our policy classes are so large because we're doing all these other checks on you know is these are blocked or so anyways. That's an initial idea. Any reactions, thoughts.

A

I I was looking at this. Do you kind of see this as being a test that is used to validate the change, but isn't necessarily checked in committed to master, or is this something that are these kind of tests that you would imagine to be committed to master.

C

um It could be either yeah like I, guess, I, don't really know. I I figured I could add this to some kind of a helper file, so folks could use it easily.

B

I was looking for one that you might be able to use I'm, not sure if you've got if you've run into it before, but there's like policy um um contexts that are written for group and projects um like we use these in um some of the elastic search tests. Were these, like these permission tables for the reporter feature, like the file that I link to you has um if.

A

You go back, it was the third one on the list is the project.

C

Okay, I'm looking at Terry's message. Let me just do it.

C

This is a nice.

B

Let me see if they're we're using those um we use them and I think other groups use them too, um where you can include a context and then kind of it runs.

C

Through all permutations.

B

And you can write like uh specs underneath those so either you could probably build off of this or at least make something similar to it. um This seems close to what you're looking for, but I think these are more around like the project feature access levels.

C

um Yeah, which what yeah I'm not sure what expected count means in this context. Count of what do you happen to know.

B

Yes, let me see you.

A

I guess maybe yeah I don't know.

B

I think hold on. Let me find an example of us using it yeah. So I think these are around like this search results, where, if you're looking for like how many things should should come.

A

B

Yeah, if you're like saying, okay, like we have this issue, and here's like the permission table for like the issues access level, uh then it kind of runs through all the permutations and says when this user has these like settings. This is how like it should get this record back.

A

C

ah Okay, so this is specific to search in a way in terms.

B

Of in a way at.

C

Least the expected count yeah.

B

I feel, like other I, see it used in other places, but not that's not necessarily search but like we. We use this. This is how um we make sure that the permissions in the database are replicated properly in the elastic search.

C

B

Because everything has to kind of any everything that permission wise has to be replicated so that you can't like look at things through the search and point um that the UI wouldn't show you but I wouldn't say necessarily add-on to these. But maybe you could utilize it in the same way.

B

C

B

Like a shared example or something that's like you feed it, something yeah.

C

Yeah because I feel like because another thing that has I've noticed that happens, just like in slack is we'll get support, questions where someone is like shouldn't an owner be able to do blah and you're like okay. Let.

B

Me go check that yeah. What.

C

Permission is checked and then you're like okay. This is the person check now. What can that permission like which roles by default? Have that permission, and that's not always like super easy to know by reading I mean, let's click on our home.

A

You know, sometimes you have.

B

A

Go through the commit history to understand why a.

B

A

Can or can't do something which is interesting, um a couple, a couple of more ideas for you uh Jesse. If you go back to the file that had all the table like the huge permutation of values and I, think you mentioned some sort of library that probably generates this kind of like parameterization of tests.

A

Did you mention something like scientist.

C

What it was called yeah, oh yeah. Well, so scientists is a different thing that I'd love to discuss. I think the idea was would be. We would use a shared example similar to this, that has like the grid, priests.

A

C

Out with you, customize that well.

A

I think but I think it's. You know it's a good point that um it's a good point, that, with different permissions, different factors are going to be at play and so you're going to kind of always want all the permutations of whatever factors you plug and play there so it'd be interesting to like have declared ahead of time.

A

These are the project visibility factors it has to be public internal private. These are the role factors like these are all the possible values for all the different factors, but then I declare as a developer. I just declare these are the only ones that would evaluate to yes, all the other ones evaluate to false or something so.

B

A

The way me as a developer, I get for free the everything's permutated yeah I get that for free because we'll forget cases for sure. Yes, all those get run, but I just have to declare just the ones that are on maybe or something like that.

C

Yeah well and in this particular case at least in this original Vision on the left. Actually you would never do any permutation like you, wouldn't do any permutations, because what you're doing is you're passing two different permissions in and what you want as an outcome.

B

C

Just to know if they're always evaluating the same value right so.

A

I have another idea for you regarding that, um but my my child is yelling for me, so I'll be okay.

B

A

B

We can sorry go ahead. Paul um I'm wondering if we can utilize like almost like a test helper like the sidekick. Has the item test thing that you kind of throw your tests into.

C

I, don't know that one do you know where that is.

B

B

uh It's I found it in the docs per month.

B

Declaring a workers are important.

B

Well, I thought that was in there.

C

Dare I search the word item button in there.

B

Yeah, it might be in somewhere ensuring a worker's item. Point uh I know they're the docs I'm, not sure where it lives in the code. It's somewhere. It looks.

C

Like there's a cop, this is an idempodent worker.

B

Yeah, if you there's it shows a shared example, called an item, potent worker and basically you kind of put that wrapper around your sidekick tests to make sure that it actually is that important when you run it twice and it this like nothing changes from one run to the other, like it doesn't matter, um okay, so let me see if I can find the file.

C

Yeah because I feel, like it's shared examples before or no.

B

How do I not how to find things? There's.

A

B

Lot of use of it, which means it hold on I, respect that shared examples. Okay item potency shared examples here uh that one thank you.

B

um It will be cool to have something like that: I'm, not like exactly how this works, but like it that you just you know, use that old policy and renew policy and then does the rest of this stuff for you. It.

C

Does so people use this whenever they add a new worker? They just use this shared example.

B

You're supposed to, when you add a new worker, that's it important. Okay, I, don't say all of them are like that. There's I, don't know if there's a cop now but before I wouldn't have mention it during Mr. Reviews like this is missing the test for it: okay, okay, I'm, not sure if there are workers but.

A

Regarding the the goal of testing, if this is a pure refactor, so we do have in our uh developer guide um something about pending tests. Have you ever used or looked into pinning tests? No, like uh I, think it's like pen, like you, pin to something? Yes? Yes, yes, yes, painting tests yeah. So this is. These are tests that, if you're doing a pure refactor, it's tests suggests to test the refactor that you don't commit to master.

A

So you, the first commit of your change, will be we're going to add this pinning test and it's going to be like a snapshot of the entire HTML page or like a snapshot of function, calls it's like as low level as you want to get just to prove. This was a pure refactor, so you have the pinning test.

A

You make the change and then you can revert the pinning tests but to just test, and then you revert moving the pinning test, because it's not the kind of thing you'd want to check into Master, because it's not really helpful to us, but you just end up reverting the um the reverting whatever remove the pinning topic and running it to just to assert. Oh yeah, this was a peer refactor.

A

um We have some like examples of Mrs. That kind of did this because we were from the front end perspective. We were converting Hamel pages to view, so this works great because it would just take a huge HTML snapshot of the handle page and then a huge HTML snapshot of the few page and we'll compare the two um okay and.

C

So this was you look uh using Pennington yeah, you see like keep those commits as well right.

A

And they're not emergency right. So then we just remove it right after that, and so whenever we want to run it just revert whatever removed it or we could share it whatever you want to do it, but you showed that debug command and that's really interesting like and so that could actually be used as a pinning test of let's debug this policy and dips it with the debug of the other policy, and if the diffs are the same, then this was a.

C

It's the same yeah yeah and that's sorry. No.

A

I was just gonna say the it's a weird test, because it's not the kind of like helpful test that you'd actually want to commit. It's just a test to prove to you. Nothing changed with the execution flow of, or whatever is the expected very specific output here, yeah.

C

No, but it's really helpful because it reminds me of how sometimes, if my Spidey Sense tells me that some, like I'm, reviewing an MR and there are tests added but I'm like I, don't know if these are really testing the change. I'll check out the Mr and comment out the code changes and see if the tests still pass and if they do I'm like oh, these aren't these.

A

Aren't testing what you.

C

Change and like almost it would almost like in an ideal world would be cool if people had to First do a cover commit with all the tests you can see that they fail. Then you see the next commitment.

A

With the code changes to.

C

Fix it like that, would save me some review time. No.

A

That is, that is, uh that is related a little bit as well, um so I think what would be cool, namely just thinking of we find ourselves needing to refactor policies. Maybe there is a script that could introduce the kind of tests we just wouldn't check in, but it's the test that just gives some Assurance. This was a peer refactor and a debug statement is like yeah. That's that's that can traditionally be used in like pinning tests in the back end.

A

It's interesting.

C

Okay, that's helpful yeah, because there is. The second um thing is I did add an issue to the declarative policy. Gem itself, saying um kind of on the same topic: can we compare these policies and then our friend Peter, very helpfully did a little proof of concept to show how we could do that.

C

um This is a bit more granular.

C

um Then the the thing I showed you before was just what does it evaluate to true or false, which again there could be a lot of edge cases in the logic that we're not accounting for, whereas This truly does a side by side on, like every single check, that's done for each of the permissions.

A

Yeah, that's really cool I mean that's a great that's a great output from as like a code reviewer. If I'm wanting to know some of the superior factor, I was like. Oh there's, no diff, that's pretty good confidence, and if there is a diff, it's like okay. This is the kind of thing you need to look at. This is this. Is.

C

A

Helpful I think.

C

Yeah yeah and again like if you're always running this with a specific user and a specific vehicle in this example, so um you have to take into account. There are different kinds of users or different kinds of vehicles, but if we it's like kind of combining those two concepts together, you could have.

A

Yeah yeah and that's what we did in some of those other pinning test. Examples was like okay: let's do this with feature flags on off, so we create like a whole bunch of snapshots and there's. Usually, though, some sort of like in a perfect world, everything would match, and so so then there's also a set of like we can clean up this. These, like white space, kind of noise or whatever so like then there's sometimes stuff. You can't get it towards perfectly green, but uh it's perfect. It's it's!

A

Since it's not committed it's just code to help you develop it's it's interesting, but.

C

um These are good.

A

Ideas, I love. The idea of you know some automatic or permutation generator thing sounds super helpful uh and then this this diffing I didn't realize this debug thing was a thing that sounds really helpful on a number of situations.

C

And you mentioned plot one point: scientist I was curious to ask if either of you had to use scientists the thing that I thought was really funny. Excuse me um was uh the example they give is you're trying to change what you handle permissions in a large web. App I'm, like oh I, think this library was developed when somebody had the exact same problem that I have.

C

um We were using this at my last job. It's um it's more of the tested and prod mentality right. You actually push the change to production and then it runs both code paths and if you only use like the old code path, but if there's ever a difference in what the old and the new evaluate to. uh In that case, we were just like. We were like throwing an error, um because we had pretty high confidence with the refactor.

C

If you don't I, maybe log or something um have either of you used this or thought about it.

A

B

I said: I haven't used that I know we talked about it a little bit in a one-on-one chat and it sounds. You know it sounds so interesting um to me because there's so much code in our specs that are like turning this feature flag on turn. This feature flag off like it's yeah.

A

This would really change this. This touches to that whole process of the feature flag, stuff. That's a I'll be curious to know. If something like this, how closely it relates to our get lab experiments.

B

A

Seems really similar but I I wonder if this one's maybe more robust or.

C

um So yeah this came up in another issue where somebody was asking about using scientists for something else. um Thanks Charlie um uh and somebody mentioned. Oh, we have this thing called: get love, experiment and apparently GitHub experiment used to have scientists as a dependency so like they do similar stuff, I. Think uh the difference now I'm, forgetting I, think the difference with gilab experiment is that it's actually for like turning things on like there's, not a focus on.

C

Do these two code paths evaluate to the same thing like scientists is truly, does path, a and path. B result in the same thing, and you want it to like, because you're trying to do a refactor. So the logic is the same, whereas gilab experiment is um here, there's like a control and then different variance and the ideas that you're like using all three.

A

C

A

The B are I, think and intentional variable names there, because I know we. This is our way of doing like a b testing to see what performs better out in the wild, not necessarily what's acceptable and what can be rejected. It's just a measurement of I, don't know for some reason. We measure some sort of data that comes out of these experiments. Yeah.

B

I feel like there's a completely different end game for the second one. The git love experiment in the future, like those are two things where the end game is, there should be something different that happens like there's, no guarantee that both code paths should result. In the same end, so I feel like that's like the huge difference, experiment or science. The scientist stuff seems like it's more so for like confidence in your code, refactors.

C

Yeah yeah I think that's right. um It's.

B

A shame that it got removed.

C

B

I guess I understand, there's like some. uh It might be confusing yeah.

C

um But it is yeah like get. A lot of experiment is truly uh like you were saying it's for running a b tests of like here's, a cool, different thing: let's try it for 10 of users and what we want. We don't want to do a cool different thing with permissions refactor, as we want the boring old thing with a new name.

A

Do you think, um do you think it's possible to for all of our permissions to.

A

To quantify all of the factors that go into it, or do you think that our permissions are too I, don't know what the for, like a better word like imperative and just have some like spaghetti code in it that you just can't.

A

Does that make sense? The question.

C

Yeah, it does I I, honestly I'm, not sure yet. I have been thinking about that um a lot, because what I've observed just like anecdotally is that sometimes people add new permissions, because they just don't want to bother understanding how old permissions work.

A

C

So they're like well, we already have something called download vulnerability, but I'm, adding a CSV export, so I'm going to create something called download, vulnerability, CSV and use that, because, like that's I, can understand new things better than I can original things and and I think those cases will be easy to condense, because when you look at them, they'll just be two things that do the same uh stuff. I think the other like project policy.

C

I was just looking at this the other day and like this is an example of like we have all this logic related to like deploy tokens like. We have users that aren't really users um yeah.

A

C

That's that's more of an edge case or the user was revoked or.

A

um I know, even in here we even have some feature: flag checks and oh yeah, those are I think always I'm always not sure is like. Does this belong here? Maybe I guess you can't do it at the features that exhaust.

C

I mean we all. We have also quite a few feature flags that just like shouldn't be checked anymore, like they've, been rolled out to 100 for quite some time, and we just haven't cleaned them up. So hopefully my.

A

My daughter's calling for me again I'm sorry about that I'll be right. Back, okay,.

C

B

C

Yeah so anyways I don't really know. This is some of those situations where I'm, like I hope it all works out with this big refactor was like yeah start somewhere before you know.

B

Yeah I mean I think just starting with like I, don't know, I feel like what I've done and I'm Paul's experience with. This is just like, adding something using it and then putting it in the docs and throwing it in as many channels as you can to like get other people aware of it, and your thing may change, or it may might like, grow or be a little bit different.

B

But it's like hard for me to get my head wrapped around like how do you get people to adopt a way of doing things in the code? The only way I know of is to use them like pretty much exclusively yourself and then also trying to like shout as loudly as possible in all of the different spots that other developers might look for, like coding examples of how do I do this yeah.

C

um Yeah and hopefully enough, it seems like enough teams are doing permissions, changes that there could be plenty of potential customers for this yeah.

B

Yeah I would have been very happy to see this and there's actually a few like permission, Mrs that I had to review where I'm like uh yeah.

B

um Because those you know the it feels like very high pressure. The security like a security Mr.

B

Those are expensive, I feel like those are expensive on developer time and sometimes they're tied to like bug bounties, which are expensive and like money.

C

B

Yeah I definitely can see the need for something like this. Okay.

C

Cool well, when I get something up which hopefully will be soon because I'm gonna try to just like get something out there, I'll uh I'll, let you know- and let me know what you think of my first first dab at it, um but anyways I. Can we can move on to something else? They've got a lot of great ideas here, so thank you you both. um Can they learned about pinning.

B

C

B

Another like I, just realized there are like hidden, that's one of the Hidden documentation pages. It is there's no left bar yeah. One of mine is like that and I'm trying to change it because I was like. Why is there no left bar like.

C

Oh yeah, you have to add it separately.

B

It makes it yeah it makes it hard to get it discovered.

A

The secret menu to your.

B

Door, yeah, yeah I'm sure it's probably uh there's a page under like development resources that has a huge list of things, because the one that I've noticed from my team- that's like not anywhere, but there it's listed there. It's just it's hard to find. If you don't already know what you're looking for I would have never found that pinning testing you either have to like somehow find it on the one page where it's linked or know to search or.

A

Something I'm surprised, um I think because it's old uh I.

B

A

When we made the Mr to add it, which is why I brought it up um but I imagine maybe it lost its reference. So now it's just hasn't.

C

A

Like that, it's it's there's nothing pointing to it. We.

C

Have the the left nav, you have to add, via the www repo I recently, did that for a new API docs page. So it's a little it's hard to remember to do Mrs to two repos for one docs, Edition I.

A

Don't know if I don't know if it's always been like that, but maybe it has I felt like though we had something also pointing to this guide as well, but I I. Don't so long ago, I, don't remember um yeah the pinning test idea. I read about in a really really great book called uh um working effectively with Legacy code, which.

C

A

Co-Worker gave to me when I got a job at a defense contractor, because he said this is what you're going to be dealing with um or.

B

Like working in Banks, I.

A

Know but it was, uh it was a fantastic book because it's like all about writing test harnesses and weird code bases, those really cool, um yeah well I'm game, for uh we can. We can try to code some sort of spec helper thing based on what we were just talking about, or we can jump on Lee Lee's issue that they mentioned.

B

That's up to you maybe.

A

We should maybe we should, uh since Lee went so far to bring something up. Maybe we shouldn't completely ignore his pleas for that's him.

B

I looked at it, but the yeah I think um I can share the Mr.

A

I can drive, or, if you want to, if you want to drive here.

B

Would you mind the uh I'm sorry, would you mind driving, can.

A

You send me the link to it. Yeah.

B

It's a link to the original Mr that um he shared in Slack yeah. Sorry I've got a tab situation on my computer.

B

You only have two tabs open so.

A

Yeah wow wow. All right did you wait? It's not here. You put it on Zoom chat.

B

I put on Zoom chat, sorry yeah.

A

I, don't potent I'm not even gonna, pretend to ever know what that word means like them.

A

B

Guess um I don't remember either but well. I know it means like it runs twice and nothing changes more than once. That's what I have in my head. You can run in as many you know. You can run it as many times as you want and it shouldn't like I, don't think I.

A

I can kind of get the word potence in there, like. Oh.

B

A

I have no idea what the items.

C

A

C

No Latin Majors here.

B

um Okay class of school.

A

Oh yes, this is Winnie crate, yeah Winnie used to be on our front end team long time ago, um so Lee he just paying due to.

B

uh He asked if we had any um ideas: I guess, let's like what the idea is, is like I think there was a some comment about the email there's like no anchor links and notification, emails for system notes um and.

B

uh They wanted to like add that, but there was some issue about the note. Wasn't like a system note. Maybe it had not wasn't created like at the time like it's not consistently created by the time whatever.

B

So it does not include the right note when closing or reopening an issue and I, don't know why um or milestones so. It seems like wherever this change was made.

A

Setup issue: mail on.

B

Yeah I have no idea how email works, I.

A

B

um Emails after having experience in their previous jobs, but um yeah, it seems like maybe I- was hoping that someone that might have a little bit more information like I, haven't delved into how notes festivals are created. I.

A

um I have very I'm like we're, probably on the same amount of you know, exposure to this, but I um there's only one way to figure it out like we're again.

C

Well, I was gonna, say: I I feel like what happened here or like so we don't have access to the note itself in the mailer method, so the mailer method has the issue and the recipient, but it doesn't it's not currently aware of the system. No, so what Winnie did, which is clever, is get all the system notes find the last one and just assume: that's it um I think we I think we can build a system with more confidence. I think we need to pass the system note in when this email is sent.

C

So we need to look at what calls set up issue mail, yeah.

B

C

And see if we can pass this.

A

The this sign up, yes, add anchors to links and notifications for system notes and.

C

A

Wrap around the code.

C

Base and see where that is called, let's.

A

See that um just reviewing code yeah.

B

I know this is also something you're gonna, maybe be interested in or not Paul, but like I've made all these changes in all the stuff. So there's another issue that he opens about there not being enough testing around the specific area, because clearly many.

A

B

Yeah, nothing failed.

A

Well, yeah I mean it's email all right, so we call this one.

C

It's a private method.

A

So we call this, for an issue is cloned for his moves. All.

B

The guys yeah this is as far as I got I kind of looked at where it was called and was like. Oh that's, a lot of places and then.

A

uh I see so we were, um the goal was right, he's wanting to get it, bring it to the bottom. Note like no matter what, let's just bring it whatever it was. Let's just bring it to the bottom note, because because you click on it, you don't really see what happened.

B

But I think there must be some disconnect in terms of um yeah I. Don't know he was like it doesn't work consistently that the note that it's linked, I, guess whatever this is doing- is not always consistently lacking like linking to the correct note. Maybe when you change your Milestone or reassign an issue or.

A

View it on you're saying view it on gitlab.

B

Yeah the comment that they made was.

B

This does not include the write note when closing or reopening an issue and I don't know why. Actually, it also doesn't work reliably for milestones.

A

B

So it looks like maybe this email thing is sent out before the note is fully.

B

A

And yeah I would.

B

That's where I was like I'm, not sure I've run into a lot of, like notes, are a little confusing because there's right um there's a lot of different kinds of notes.

C

A

Anchors to links and notification for system notes, so this is this is the Crux of the issue. I think is really interesting, is and I would I would say we kind of want to Loop in ux. Here.

B

A

Figure out what do we anticipate? The user really wants to happen when they click on this email link.

A

um We reassigned the issue if I click view it on gitlab. Are we really anticipating going to the system? Note that says something get reassigned or is just going to the issue: sufficient um I'm really not sure uh so. I I, I I was kind of hoping, like maybe there's a pre-existing issue here, so we can get more context of what's the actual problem, we're trying to solve.

B

A

Because it is then really interesting that this link reassigned issue 41 and then this one at the bottom or one has the anchor one doesn't yeah.

C

Which one does oh before, that's the after I mean I would I agree with bringing in U.S.

B

C

Do think there are some cases where it seems like. We definitely wouldn't want to link to the note. For example, your Mr was merged like you, don't really need the system note when it was merged right, like that's, not very important interesting, but there is also like new mention in issue.

C

um I guess that actually does linked already to the issue thread.

A

Yeah, uh let me see what what else? Maybe this target URL? It's only ever used there, this project issue Euro the the view it on git lab review it on git lab thing is gonna, be that's where this thing comes from the target. Url.

A

So this is like our notify. This is our email layout, which is gonna always have that thing, so we're just populating that.

B

I guess um what what team is it like? What team is the owner the dri for emails, or is that one of the like, like kind of like lilac areas, if we're not sure.

A

I think it's the latter, but let's do a quick search. I actually think it's has to I think in a strange way. Modification.

B

Project management group you found it.

A

um So but I mean it's email is such a shared thing that it's kind of I think technically it belongs to plan, but I don't know so I'd be really curious. If they'll get plans, um ux opinion on. Do we want to link to the system note here, um but with that I, don't I, think I. Think the I think what you said. Jesse is like yeah, sorting it and then getting. The last note isn't going to be reliable because these run off of like some sort of sidekick job. It's not these emails aren't sent.

A

Sometimes they they build up and more notes are going to be added into it and other changes or whatever. So if we've done one reassign and then I add a new note or whatever this.

A

I know that these aren't these don't happen. Synchronously, as the note was created, you know what I mean.

C

B

Definitely new.

A

Notes can be added, as we've done the thing.

C

A

Am concerned about the.

A

C

A

Condition, we've kind of are considering.

C

Yes, also a Counterpoint to what I previously said, just looking at the screenshots in the Mr I now realize that, like the issue is linked in the body, but this the anchor link was just added in the footer, so it's not changing the main body of the email like I guess. I can see the value of being taken like having the option to see the note system. Note um assuming it was the correct one. Yeah.

B

I mean I feel like I wonder if this was a suggested um when you're looking at like comments, that is the behavior, uh meaning that the viewing on gitlab link it links to the to the note itself.

A

B

C

Yeah but I'm not.

B

Sure, like is there a call to action that you can do with a system node, so I feel like a discussion comment where someone's like sent you something there. There is like a CTA. You click on the view in a git lab, and then you respond to them.

C

Yeah yeah, with most of them I'm just looking at a random Mr to see what the system notes are and it's like label added label removed. It would merge.

A

I'm gonna I'm gonna respond um saying that um I think there's more to be concerned about than just waiting for persistence.

A

um We also aren't guaranteed that the relevant note is the most recent. Why.

B

Yeah things happen quickly, right, there's like bots of stuff that are like doing things in the background.

A

Right, the email jobs run asynchronously and can get backed up.

A

B

A

Notes are added.

A

A

B

um In between, like I guess like queuing and like, and the notification being sent, yeah.

A

Which means assistants are added between uh yeah, we'll lose the one we're interested in brainstorm.

A

Think the most reliable, reliable way to do this would be to um would be to include the system. Note ID in the email job that it's created by whatever service is, you know creating email, jobs and system notes right is that kind.

B

A

I, don't know what that sounds.

C

B

C

I was also wondering I have this speaking suspicion that system knowledge might be created in background jobs too, so maybe like when action happens like we encue a job for the no, and we can keep a job for the email, and that might be why it's not persisted, sometimes by.

A

The time the mail.

B

Comes in well, that's.

A

Interesting all right, brainstorm too.

B

Yeah I'm not sure you know I, like you do like quick actions or something that you know. Maybe like reassign quick action, you get an email, yeah.

A

Themselves are created by a an a synchronous job, so we don't even have a.

A

We don't even have I, don't know. What's the word I'm thinking of anything to go of sending email, yeah, I, think so I'm.

C

Looking I'm just looking to see if I can confirm this I.

A

Think you're I think you're right and you mentioned quick actions. Terry I think that I think that um makes sense especially doing like I don't know. Do we ever do sister notes for like we ran a pipeline or something yeah I, don't.

B

Know I think sometimes well, no I don't know is a bot episode. No there's like something: that's like hey. You approve this thing, I'm doing something. Yeah.

A

Let's, let's see where requesting you from.

C

I just linked a code for where handle assignee is handled, changed assignee change. If that's could be a good example too. In the zoom chat.

A

Create yes check this out change, issueable reviewers! We create a node. How interesting, okay so check this out change. Issueable reviewers system notes, issueable service change, issueable reviewers, some sort of Base Service. We create a reviewer node.

A

B

Multiple I looked at this and it's like oh.

A

Yeah yeah check this out, handle reviewers change, create reviewer note: oh.

B

A

Then here's the notification service, yeah yeah yeah. So this is that, like whatever just created the notes, we would want to pass that ID onto the notification service.

A

You know what I mean.

B

And that looks like very similar to where you just like. Let's see that's like create.

C

A

Yeah, basically,.

B

Like the same pattern, yeah.

A

Base Service scenes.

C

Yes system note service is what creates the notes like I was trying to find that I. Just don't know if there is app services.

C

Well Services on Services yep.

A

If you don't, if you don't need something controlled and it's a service, I think I think that's the that's the design patterns we employ.

A

All right, yeah, that's interesting.

B

Well, thanks for the good pairing session.

A

Yeah, that was fun, um yeah kind of got the brainstorm on some stuff and then got to take care of Lee's thing almost to not to satisfactory conclusion, but to complete this is the most. This is the best we could do. Okay,.

B

It's like 25 minutes I feel like we did a good bit so yeah.

A

Cool, well thanks appreciate it thanks for hopping on Jesse and Terry I'll catch you all later. Thank you. Thanks for your help, bye.