Signatures: treat dev_block as out-of-date CR 💥

[danielbeardsley]

When you "dev_block" something, have that inject a "synthetic" but out-of-date CR signature in the UI. This means that a dev_block somewhat reserves a place for you to CR it in the future.

Most dev_blocks come after a review and we generally presume that the dev blocker will come back and CR it after the requested changes have been made. Therefore, we now show a dev_block as an "inactive" CR.

Also, move the typescript comment to the line it's referring to.

CC @andyg0808 (issue author)

Closes #193

[andyg0808]

CR 👍
Looks like a reasonable approach.

[erinemay]

#193 (comment)

This also avoids looking at pulls which have another dev already involved in CRing if we don't want to.

Do y'all often see the previous CR marks on Pulldasher and skip checking out a pull under the assumption that someone else will come back and re-cr it? QA dev_blocks a lot. Is this gonna result in pulls waiting for CR longer?

[jordycosta]

QA 🟢

Adding the dev_block signature will mark a CR bubble as "inactive" (the red/yellow check-mark) 👍

---

Should this signature now invalidate an already valid CR stamp?

The current behavior is that this will only mark an "inactive" stamp if it is already inactive or doesn't have any CR

[erinemay]

Should this signature now invalidate an already valid CR stamp?

That's a good point.
A not uncommon occurrence is CR -> dev_block -> un_dev_block without changes. @jordycosta, is this the case you're thinking of?

[jordycosta]

No CR -> dev_block -> un_dev_block

Note: If there is already an inactive CR stamp, using the dev_block signature will overwrite that one as the new inactive CR stamp until it is un_dev_block'ed (and in return going back to the previous inactive CR)

---

@erinemay Yes, that is the case CR (active) -> dev_block -> un_dev_block

Throughout testing this, the signature won't invalidate the active CR (which I think is what we would like). I just wanted to double-check that this is the correct behavior 😄

[danielbeardsley]

Throughout testing this, the signature won't invalidate the active CR (which I think is what we would like).

Correct. This isn't intended to invalidate anything, a dev block already prevents things from being fully green while it's active. If a blocked pull is unblocked with no additional commits, the original CRs are still valid.

[erinemay]

Throughout testing this, the signature won't invalidate the active CR (which I think is what we would like). I just wanted to double-check that this is the correct behavior 😄

I think that is what we want.

Person A has cr'd a pull, so CR is active.
Person B comes through and dev_blocks on what they think is a bug or an issue.
Anyone un_dev_blocks it because it's not a bug, or it's not from the pull, or just erroneous.

In this case, I think the original CR should still be valid.

[danielbeardsley]

Do y'all often see the previous CR marks on Pulldasher and skip checking out a pull under the assumption that someone else will come back and re-cr it? -- @erinemay

Yes, 100%. Sometimes I will peek at who the CR was in case I know they'll be unavailable to finish up the CR (vacation and such). I see. Your point is that a dev_block can also be done by QA in which case marking it as an out-of-date CR seems a bit.... odd.

I'm not around today, but I woudn't mind other's feedback here about how to represent it. Maybe a dev-block should look like a green CR cause the dev-blocker's work is done until there's been more commits. But, that would make even less sense for QA.

Maybe instead we should get in the habit of marking dev_block + CR at the same time? I've always disliked the idea of the confusing message: "CR: I approve this" and "dev_block: it's not done".

[davidrans]

deploy_block ✋ so this doesn't go out without more discussion

[davidrans]

What about when QA dev_blocks something?

[andyg0808]

There's an ugly solution: new dev_block_cr and dev_block_qa tags. That would also allow QA to mark a thing as "we tried, but it broke before we could do anything."

@erinemay, does QA tend to have the same person re-QA a pull, or does it change? If the former, QA might also like this feature, if we could make it reflect helpful state for them as well.

One potentially interesting idea might be to show the pull as having out-of-date QA and CR both, then if the user who left the dev_block marks it as un_dev_block, CR, or QA, drop the out-of-date marker from both sides (regardless of which they do). That would allow both devs and QA to use the feature, and it would avoid leaving red dots in the wrong column once the user who dev_blocked it has seen the update.

[erinemay]

@erinemay, does QA tend to have the same person re-QA a pull, or does it change? If the former, QA might also like this feature, if we could make it reflect helpful state for them as well.

The answer is both. Some pulls are much more practical for us to re-qa because we're already familiar. With others, we specifically want someone else to take a look for the value of a new set of eyes.

[danielbeardsley]

One potentially interesting idea might be to show the pull as having out-of-date QA and CR both, then if the user who left the dev_block marks it as un_dev_block, CR, or QA, drop the out-of-date marker from both sides (regardless of which they do).

That's really easy to implement. Effectively add this same kind of code to the QA signatures calculation. This seems reasonable as dev-blocked pulls already don't show up in the QA column.

[andyg0808]

@danielbeardsley The one wrinkle that had occurred to me is that we probably wouldn't want the pull to show up as having an out-of-date QA to a dev who dev_blocked it and then CRed it afterward. In other words, we don't know if a person dev_blocking a pull would be expected to CR or QA once it's fixed, but once they do one of those, we almost certainly don't want to pester them for the other one.

[danielbeardsley]

we don't know if a person dev_blocking a pull would be expected to CR or QA once it's fixed, but once they do one of those, we almost certainly don't want to pester them for the other one.

I don't think that applies. This change only renders active dev_block sigs as an out of date CR. It doesn't create any. Thus as soon as it's un-dev-blocked the extra out-of-date CR/QA will disappear. I would gather that most dev-blockers block, then wait for changes, then CR and unblock.

That said, If I dev-block something, I don't want it to look to me like there's still something I need to do... otherwise it won't look different once the author pushes changes (though It'll be in a different column).

[sterlinghirsh]

CR ⚡ dev_block ⚡ IMO the existing system is fine because CR ⚡ dev_block ⚡ completely encapsulates what I want to convey. I even have a saved quick reply for it:

The conflict of CR meaning approved and dev_block meaning changes required is in your head. CR by itself means approved, CR + dev_block means reviewed with changes or explanation required.

[danielbeardsley]

The conflict of CR meaning approved and dev_block meaning changes required is in your head

You're right about the conflict of meaning. But the reason I don't do both is cause it's easy for a pull author to respond with "I don't think your concerns are valid, un_dev_block" and then the pull is green (cause I stamped it as CR) and can be deployed. We typically lean on authors to un block when they've addressed the concerns. I use dev block as "Pause on deploying this until we find a way to address these concerns (commits or no).

the existing system is fine because CR zap dev_block zap completely encapsulates what I want to convey.

I guess not me, cause I want to read the pull authors responses and evaluate before putting my stamp on it.

[davidrans]

it's easy for a pull author to respond with "I don't think your concerns are valid, un_dev_block"

I think that's more or less how things are supposed to work. Obviously, there are exceptions where the reviewer knows better, but I think we ought to err in the direction of giving the author autonomy in the review process

[davidrans]

This seems like basically exactly what we want:

Teaching pulldasher how reviews work (#88) would, I think, solve this problem as well as #239

[danielbeardsley]

I think that's more or less how things are supposed to work. ... but I think we ought to err in the direction of giving the author autonomy in the review process

While I like that sentiment, practically, this makes dev_block nearly the same as deploy_block. I generally use dev block to mean "I don't approve (or perhaps understand) this yet and I don't want to stamp it until we go back and forth". If I just have some suggestions that I don't mind the author ignoring and deploying anyway then I won't dev block.

Writing that out I sure feel like a control-freak. Hmm.. Given that this change is just a rendering option and doesn't affect when a pull is actually green, maybe I should just turn it into a UI option.

Teaching pulldasher how reviews work would, I think, solve this problem...

I think that's a useful feature, and I think we should do it. It doesn't affect how we render dev blocks and CRs in pulldasher, but it does make Sterling's workflow impossible: you aren't allowed to simultaneously approve and request changes in a review, it's one or the other.

[davidrans]

you aren't allowed to simultaneously approve and request changes in a review

But isn't the only reason to approve and request changes at the same time to basically hold your place as a reviewer? Which is what would happen if you have a pending review (pending requested changes)

[davidrans]

this makes dev_block nearly the same as deploy_block

deploy_block: go ahead and keep CRing, QAing, I just have a question/concern I'd like to see an answer to before this goes out, or this needs approval from some third party

dev_block: this is busted, let's take this out of the CR/QA queue till the dev gets a chance to rework this

[andyg0808]

@danielbeardsley I make the same differentiation. @sterlinghirsh, are deploy_block and dev_block synonymous to you, or do you distinguish them somehow?

I tend to differentiate them by using dev_block when I think the code is wrong and needs to be changed to be correct, while I use deploy_block when I have a suggestion or idea that might make the code better or when I'm not certain it's wrong. When I deploy_block I'm fine with the author un_deploy_blocking and merging, while when I dev_block, I generally expect the author to explain/convince me that they're right if they're not going to change it.

Re. my earlier point about dev_block showing up as an out-of-date CR and QA:
@danielbeardsley I don't generally un_dev_block as the reviewer; I wait for the author to un_dev_block, then re-review the pull. That's the only time I want to see the red dot: in the CR column after it's un_dev_blocked. Otherwise, as far as I'm concerned, it's still dev_blocked, which I generally take as "not ready for review."

[danielbeardsley]

Which is what would happen if you have a pending review (pending requested changes)

Since the feature hasn't been implemented it's up to us to make it act how we'd expect. You're saying that a "Request Changes" github review ought to be treated as an out-of-date CR? Or some new state / representation?

That's the only time I want to see the red dot: in the CR column after it's un_dev_blocked

Maybe we need a way to represent the state for "reserve a CR space, but don't make it look like the reviewer needs to take action".

[sterlinghirsh]

are deploy_block and dev_block synonymous to you, or do you distinguish them somehow?

I almost always use dev_block, and to me it should prevent further CR or QA until the issue is resolved. Often, that's a request for changes, though sometimes it's a question that I want answered before proceeding. Like "Are you sure this won't break the cache in prod?" If the response is "yes I'm sure because I diggity doubled checked" then I'm fine with them unblocking it without re-review. If the response is "great job finding my mistake, I will fix it and then unblock" then it will need re-review anyway.

On the other hand, deploy_block is more for when CR and QA can continue, but I want to time the deploy of something. e.g. "This should go out Tuesday morning" or "Only merge this after the backend change from pull XYZ is deployed".

[danielbeardsley]

This change only affects pulls in the Dev Blocked column and it'll make them look like they have an out-of-date CR (effectively "holding a place"). This doesn't change anything about our process or requirements, it only gives a clue to other reviewers that there's already somebody reviewing this. I'm inclined to alter this to render dev blocks as both out of date CRs and QAs so it'll also make sense for QA.

Despite there being no actual limitations on double stamping (CR + dev_block as Sterling uses) I still choose not to do that as I think it goes against the spirit (CR == I approve) and puts the decision into the pull authors hands.

The problem is that a pull in the dev blocked column like below appears that no CR work has been done and is inviting others to come CR it.

[danielbeardsley]

I've pushed a commit that treats dev blocks as out-of-date CRs and QAs

[davidrans]

This change only affects pulls in the Dev Blocked column

Are reviewers even looking in the dev blocked column?

[sterlinghirsh]

I occasionally check Dev Blocked pulls to make sure they're not waiting on something from me, but never when I'm just looking for something to review.

[danielbeardsley]

Are reviewers even looking in the dev blocked column?

OMG, I forgot the purpose of this pull request and now I'm not even sure it lives up to my intent.

When a pull is un blocked, your original dev block should count as an out of date CR so that it it both prompts you to review it and as a signal to others that it's already being reviewed by someone.

[danielbeardsley]

Ironically, dev_block 👍 as this doesn't do what I had intended.