Skip to content

Check WIP status as late as possible

Thomas Randolph requested to merge 21318-rebase-button-not-available-for-wip-merge-requests into master

What does this MR do?

For #21318 (closed)

Moves the WIP check to as low as possible in the sequence of state checks.

Unfortunately, the unresolvedDiscussions check also checks for WIP, so it has to be after the real WIP check.

In theory, we could move WIP to the second to last check and move Unresolved Discussions to the last check.
In practice - however - the order of these checks is important, and moving a lot of checks around causes cascading side effects. So this MVC just moves the WIP down two spots.

Testing

This MR also updates the test for this state check, and the test file is a little confusing.

I'll try to explain it here.

Since the state checks are organized as a sequence of else if checks, only one can be in effect at any time, and - critically - the first matching expression will short-circuit the whole block. The test takes advantage of this by checking the possible states in reverse order: from the bottom up.

So my changes add the WIP test right before the pipeline failure test. Later (in the tests) or earlier (in the source) matching expressions will short-circuit the else if block and keep it from being matched.

Similarly, rebase was untested, so I added a test for that state. The matching expression in that case (context.shouldBeRebased == true) has to come after the test for the pipeline failure, because when it's true, the code never checks for the pipeline failure case.

To test manually, please see the steps from the originating issue.

Solution

In the original issue, it was requested that we move the WIP check to be the very last thing since everything else has to be resolved before the MR can be merged anyway, so WIP is the last thing that needs to be addressed. However, since that move causes a bunch of cascading side effects, I think the right iterative approach here is to fix the inability to see the rebase button first, and then come back to refactoring this section of the product.

Screenshots

Before After
before after

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team
Edited by Thomas Randolph

Merge request reports