Mark soft conflict if not contiguous with trunk

[ktuite]

Closes getodk/central#698

This PR makes two changes:
1: Update per-version conflict state

The <entity uuid>/versions endpoint returns a conflict state for each version of the entity, which is computed from other information about the entity versions. These values should stay the same even when the top level conflict flag on the entity changes (or is manually resolved). These version-specific conflict states used to be detected just from baseVersion+1 != version.

In this PR, we've copied branch-tracking logic from Frontend to check whether each version if it is contiguous with its trunk version, and return a soft conflict state if it is non-contiguous but otherwise not a hard conflict.

We could also consider having backend return more of the details that frontend is currently computing about branch continuity.

2. Update top-level conflict state

The above work to make the per-version conflict state more descriptive wasn't enough. Consider a situation where an offline branch has been interrupted by another branch or online update, a conflict has been resolved, but then another update from that interrupted branch comes in. We want every update that is not contiguous with its trunk version to be a conflict, and that means updating the conflict state on the entity itself, too.

Before, we only noted a conflict when the base version wasn't what was expected. An offline update would not be counted as conflict as long as the previous action in the branch was present.

This PR adds a check that the new entity is not only contiguous with its base version, but is also contiguous with its specified trunk version. (This is done by looking for a version of a different branch that interrupts the given branch, rather than the other code that tracks the last contiguous version per branch.) If it is not contiguous, i.e. if there is an interruption, the top level conflict flag on the entity itself is set to soft instead of null.

What has been done to verify that this works as intended?

Tests.

Why is this the best possible solution? Were any other approaches considered?

See explanation above.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

Highlights conflicts more.

Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.

No.

Before submitting this PR, please make sure you have:

• run make test and confirmed all checks still pass OR confirm CircleCI build passes
• verified that any code from external sources are properly credited in comments or that everything is internally sourced

[matthew-white]

We could also consider having backend return more of the details that frontend is currently computing about branch continuity.

I think this is a pretty interesting idea. I don't think it's necessary for this particular issue, but I think it's something we should keep in mind for the future.

[matthew-white]

I copied this branch tracking logic directly from Frontend, and there's probably more there than necessary for backend, but I just wanted to see it in action.

I'm going to pare back aspects of the Branch class and related logic to only what Backend needs, which I think will make the Backend code clearer. However, even pared back, I like the Branch class here. It moves the branch logic into a separate part of the code from the rest of the getWithConflictDetails() function, which is already fairly complex. I also think it's very possible that we'll want to grow the Branch class in the future.

Before I push my commit, I thought I'd leave a few comments about specific things I'm changing.

[matthew-white]

This ends up looping over defs twice, which I don't think we need to do. I think we can build up result and branches in the same loop, as no version/def needs to look ahead to a later version, including for branch information.

[matthew-white]

I know we do it in Frontend, but I think we should avoid mutating version here unless it's necessary.

[matthew-white]

I added version.branchBaseVersion != null to Frontend mostly as a temporary measure, since at the time, there were submissions where the entity creation got a branch ID. I think we can remove the condition here.

[matthew-white]

This PR makes changes to the conflict property of entity versions, but in some cases, I think we also need to make changes to the conflict property of the entity itself. For example, say an entity had a conflict, but was then resolved. A new offline update comes in that applies to the current version. That all seems good, and currently, the conflict property of the entity will be null. However, with the change to the conflict logic, the new version may actually be a conflict: the conflict property of the new version will be 'soft' if its base version is not contiguous with the trunk version. In that case, the conflict property of the entity itself should also be 'soft'. However, right now I'm pretty sure it's null. I modified the first test to try to create an example:

it('should mark an update that is not contiguous with its trunk version as a soft conflict', testOfflineEntities(async (service, container) => {
  const asAlice = await service.login('alice');
  const branchId = uuid();

  // Update existing entity on server (change age from 22 to 24)
  await asAlice.patch('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc?baseVersion=1')
    .send({ data: { age: '24' } })
    .expect(200);

  // Send update (change status from null to arrived)
  await asAlice.post('/v1/projects/1/forms/offlineEntity/submissions')
    .send(testData.instances.offlineEntity.one
      .replace('branchId=""', `branchId="${branchId}"`)
    )
    .set('Content-Type', 'application/xml')
    .expect(200);
  await exhaust(container);

  await asAlice.patch('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc?resolve=true&baseVersion=3')
    .expect(200);

  // Send second update (change age from 22 to 26)
  await asAlice.post('/v1/projects/1/forms/offlineEntity/submissions')
    .send(testData.instances.offlineEntity.one
      .replace('branchId=""', `branchId="${branchId}"`)
      .replace('one', 'one-update2')
      .replace('baseVersion="1"', 'baseVersion="2"')
      .replace('<status>arrived</status>', '<age>26</age>')
    )
    .set('Content-Type', 'application/xml')
    .expect(200);
  await exhaust(container);

  await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc/versions')
    .then(({ body: versions }) => {
      versions.map(v => v.conflict).should.eql([null, null, 'soft', 'soft']);
    });

  await asAlice.get('/v1/projects/1/datasets/people/entities/12345678-1234-4123-8234-123456789abc')
    .then(({ body: entity }) => {
      // This assertion currently fails.
      should(entity.conflict).equal('soft');
    });
}));

[matthew-white]

Also, @ktuite, I've pushed my commit. Would you mind taking a look at it?

[ktuite]

@matthew-white I added some code in the _updateEntity section and added in your test scenario. I also updated the description of this PR.

[ktuite]

fourth update from the branch (3rd update is missing). this will get held in the backlog and when it gets force-processed, it will take the latest version as the base version and therefore not have a conflict due to version mismatch...

[ktuite]

link to issue here and mention the scenario is mentioned there

[ktuite]

We saw a scenario in a test where this logic didn't quite work out

conflict false false
branches.get(v.branchId).lastContiguousWithTrunk 2
v baseversion 1
add: version, base version and last contiguous 3 1 2
conflict true false
branches.get(v.branchId).lastContiguousWithTrunk 2
v baseversion 1
add: version, base version and last contiguous 4 3 2
conflict true true
branches.get(v.branchId).lastContiguousWithTrunk 2
v baseversion 3