GitLab UX Research, 8 Jun 2022

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: Toon reviewing an MR

Description

Toon demonstrates how he uses Merge Requests to review code. The focus is on reviewing commit-by-commit and attention requests.

Related issue: https://gitlab.com/gitlab-org/ux-research/-/issues/1944
Reviewed MR: https://gitlab.com/gitlab-org/gitaly/-/merge_requests/4604/

A

All right here we go, um I'm gonna review. uh I'm gonna show you how I review my how I do with my reviews. So let me explain what my workflow usually is. So usually I I start off with like merge requests requiring my attention, um so I have these merch requests, which are requiring my attention now this one I have seen quite a time ago. I know it's stalled for a reason, so I'm not looking at that one right now, um this one, uh these three are new.

A

Now, ideally, I think I would uh want these to be sorted by like when when did attention requests come in, but I think like by default, they're sorted by create dates. um Let me quickly look at because, like this one was updated 12 minutes ago.

A

Let me quickly open a new tab to see like when was my attention requested.

A

Okay, so like 12 minutes ago, my attention was requested. um This seems like 10, commits quite a big one, I'm not sure it's the best candidate for this, I was, might have been looking at this one or might be looking at this one. um This one has six commits. um It hasn't been reviewed yet by anyone uh who, whose attention is requested. So there might not be any comments from colleagues yet, okay. Well, one comment from will, so it seems- and this one is only a single commit.

A

So this is probably not a good candidate for this demonstration. So let me start off with uh this one, and then we can maybe uh look at the other one afterwards. If, if this one was not a good candidate for this demonstration, so when I review a merge quest, I look at the merge request description as first so we're about to convert user delete range to use structured errors. uh The first.

A

Okay, so this is like I'm reading like the description where what is it about um sometimes? I know this comes back in the commit messages also, so I usually just skim it a little bit because I know like it's great, to see the bigger picture of all the commits together, but sometimes you just have to look at the commits individually.

A

um As far as I'm concerned, there's nothing to see here. So I switch to the commits tab and then I I start looking at the bottom commit which is the first one for this merge quest. I find this a little bit awkward to um locate, locate the first commit this way, I'm not sure if we can approve the how we can improve the ux here um to make it easier like on the changes tab you get um like all the changes for all the commits combined, but I like to see them commit by commit.

A

So I click it and it opens, and then we have the we start by reading the commit description, proto document user delete branch, rpc, the user, delete branch, rpc is missing, documentation, add it to document its behavior and expected error cases all right. So I I like this. um I like that people add documentation.

A

So in this case uh there are a bunch of files I can ignore, like the this file is auto generated, but we committed so uh we committed to git, but we don't have to look at it directly because generated, and um there is a ci job making sure that it's it's correct. So I can collapse this one. I so can I this one not this one and also this one.

A

So these are the changes that are relevant for this commit and then I'm gonna read what's happening here: user delete branch force deletes a single branch in the context of a specific user. It executes hooks and calls contacts rails to verify that the user is indeed allowed to delete that branch. Okay, that's that's what this rpc! What this request does?

A

I have nothing to say about that. So, let's continue with next file, and so usually branch forces delete a single branch and account a specific user. It'll execute hooks else. It's following known error conditions might happen, return invalid argument in case there's: either the branch name or the user are not set. It returns failed precondition in case the branch does not exist.

A

Okay or with a pre-receive error in case the custom hooks refused to update returns, failed recognition in case updating the reference fails because of a concurrent right of to the same reference sure um I'm pretty confident that that actually actually is what's happening here. So, let's continue usually branches. The request for the user lead rpc yeah, it's just not much more. To say about that repository is a repository to delete the branch and branch name is the name of the branch that shall be deleted.

A

It's expected to be a branch name only eg in case you want to delete refset main. The request needs to only contain main, as the branch name user is the user, on whose behalf.

A

We should delete this branch, the information issues to perform access sense using the rails internal api. This user is also expected. Any custom hooks is a response for delete or pc. Pre-Receive error is the error. That's returned in case the deletion of the branch failed either because failing access chest or because hooks have refused to update all right, um yeah, there's not much but to say about this commit next one.

A

So I push the button to go to the next commit remove comments stating that we don't verify the branch name and or user in the user create branch. While we do in user delete branch, we do it in both. So this comment is clearly still.

A

So he claims that the user delete branch.

A

Like this um so yeah, it's true like it's weird that this comment is here, that we do the branch name and usually check here. First, only in user delete, but not in usually create it's intentional okay. So it's um it's weird that this comment is over here. I'm not sure I hope, like the the update branch, I hope the create branch.

A

Okay, I will not find the create branch method here, so I'm gonna open the file a new tab and then I should be able to see user. Create branch show. Yes, this check is here, and this check is here. So patrick is right. That comment is still so we can remove it now. I want to switch to the next commit, so I have to scroll to the top again to find these buttons and then I'm can push next operations.

A

Modernize error message returned by user delete branch, modernize error message to measure modern coding style more closely.

A

um Well, this is a good point, I must admit, I'm not 100 percent sure what the helper does, but it's absolutely true that we use this more often so yeah, it's just some formatting.

A

It's pretty obvious to review this. So yes, there's nothing to say about this. One next commit modernize test name to match or couldn't support. Okay, so we say test what we're testing and what kind of tests we want to do so, yes, um user, create branch transactions, choose range, create branch hook, start point hook, failure failure, success, hooks yeah; these are all all pretty straightforward, which is nice.

A

Next one improves test coverage for user delete branch to exercise failures caused by concurrent writes to the same reference and when the call to rails internal allowed interface fails for the access checks, all right, so um adding test coverage is also always nice. So what are we doing?

A

Test user delete branch allowed when it is allowed to the allowed function, returns true string and error. What is the string not sure yet, but we'll figure it out soon so not allowed is false, something something nil. I'm not sure what the error is uh expected response. Then we expect this pre-receive error here. We expect there's an empty response when, in case of an error, we inspect this and we expect this. Yes, um if I'm not mistaken, this gitlab keyword is pretty important, so it's really nice that patrick, is adding test coverage for this.

A

We run these tests. We set up an operation service without repo, because it's just mocking- and we have this function- that mocks the the internal allowed um api response, and so that's that's what this does? That's nice.

A

We create a repository.

A

We write commits to.

A

Branch not sure if we can't call a branch here, but it doesn't matter. I guess, then we say user delete branch we say require the grpc error for the expected error and we expect pro to equal for the response.

A

Okay, that's pretty straightforward. A concurrent update um test helper contexts, setup operation, concurrent updates, create a git update, ref process, that's locking the concurrent update branch.

A

We do not commit the update. Yet true, though we keep the reference lock to simulate concurrent rights all right, so we have an updater and we wanna. Yes, then we want updater to delete this branch and we prepare it. So at that point the commit uh reference is locked and we cancel it in the end.

A

Then we do a user's delete branch on the same branch name and then we get an error, could not update heads revs had concurrent update, please refresh and try again all right, and then we require nil for the response.

A

A

Is there anything I'm concerned to read this commit, I don't know, there's nothing in particular, I could say like.

A

The um well we could write this differently, but that's not per se better.

A

Okay, let's scroll to the top for the next uh for the next commit all right, so this is probably the most important commit where um we're chatting about. If you ask me like this one, usually I don't even read this one, so I just close it, but I'm like okay. This is something I I I'm familiar with I'll see it, because it's the same as this one like okay, you can expand all files, but I you also have this button here. So now, I'm thinking about it's like this is this is totally unnecessary.

A

In my opinion, all right, we're about to convert to user, delete branch to use structured errors as a first step, we introduce user delete branch error messages that host all errors and expected. Failures must be handled by the client. Rpc is not yet convert to use new error types because we first have to update the clients to handle them.

A

Well, um it's a little bit um the rpc okay. So now it would be nice if I could comment on on the commit, because it's a little ambiguous what what he means with rpc like.

A

We could uh what we should do here. So let me let me try to give a little bit of context. So um there's an rpc, an rpc, usually or in this case, has a request and uh and a response message and what this uh commit does is it changes those messages, but it doesn't change the handling of the rpc, and so, if you take rpc as the handling of messages with the request in the respond format, then then it's true, but actually it's more about.

A

um In my opinion, my opinion, rpc is, is a message. It's effective effectively like the message type, so um I would say like I would ask him to.

A

To rephrase that a little bit, so let me do that. What I usually do is um is just post a comment somewhere then um say something like um the commit message is a bit mv. Who was uh for me?

A

What do you think about, and then um I usually do it, something like it depends on. Like am I suggesting changes or am I like.

A

So what we do, um how shall I do it this time? Well, I will write it fully: we're about to convert to usually branch to use structured errors as a first step. We introduce user branch. Lead range error range error, message that hosts all errors expecting failures which may be just handled by the client.

A

Well, okay, I must be wrong here. Okay, my bad so um cancel.

A

I was mistaken, um it's actually what it is. It just adds the usually punch error and that's that's that's about it. Sorry, for the confusion. Anyhow, um there's an error returned by users, lead branch, rpc and some specific well defined error cases. So we have an access check error. We have a reference uh-huh.

A

If they're rpc filled because of updating reference, new object, health failed custom hook, custom hooks rolling apart all right, that's it.

A

So what I usually do is also switch to this page to see what are the, um what is the overall diff of the complete merch request and um okay.

A

Now, this one is back again, I'm it's screaming too loud for my day, so I I like to close it because I don't like to have it open uh uh screaming at me all the time now in this case, when I know there are files to ignore like like these, I should be ignoring, uh and these also I I tend to use the the the the tree view of the of the merch request.

A

I don't use it that often because I usually just scroll to all the files, but if I have to locate some files- or I just check which files I need, then I tend to do that. So these are like the tests. This is branches. So there's a lot of cleaning up done here. A lot of maintenance work done, but there's nothing uh nothing out of the ordinary that uh has been happening here. uh It seems that this didn't really work as well, as I hoped it would.

A

Yeah, that's that's the downside of um uh of these generated files. They they are lengthy and they get in the way. um Also like this checkbox, I never uses it. I never use it, which might be quite useful for this use, actually because it will make it easier for me to ignore these files, so maybe I should adopt use of them a little bit more.

A

So let me check them so they are viewed which, which is interesting. So if I would, um if I would go to this, commit back um okay, so the so the view checkbox is not persisted between the commit and the and the complete merge request view. That's right!

A

Okay! Didn't I check that one? Oh, this is this. One is checked. Okay, this one wasn't checked, that's nice! If I now go back to latest changes.

A

uh huh Well, that's probably because one file was touched in a later comment and that's why it wasn't checked because it wasn't viewed for that revision, which makes sense, and now these are now. I have these checked now these only these are bold. That's interesting, that's very useful and that's something I learned today.

A

So if I would now go to the other commit that changes the the cheaters auto generate files, then these are checked because I've checked them uh on the on the merge request and the revision of that file is the same on the last commit. This is done based on complete, merge requests, which makes sense um yeah.

A

The only thing with these collapsed files is like. Sometimes they don't really do what I expect like. um If I like to collapse this, then it opens. It's really weird to my. In my opinion, it's not what I expect, but looking at iconography. So let me refresh the page.

A

It says here that it is collapsed. I never looked at that. What the item was, I always assumed. If I would click this, this message would uh would be gone and it's it's. It's the opposite.

A

I'm not sure what is expected here, but this is what I wanted any case. All right. um I'm quickly want to have a look at um at batch reviews, so batch com comments.

A

So, for example, when I would add a comment over here, so let me say.

A

Let me add a comment somewhere.

A

Fill precondition, custom hooks, yeah, okay, it doesn't matter um writing a comment as a demonstration, so um I would start with you.

A

This is one thing I think that's weird with these with these comments, so you have this bottom bar, that's added now, which is not my favorite, but because it boxes in the main context too much for my days. So I like I like that this, like a div, is my main goal or my main uh objective. I'd like to that. That's that's important with this much request, so having too much clutter around is not great and having another bar. Here is um well, it's a little bit distracting if we can say like that.

A

um So when I will be reviewing this commit- and I I would be like reviewed and a scroll to the bottom, then I would my um natural, like response would be like okay now I can push this button or some button here to go to the next commit. That would be my uh what's the call again, I'm not sure um so this is not true. I have to go back up here. Just to go to the next commit commit yes commit.

A

It's sometimes using commit versus comment, so I can add, add another uh test comment here and um I always have to read this which which one of both I need, but I need to add it to review all right and yeah, so it I might be like adding comments on all the different commits and then when I'm uh at this page. So, as I have said like on any end, I like to review the whole uh the whole merge request and okay, my bar has disappeared, so it seems so it doesn't know about pending comments.

A

That's really unfortunate! I think if I would add another here like another one uh wait: what happened all right start with you? Oh it's a story review.

A

uh What's happening here.

A

So these are like my two original comments. ah Okay, I'm not sure what happened here, but let's ignore that happened all right, so I'm only seeing so when I scroll to do through this div, I'm only seeing.

A

The comment I've added on on this page. So if I have been adding a comment on a comment page, then it's not showing up here and vice versa, because uh where's the other comment so in the branches that go um this is like. Okay, let me go to that. Commit modernize error message. That's that one. I've added a comment to that div and like a pending comment is not here.

A

I can say okay now before. Okay, imagine I have these three comments added and I want to review what I've been saying like. Does it still make sense what I've been seeing then I would push these and then nothing happens.

A

Nothing happens, so it's! It seems to do nothing at all. So if I know which com commit, the comment is on like this one is on this page on the total div. Then this works. uh I know there are two there's a comment on the first commit, so I can pending commits writing a comment to demonstrate. So this works um one quality.

A

Also, here is, let imagine I've expanded this view because I was like well what's else in this file and like uh you should be updating this as well, which might might happen, uh and then I go okay. So now like, for example, I go to the next comment. I know okay, this one is here so uh test comment here, so that also works. But now, when I go back.

A

This one is here, but the one I've added above is not so you should update this as well. There's it doesn't expand the code where it where this comment was added. I have to manually, expand it and now it's visible and if I'm not mistaken, that happens on on the first for full request if that works as intended as you would expect. So, if I would go to branches- and I would say okay here- this is not on error- I don't know, add to review now. If I would again refresh the page.

A

So now, now that the command is not visible, but when it's appending comments comments, this is not an error. Okay, okay, neither this works. Well, that's good to know! I wasn't sure about that now, it's shown so that that's one thing with pending comments: that's not working so this like because I have to to for like I have to find where this comment was added. I have to manually go to this commit or manually browse all commits and then see. Okay was the comment edit here was here. Was it here?

A

So if I would say, okay, um this comment is: where is that? Okay, I'm on the right page and then only then I can delete it because I know okay, I've seen in later comments that it's not no longer relevant. Let me delete it because it doesn't make sense.

A

uh It gets clear in a later comment um so that that's the case like with all of these, like, I only can delete them one by one, if I know on which commit they are on like I know um this, I know the one about like this is not an error. I know okay, this. This doesn't make sense, no more, let's delete it.

A

I cannot do it without going to the comment to exactly the right commit, um so I'm trying to delete all the comments here, all the pending comments, because it was just for testing purpose and it's it's quite tedious if you have to do it like this like. If you can read this line and you know okay, I wrote this. I know it's no longer relevant. Let's move, let's delete it, then so for this one also, I have to open this again and to get it's deleted.

A

You should be updating this one as well. I'm not sure it was on one of these, but I'm not entirely sure so like like this is this is the pain all right? It was uh it's protooperation.proto. I think it was over here yeah.

A

So now my pending com comments are gone, which is what I want um so now we like sometimes just read the description again like make, doesn't make sense, it's clear enough what what and why it was doing this, then I sometimes also scroll through what did other people say about this merch quest?

A

Did it say something that I missed and um I should have um sort of seen or is it something that someone else saw and it can be sometimes just educational to um to learn about what what other people picked up and what they considered important pipelines? Failing error returned by cancer? Okay, yes!

A

Well, that's nice of a will to check the pipeline, but that's usually the responsibility of the of the author. um So what else do we need to do like danger? Is happy there's nothing here. The milestone is set, um other things are generally in order, so I would say, approve and um what it does and that's what I'm expecting a proof. Current merge quest your tensing resource moves, which is nice, um although it's not clear from the sidebar here that that happened, that's yeah! So I'm not sure.

A

If I what happens if I click it remove detention request. Okay, let me quickly and that's another thing like um the url in the address bar doesn't always seem to resemble which page I'm on. If I were to refresh this page or visit this page again, then I would go to the div scan. I know I want to see the overview page. I will have to do like this also that the commit id is added. This is quite weird, okay, so this is what I expected.

A

um There's another person reviewing this, um and I don't have any comments for the author, so I don't have to request their attention, um so I think I'm done with this merch request. I hope this was helpful.

A

A