[Form Autofill] Add the ability for "startSearch" to retrieve credit card profiles from storage

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ralin, Assigned: ralin)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [form autofill:M4][ETA:7/18])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
We need to a way rather than _getAddresses to retrieve records from credit cards storage in "startSearch" in order to provide corresponding results for showing popup.
(Assignee)

Comment 1

2 years ago
Maybe we could consider creating a new method: _getCreditCards() or refactoring the original _getAddresses() to a more generic one like: _getRecords() in FormAutofillContent.jsm.
Duplicate of this bug: 1371118
(Assignee)

Updated

2 years ago
Assignee: nobody → ralin
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Whiteboard: [form autofill:M4] → [form autofill:M4][ETA:7/18]
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
Add a demo page for credit card: https://luke-chang.github.io/autofill-demo/basic_cc.html

We can use this page to verify if the results are actually retrieved from "creditCards" when focus on credit card fields.
(Assignee)

Comment 6

2 years ago
Posted file autofill-profiles.json (obsolete) —
We'll need a storage with credit cards profiles as well.
(Assignee)

Comment 7

2 years ago
update autofill-profiles.json to conform with storage schema.
Attachment #8886930 - Attachment is obsolete: true

Comment 8

2 years ago
mozreview-review
Comment on attachment 8886115 [details]
Bug 1380279 - Add an ability to retrieve multiple collection type in startSearch to support credit cards autofilling.

https://reviewboard.mozilla.org/r/156914/#review163464

It looks good.
I have only concern that there is no specific xpcshell-test or others to verify the behavior of retrieving credit card records. e.g. `FormAutofillContent._getRecords({collectionName: "creditCards"})`
May I know how do you verify it while implementing? Could you find a proper place to add the test? Thanks.

::: browser/extensions/formautofill/FormAutofillContent.jsm:166
(Diff revision 2)
> -      Services.cpmm.addMessageListener("FormAutofill:Addresses", function getResult(result) {
> -        Services.cpmm.removeMessageListener("FormAutofill:Addresses", getResult);
> +      Services.cpmm.addMessageListener("FormAutofill:Records", function getResult(result) {
> +        Services.cpmm.removeMessageListener("FormAutofill:Records", getResult);
>          resolve(result.data);
>        });
>  
> -      Services.cpmm.sendAsyncMessage("FormAutofill:GetAddresses", data);
> +      Services.cpmm.sendAsyncMessage("FormAutofill:GetRecords", data);

Use `JSON.stringify(data)` to prevent the warning message in console.

::: browser/extensions/formautofill/FormAutofillParent.jsm:243
(Diff revision 2)
>     * @param  {nsIFrameMessageManager} target
>     *         Content's message manager.
>     */
> -  _getAddresses({searchString, info}, target) {
> -    let addresses = [];
> +  _getRecords({collectionName, searchString, info}, target) {
> +    let records = [];
> +    let collection = this.profileStorage[collectionName];

`collection` should be checked if it's `null`.
Use an empty array as the result e.g. `target.sendAsyncMessage("FormAutofill:Records", []);`.
The warning message is necessary here to record the incorrect `collectionName` access here.

::: browser/extensions/formautofill/content/manageProfiles.js:82
(Diff revision 2)
> +   *
> +   * @param  {string} data.collectionName
>     *
>     * @returns {promise}
>     */
> -  getAddresses() {
> +  getRecords(data) {

The comment only mentioned `data.collectionName`, but `data.info` and `data.searchString` do affect the results as well.
I think there are two ways to improve it here:
1. `getRecords` only allows `collectionName`.
e.g. `getRecords(collectionName)`
2. Update the comment to describe that it will pass the data object to `"FormAutofill:GetRecords" message` as well.
```* @param {Object} the parameter for "FormAutofill:GetRecords" message.
```
Attachment #8886115 - Flags: review?(selee)
(Assignee)

Comment 9

2 years ago
mozreview-review-reply
Comment on attachment 8886115 [details]
Bug 1380279 - Add an ability to retrieve multiple collection type in startSearch to support credit cards autofilling.

https://reviewboard.mozilla.org/r/156914/#review163464

Thanks for the review :D

I'll fix the patch soon. For the lacking xpcshell-test, it seems reasonable to me to crib from the original test_addressRecords.js and maybe create test_creditCardsRecords.js doing something similarly.

> Use `JSON.stringify(data)` to prevent the warning message in console.

Gonna check this out, IIRC, passing pure object won't cause warning message unless the data contains DOM object?

> `collection` should be checked if it's `null`.
> Use an empty array as the result e.g. `target.sendAsyncMessage("FormAutofill:Records", []);`.
> The warning message is necessary here to record the incorrect `collectionName` access here.

Can we just refactor the if-statement instead, like:
```javascript
  let records;
  let collection = this.profileStorage[collectionName];
  
  if (!collection) {
    records = [];
  } else if(info && info.fieldName) {
    records = collection.getByFilter({searchString, info});
  } else {
    records = collection.getAll();
  }
  target.sendAsyncMessage("FormAutofill:Records", records);
    
```
would this be conciser than having another `target.sendAsyncMessage("FormAutofill:Records", []);`?

> The comment only mentioned `data.collectionName`, but `data.info` and `data.searchString` do affect the results as well.
> I think there are two ways to improve it here:
> 1. `getRecords` only allows `collectionName`.
> e.g. `getRecords(collectionName)`
> 2. Update the comment to describe that it will pass the data object to `"FormAutofill:GetRecords" message` as well.
> ```* @param {Object} the parameter for "FormAutofill:GetRecords" message.
> ```

Good point, I'd prefer approach 2 in case we need the rest of options in the future.
(Assignee)

Comment 10

2 years ago
(In reply to Ray Lin[:ralin] from comment #9)

> I'll fix the patch soon. For the lacking xpcshell-test, it seems reasonable
> to me to crib from the original test_addressRecords.js and maybe create
> test_creditCardsRecords.js doing something similarly.
I found this is wrong, the test_creditCardsRecords.js is already existing and irrelevant to this patch. I'll find the place to test for _getRecords, though I didn't find any testing _getAddresses() prior to this changes.
Comment hidden (mozreview-request)
(Assignee)

Comment 12

2 years ago
A unit test for parant's _getRecords was added in this revision. It's basically verify whether Parent grab the correct collection, and make sure the getAll be called in the storage. The rest of operations in Content or Storage are already covered in test_addressRecords.js/test_creditCardRecords.js, so I don't tend to duplicate those here. Thanks.

Comment 13

2 years ago
mozreview-review
Comment on attachment 8886115 [details]
Bug 1380279 - Add an ability to retrieve multiple collection type in startSearch to support credit cards autofilling.

https://reviewboard.mozilla.org/r/156914/#review164494

LGTM! Thank you!
Attachment #8886115 - Flags: review?(selee) → review+
Autoland can't push this because it doesn't meet the review requirements in MozReview.
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits
Keywords: checkin-needed
Comment on attachment 8886115 [details]
Bug 1380279 - Add an ability to retrieve multiple collection type in startSearch to support credit cards autofilling.

https://reviewboard.mozilla.org/r/156914/#review164738

re=me after Sean's review.
Attachment #8886115 - Flags: review+

Comment 17

2 years ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/a15a4779cbde
Add an ability to retrieve multiple collection type in startSearch to support credit cards autofilling. r=MattN,seanlee
https://hg.mozilla.org/mozilla-central/rev/a15a4779cbde
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.