Revert proposed change state to open on merge failure

[ogenstad]

If we merge a proposed change and there's an error we previously didn't revert the state back to "open" from "merging", this PR corrects this behaviour. Previously we did revert this if a user tried to merge a proposed change with data conflicts without providing a resolution to all conflicts.

Fixes #5563

Note: Today we could reach this state where the proposed change was stuck in the "merging" state if there was a data conflict and the user tried to resolve the conflict using the old way and do it within the Checks tab. I'm not certain how to create a scenario where we end up here? I.e if we want to create a test that hits this path. Does anyone have any ideas about that?

[codspeed-hq]

CodSpeed Performance Report

Merging #5564 will not alter performance

_{Comparing pog-proposed-change-merge-state-IFC-1157 (cd9d598) with stable (7e98238)}

Summary

✅ 10 untouched benchmarks

[ajtmccarty]

I'm not certain how to create a scenario where we end up here? I.e if we want to create a test that hits this path. Does anyone have any ideas about that?

Could you have a test that creates a proposed change for a branch with a data conflict and try to merge it?

[ogenstad]

Could you have a test that creates a proposed change for a branch with a data conflict and try to merge it?

The thing is that we already have a test like that here: https://github.com/opsmill/infrahub/blob/infrahub-v1.1.4/backend/tests/integration/proposed_change/test_proposed_change_conflict.py#L149-L152 The problem in this case was that the current logic also checks for resolutions on the check object itself and we're going to remove that so I don't see the point of adding a test for when the resolution is solved that way. So we'd need something else to cause the failure. Do you have any other thoughts as to what could cause a failure? Otherwise I'll merge this as is for now.

[ajtmccarty]

Do you have any other thoughts as to what could cause a failure? Otherwise I'll merge this as is for now.

Hmm. Not really. Maybe you could patch merge_branch? We don't do a lot of patching in our unit tests, but it might be appropriate here b/c we are trying to enter a failure state that should normally be prevented

[ogenstad]

Hmm. Not really. Maybe you could patch merge_branch? We don't do a lot of patching in our unit tests, but it might be appropriate here b/c we are trying to enter a failure state that should normally be prevented

I added a test please take another look.

[ajtmccarty]

LGTM

[ajtmccarty]

not sure if you still need this fixture

[ogenstad]

I think it's mostly just following the current convention where we have some test setup that is happening within a fixture. It's following what we are doing in the other tests. But I can move this code do the actual test instead if that's what you mean as we typically don't reuse these fixtures?

[ajtmccarty]

I meant that I don't think this fixture is actually used in the test_merge_failure test b/c that test seems to use the conflict_free branch and not the failing_branch branch created here. But I could be missing something

[ogenstad]

Ah ok I see the confusion. I managed to change the name of the proposed change to "failing_branch", where I should really have named it "failing_branch". I think it's probably cleaner if I swap that part out so the branches doesn't interact with each other.