Forms validator should ignore clientMissing

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: markh, Assigned: tiago, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment)

Reporter

Description

2 years ago
We never delete form history records from the server as we don't get notified when that happens. Sad! Therefore we should probably have the form history validator ignore clientMissing as it will be expected they are missing (ie, there were deleted locally but we didn't record the deletion on the server).
Reporter

Updated

2 years ago
See Also: → 1346850
Flags: needinfo?(tchiovoloni)
The needinfo was intended for me to look for a dupe which I cannot find.

This is a very easy bug and would be a good candidate for someone getting ramped up on desktop (*cough* :Grisha *cough*), although it's a fine first bug otherwise.

To ensure it's actionable either way, I'll say what probably needs to be done:

The approach I'd recommend would be to add a property to CollectionValidator that basically equates to "does this collection support tombstones" (defaulting to true), and if it doesn't, somehow ensure that problems.clientMissing is empty when you return from compareClientWithServer (probably either by clearing it out at the end, or never pushing into it). Then set this property to true in FormValidator's constructor.

We don't currently have any FormValidator or general CollectionValidator tests, so the most natural place to test this would be in the PasswordValidator tests, which basically is the stand-in for the latter.
Assignee: nobody → tchiovoloni
Flags: needinfo?(tchiovoloni)
Whoops, didn't mean to assign myself.
Assignee: tchiovoloni → nobody
Mentor: tchiovoloni
Priority: -- → P3
Whiteboard: [good first bug]
Comment hidden (mozreview-request)
Assignee

Comment 4

2 years ago
(In reply to Santiago Paez [:tiago] from comment #3)
> Created attachment 8871983 [details]
> Bug 1354016 - Forms validator ignore clientMissing.
> 
> Review commit: https://reviewboard.mozilla.org/r/143528/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/143528/

Hi!

This is a first try at the bug. If there is any improvement or change, I'll add a new version, and after that I'll add the tests.

All comments welcome, thanks!

Comment 5

2 years ago
mozreview-review
Comment on attachment 8871983 [details]
Bug 1354016 - Forms validator ignore clientMissing.

https://reviewboard.mozilla.org/r/143528/#review147942

With tests and the issue addressed this looks more or less right.

::: services/sync/modules/engines/forms.js:269
(Diff revision 1)
>  }
>  
>  class FormValidator extends CollectionValidator {
>    constructor() {
>      super("forms", "id", ["name", "value"]);
> +    super.setIgnoresMissingClients(true);

If we decided to override this in the subclass (and I'm not sure why we would, which argues against having this as a setter), this would bypass the override.

Honestly, IMO you should just do `this.ignoresMissingClients = true` here, and get rid of the setter. But if you're really attached to using a setter, you should call it on `this` and not on `super`.
Attachment #8871983 - Flags: review?(tchiovoloni)
Assignee

Comment 6

2 years ago
mozreview-review
Comment on attachment 8871983 [details]
Bug 1354016 - Forms validator ignore clientMissing.

https://reviewboard.mozilla.org/r/143528/#review148136

::: services/sync/modules/engines/forms.js:269
(Diff revision 1)
>  }
>  
>  class FormValidator extends CollectionValidator {
>    constructor() {
>      super("forms", "id", ["name", "value"]);
> +    super.setIgnoresMissingClients(true);

Not attached to the setter, it's gone :)
Comment hidden (mozreview-request)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8871983 [details]
Bug 1354016 - Forms validator ignore clientMissing.

https://reviewboard.mozilla.org/r/143528/#review148444

::: services/sync/tests/unit/test_password_validator.js:160
(Diff revision 2)
>  
> +add_test(function test_formValidatorIgnoresMissingClients() {
> +  // Since history form records are not deleted from the server, the
> +  // |FormValidator| shouldn't set the |missingClient| flag in |problemData|.
> +  let validator = new FormValidator();
> +  let { server, client } = getDummyServerAndClient();

Unfortunately, these give password records, which have a (very) different format from form records. And so it's not really supported to send it to the form validator.

This isn't actually a problem at the moment (obviously, since your test passes), but I don't like the thought of our tests relying on the forms validator not reporting errors when given password records.

The easiest fix is to test the ignoresMissingClients functionality on the password validator instead -- that is, do `let validator = new PasswordValidator(); validator.ignoresMissingClients = true;` (and also remove the forms import from the top).

Thanks for your work so far!
Attachment #8871983 - Flags: review?(tchiovoloni)
Assignee

Comment 9

2 years ago
You are correct, I didn't realize that.

I can make the fix you suggested, but what do you think about creating a |test_form_validator.js| file with a |getDummyServerAndClient| that gives form records? If I'm not wrong the structure of the code would be pretty similar to the |test_password_validator.js|.

Thanks for the feedback!
Flags: needinfo?(tchiovoloni)
Adding one wouldn't bother me and couldn't hurt. If you feel like it, go for it, but also don't feel like you need to do it if it ends up being unexpectedly difficult (note that you will have to add the new filename to the xpcshell.ini in the same directory, if you've never added new test files before).

(For some context, the reason we don't have one is that it would mostly just serve to test the common collection_validator code, which is already exercised by the password validator test, but again, having it couldn't hurt).
Flags: needinfo?(tchiovoloni)
Comment hidden (mozreview-request)

Comment 12

2 years ago
mozreview-review
Comment on attachment 8871983 [details]
Bug 1354016 - Forms validator ignore clientMissing.

https://reviewboard.mozilla.org/r/143528/#review148838

Thanks, this looks great.

Fix the nit and resubmit and it should be good to go.

::: services/sync/tests/unit/test_form_validator.js:75
(Diff revision 3)
> +
> +add_test(function test_formValidatorIgnoresMissingClients() {
> +  // Since history form records are not deleted from the server, the
> +  // |FormValidator| shouldn't set the |missingClient| flag in |problemData|.
> +  let { server, client } = getDummyServerAndClient();
> +  

nit: trailing whitespace on this line.
Attachment #8871983 - Flags: review?(tchiovoloni)
Comment hidden (mozreview-request)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8871983 [details]
Bug 1354016 - Forms validator ignore clientMissing.

https://reviewboard.mozilla.org/r/143528/#review149826

Thanks!
Attachment #8871983 - Flags: review?(tchiovoloni) → review+

Comment 15

2 years ago
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be7694b72c8a
Forms validator ignore clientMissing. r=tcsc
https://hg.mozilla.org/mozilla-central/rev/be7694b72c8a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Assignee: nobody → tiago.paez11
You need to log in before you can comment on or make changes to this bug.