The ACK state of all validation errors is lost when changing a single one

[schlessera]

Bug Description

When changing a single validation error's state (from "Removed" to "Kept", for example, the ACK state of all validation errors is lost. As new ones are created as well, this makes for a very consuming and random experience.

I assume this is due to the fact that the dropdown with the values does not distinguish between ACK or not anymore, so WP will just store it as a user value either way.

Expected Behaviour

The ACK state should only change if I specifically act on something. This might need a new UI element, though.

Steps to reproduce

1. Go to the validation errors screen for a page with multiple new AMP validation errors.
2. Verify: The screen should have multiple errors with a reddish border at the left.
3. Change a single validation error from "Removed" to "Kept".
4. Hit the "Update" button.
5. Verify: Not only the changed validation error has lost its reddish border, but multiple other ones as well.

Screenshots

https://youtu.be/yV82f6isY7U

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation brief

QA testing instructions

Demo

Changelog entry

[westonruter]

Thanks for bringing this up. It's something that I acknowledged when working on the changes in #3630.

See in particular #2023 (comment)

You can see there I was wrestling with how to best provide an interface for managing the new state, considering either having a row action link or adding a new column that has a button to toggle the new state:

Ultimately, I decided the ability to mark something as acknowledged or new was not critical and users may not need it: the way to clear the new state was just to “touch” the validation errors to confirm the state they are already in. Nevertheless, the fact that the the new state gets lost when updating has been bothering me, and I agree with you that the new state should be made independent of the removed/kept state.

On the validated URL screen, toggling the new state of a validation error can be an Ajax operation not requiring a full page load. This is in contrast with saving updates to the removed/kept state, which is something that has to be previewed as it has impacts on what the user will see on the frontend. The new state has no such frontend impacts, and since it doesn't need to be previewed it can be applied immediately.

Having a row action link seems the most WordPressy way to present this. A new validation error can have “Mark as read” and an acknowledged error could have “Mark as unread” (or perhaps some other term than “read”). In general then the behavior can be similar to moderating comments. When you click “Approve” then it changes the new (unmoderated) state to the normal state, and the “Unapprove” link moves a comment back to the new (unmoderated) state.

Aside: I was thinking about adding “approve” and “unapprove” to validation errors before the change in terminology to removed/kept. That could have meant a user could have validation errors in these states:

• Unapproved accepted
• Approved accepted
• Unapproved rejected
• Approved rejected

😂

---

The issue you're seeing with toggling the removed/kept state on an Excessive CSS validation error behaves uniquely because it can cause other validation errors to be raised as the CSS limits then change. So I think that is expected, but what makes it more disorienting is the “new“ state that gets added or removed apparently at random.

[westonruter]

Aside: I think this is fine to defer to v1.6.

[schlessera]

Per @westonruter in #4382 (review):

I think we need to defer this until we have a good UI to manage the new/ack state. Let's hold off on this until v1.6 when we have a good design in #3725.

[westonruter]

In conjunction with #3726, the fastest way to implement the introduction of a new Approved column in the existing post list table would be:

1. Make the select dropdown for Removed/Kept only contain a boolean 0 or 1 which corresponds to AMP_Validation_Error_Taxonomy::ACCEPTED_VALIDATION_ERROR_BIT_MASK (where “Accepted“ is the old name for “Removed”).
2. Remove the name attribute from the select.
3. Introduce a new Approved column which contains a toggle that contains the boolean state for Unapproved/Approved (aka new/acknowledged) which corresponds to \AMP_Validation_Error_Taxonomy::ACKNOWLEDGED_VALIDATION_ERROR_BIT_MASK.
4. Add a new hidden field which contains the computed combination of these two bits. This would have the name which the select used to have.
5. Each time a select dropdown is modified to change Removed/Kept or the toggle is flipped to change Approved/Unapproved, the hidden input would be updated.

I acknowledge this is old-school DOM dancing, but it's what we have at the moment. The smaller the change here the easier it will be to merge into a minor release. Eventually we'll make this all much cleaner and elegant when the screens get redesigned to use React (#2316), but this is not minor release territory!

[amedina]

Tested this and is working. I am noticing a delay on the updating of the status of the errors upon an update, but the behavior of the PR holds.