[signoff] Pending review information should not include backed out changes

[leplatrem]

Source: https://bugzilla.mozilla.org/show_bug.cgi?id=1630609

STR:

• Submit some changes to remote settings for review.
• Decide we don't want them so rollback.
• Do some more changes and request review.

Actual Results

• in the records list, under the Waiting Review progress step, the Changes: section includes the backed out changes in the counters (-X, +Y), and the details... link leads to a history page that includes the backed out changes too.
• the diff view has the backed out changes from the rollback, as well as the new changes.

Expected Result

=> the Changes: section counters should not include backed out changes
=> The history view when following the details... link should only show the changes from the last published version

[alexcottner]

When I try to reproduce this, the list view has the rolled back changes but the diff view is accurate.

Regardless, the root problem is the API is returning rolled back changes. I think we should move this issue to the remote-settings project as its signer/changes plugins are responsible for this one.

We have a few possible issues:

1. When a rollback happens, set a flag or status on the affected changes records so we can render that in the UI views
2. When a rollback happens, update the modify date that we compare changes against so the list views know to ignore previous changes
3. When a rollback happens, wipe out the current change records

Of those options, I like 1 and 2 since they don't lose data. But if the changes data should be considered ephemeral, then I can see option 3 being the correct answer.

[leplatrem]

the list view has the rolled back changes but the diff view is accurate.

Just to confirm, you mean the list view of history versus the "simple review" page?

the root problem is the API is returning rolled back changes.

So, the client requests the history endpoint for a list of changes, using the records timestamp of the destination collection (ie. main/{cid}). When rollbacks occur, the destination collection isn't modified. So, indeed, the server returns everything that happens since the last publication went through, not since the last rollback.

kinto-admin/src/sagas/signoff.ts

Lines 175 to 180 in d55c780

	// We get the records timetamp, because it's only bumped when records are changed,
	// unlike the metadata timestamp which is bumped on signature refresh.
	let sinceETag = await client
	.bucket(other.bucket)
	.collection(other.collection)
	.getRecordsTimestamp();

1. When a rollback happens, set a flag or status on the affected changes records so we can render that in the UI views

On rollback, the server sets the resetted collection to "signed":

https://github.com/mozilla/remote-settings/blob/6169a51ec1b9ec04ead5198cdcfb9dc7b9cafb53/kinto-remote-settings/src/kinto_remote_settings/signer/listeners.py#L197-L209

Maybe we could look for the timestamp of this operation, and adjust the request to the history endpoint accordingly?

For example, instead of this:

kinto-admin/src/sagas/signoff.ts

Lines 190 to 197 in d55c780

	// Look up changes since ETag.
	const { data: changes } = await client
	.bucket(source.bucket)
	.collection(source.collection)
	.listRecords({
	since: sinceETag,
	fields: ["deleted"], // limit amount of data to fetch.
	});

We could do sinceETag = Math.max(sinceETag, sourceData.lastReviewDate);

Or, since we have a server scheduled process that refreshes signatures regularly, we cannot use lastSignatureDate

[alexcottner]

the list view has the rolled back changes but the diff view is accurate.

Just to confirm, you mean the list view of history versus the "simple review" page?

"Simple review" as well as the "Diff view" from the history page. (screenshot)

Maybe we could look for the timestamp of this operation, and adjust the request to the history endpoint accordingly?

For example, instead of this:

kinto-admin/src/sagas/signoff.ts

Lines 190 to 197 in d55c780

	// Look up changes since ETag.
	const { data: changes } = await client
	.bucket(source.bucket)
	.collection(source.collection)
	.listRecords({
	since: sinceETag,
	fields: ["deleted"], // limit amount of data to fetch.
	});

We could do sinceETag = Math.max(sinceETag, sourceData.lastReviewDate);

That is a very simple solution to this problem. Nicely done

[alexcottner]

Unfortunately, last review date does not get touched when a rollback happens. The only date that appears to change is the collection's last_edit_date property, which won't work for us. So I guess that's going back to our other options, or changing the logic to update the last_review_date property to get updated when a rollback happens.

[leplatrem]

Oh 😢 That was too simple and too nice 😓

The list view is not the main issue here, because we could consider it exhaustive about past changes. The issue is more about the counters shown on the toolbar.

There is also the option to look in the collection history and look for the last signature refresh.

Filtering history on the status field:

$SERVER/buckets/main-workspace/history?uri=/buckets/main-workspace/collections/product-integrity&target.data.status=to-rollback&_after={last-review}&_sort=-last_modified&_limit=1

Would return one of zero entry, and give us the last rollback date.

Adding a new last-rollback-date field on the collection metadata (option 2?) is not ideal. But if the additional call to the history takes too much time, we could consider this option.

I like to see the history entries as an immutable trail of every operation that was done on the server, and therefore I'm not fan of options 1 and 3.