[CVV Storage][Settings] Adding the checks for cvv tag on each card entry [chromium/src : main]
Vipul Koul (Gerrit)
Attention is currently required from: Vipul Koul.
Vipul Koul uploaded patch set #2 to this change.
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
Bug:1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
---
M chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
M chrome/browser/extensions/api/autofill_private/autofill_util.cc
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/common/extensions/api/autofill_private.idl
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
M tools/typescript/definitions/autofill_private.d.ts
12 files changed, 148 insertions(+), 4 deletions(-)
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Vipul Koul.
Vipul Koul uploaded patch set #3 to this change.
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
Bug:1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
---
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
8 files changed, 136 insertions(+), 4 deletions(-)
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Vipul Koul.
Vipul Koul uploaded patch set #4 to this change.
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Screenshots for cvv tag on credit card entry-
go/strpic/8a5bd15701a19ba75c7d59bbfd6149723177c384
Bug:1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
---
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
8 files changed, 136 insertions(+), 4 deletions(-)
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Vipul Koul.
Vipul Koul uploaded patch set #5 to this change.
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
If a credit card entry has cvv present, "CVC Saved" will be added to the sub-label of the card entry.
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Screenshots for cvv tag on credit card entry-
go/strpic/8a5bd15701a19ba75c7d59bbfd6149723177c384
Bug:1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
---
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
8 files changed, 136 insertions(+), 4 deletions(-)
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Siyu An.
Vipul Koul would like Siyu An to review this change.
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
If a credit card entry has cvv present, "CVC Saved" will be added to the sub-label of the card entry.
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Screenshots for cvv tag on credit card entry-
go/strpic/8a5bd15701a19ba75c7d59bbfd6149723177c384
Bug:1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
---
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
8 files changed, 136 insertions(+), 4 deletions(-)
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Siyu An.
Patch set 5:Commit-Queue +1
Siyu An (Gerrit)
Attention is currently required from: Vipul Koul.
7 comments:
File chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts:
/**
* Returns virtual card metadata if the card is eligible for enrollment or has
* already enrolled, or expiration date (MM/YY) otherwise.
* E.g., 11/23, or Virtual card turned on
*/Can you update this comment since you are here?
Patch Set #5, Line 158:
EXPIRATION_DATEThis is no longer accurate?
Don't have a good idea in mind, but can we change to something like "card secondary information" or add a new type exp + cvc?
Patch Set #5, Line 172:
getSummaryAriaSublabel_I think we need to update this too, and its related tests
File chrome/browser/resources/settings/autofill_page/payments_section.html:
Patch Set #5, Line 49:
creditCardsDo you need this param?
File chrome/browser/resources/settings/autofill_page/payments_section.ts:
Patch Set #5, Line 660:
enableCvcStorageDeleteDataSublabelOh so this string is already in the codebase but we didn't use it. Do you have the link clicking handling ready?
File chrome/test/data/webui/settings/payments_section_card_rows_test.ts:
Patch Set #5, Line 487:
verifyCvcTagPresentForServerCardI think you can probably merge the two tests if not extracting duplicate logic out? As the two cases tested are not much different that need two tests
Patch Set #5, Line 503:
assertEqualsIf you update the sublabel type please also verify it here if possible
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Vipul Koul.
Vipul Koul uploaded patch set #6 to this change.
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
If a credit card entry has cvv present, "CVC Saved" will be added to the sub-label of the card entry.
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Screenshots for cvv tag on credit card entry-
go/strpic/8a5bd15701a19ba75c7d59bbfd6149723177c384
Bug:1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
---
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
8 files changed, 144 insertions(+), 13 deletions(-)
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Siyu An.
7 comments:
File chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts:
/**
* Returns virtual card metadata if the card is eligible for enrollment or has
* already enrolled, or expiration date (MM/YY) otherwise.
* E.g., 11/23, or Virtual card turned on
*/Can you update this comment since you are here?
Done
Patch Set #5, Line 158:
EXPIRATION_DATE
This is no longer accurate? […]
Done
Patch Set #5, Line 172:
getSummaryAriaSublabel_I think we need to update this too, and its related tests
The existing tests should be fine as we would show the new sublabel only for `EXPIRATION_DATE_WITH_CVC_TAG`
File chrome/browser/resources/settings/autofill_page/payments_section.html:
Patch Set #5, Line 49:
creditCardsDo you need this param?
Yup, this is needed due to polymer properties. So if any changes happen to creditCards, all the methods tied to this variable will be triggered. So if any change happens, cvc checked on each credit card and update the sublabel if needed
File chrome/browser/resources/settings/autofill_page/payments_section.ts:
Patch Set #5, Line 660:
enableCvcStorageDeleteDataSublabel
Oh so this string is already in the codebase but we didn't use it. […]
Yup this was added previously here- https://chromium-review.googlesource.com/c/chromium/src/+/4685927
The link clicking handle returns a dummy bubble right now, which is no-op. The bulk delete functionality will be added in the next CL.
File chrome/test/data/webui/settings/payments_section_card_rows_test.ts:
Patch Set #5, Line 487:
verifyCvcTagPresentForServerCard
I think you can probably merge the two tests if not extracting duplicate logic out? As the two cases […]
Done
Patch Set #5, Line 503:
assertEqualsIf you update the sublabel type please also verify it here if possible
Sublabel type is neither added to the credit card object nor to the DOM. So we can't check for it's value.
But if you see my updates, this check is intuitive now!
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Siyu An (Gerrit)
Attention is currently required from: Vipul Koul.
5 comments:
File chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts:
Patch Set #5, Line 172:
getSummaryAriaSublabel_The existing tests should be fine as we would show the new sublabel only for `EXPIRATION_DATE_WITH_C […]
I have a question here: so we only read out "Expiration date: <ph name="CARD">$1<ex>11/2028</ex></ph>" even if it has a CVC? Would it be Expiration date: 11/2028 | 123?
File chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts:
if (loadTimeData.getBoolean('cvcStorageAvailable') && this.creditCard.cvc!
) {
return CardSummarySublabelType.EXPIRATION_DATE_WITH_CVC_TAG;
}Ahh is this formatted?
File chrome/browser/resources/settings/autofill_page/payments_section.html:
Patch Set #5, Line 49:
creditCards
Yup, this is needed due to polymer properties. […]
Got it. Makes sense
File chrome/browser/resources/settings/autofill_page/payments_section.ts:
Patch Set #5, Line 660:
enableCvcStorageDeleteDataSublabel
Yup this was added previously here- https://chromium-review.googlesource. […]
Acknowledged
File chrome/test/data/webui/settings/payments_section_card_rows_test.ts:
Patch Set #6, Line 510:
serverNit: serverCardExpectedSublabel? Same for local
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Vipul Koul.
Vipul Koul uploaded patch set #7 to this change.
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
If a credit card entry has cvv present, "CVC Saved" will be added to the sub-label of the card entry.
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Screenshots for cvv tag on credit card entry-
go/strpic/8a5bd15701a19ba75c7d59bbfd6149723177c384
Bug:1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
---
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
8 files changed, 146 insertions(+), 13 deletions(-)
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Vipul Koul.
Vipul Koul uploaded patch set #8 to this change.
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
If a credit card entry has cvv present, "CVC Saved" will be added to the sub-label of the card entry.
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Screenshot for cvv tag on credit card entry-
go/strpic/8a5bd15701a19ba75c7d59bbfd6149723177c384
Screenshot for cvv tag captioning on credit card entry-
https://screenshot.googleplex.com/8UmVMKTTJ7YEaUy
Bug:1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
---
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
8 files changed, 146 insertions(+), 13 deletions(-)
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Siyu An.
Patch set 8:Commit-Queue +1
3 comments:
File chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts:
Patch Set #5, Line 172:
getSummaryAriaSublabel_
I have a question here: so we only read out "Expiration date: <ph name="CARD">$1<ex>11/2028</ex></ph […]
Discussed offline and added the image in description for audio captioning
File chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts:
if (loadTimeData.getBoolean('cvcStorageAvailable') && this.creditCard.cvc!
) {
return CardSummarySublabelType.EXPIRATION_DATE_WITH_CVC_TAG;
}Ahh is this formatted?
Yes it is. I had the same question but git cl format did this!
File chrome/test/data/webui/settings/payments_section_card_rows_test.ts:
Patch Set #6, Line 510:
serverNit: serverCardExpectedSublabel? Same for local
Done
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Siyu An (Gerrit)
Vipul Koul (Gerrit)
Attention is currently required from: Stephen McGruer.
Vipul Koul would like Stephen McGruer to review this change.
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
If a credit card entry has cvv present, "CVC Saved" will be added to the sub-label of the card entry.
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Screenshot for cvv tag on credit card entry-
go/strpic/8a5bd15701a19ba75c7d59bbfd6149723177c384
Screenshot for cvv tag captioning on credit card entry-
https://screenshot.googleplex.com/8UmVMKTTJ7YEaUy
Bug:1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
---
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
8 files changed, 146 insertions(+), 13 deletions(-)
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Stephen McGruer.
1 comment:
Patchset:
Hey Stephen,
Can you please review this CL?Thanks and please LMK if you have any questions!!
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Stephen McGruer.
Vipul Koul uploaded patch set #9 to this change.
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
If a credit card entry has cvv present, "CVC Saved" will be added to the sub-label of the card entry.
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Screenshot for cvv tag on credit card entry-
go/strpic/8a5bd15701a19ba75c7d59bbfd6149723177c384
Screenshot for cvv tag captioning on credit card entry-
https://screenshot.googleplex.com/8UmVMKTTJ7YEaUy
Bug:1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
---
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
8 files changed, 147 insertions(+), 13 deletions(-)
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Stephen McGruer (Gerrit)
Attention is currently required from: Vipul Koul.
6 comments:
Patchset:
High level this looks ok to me (a few comments so not LGTMing yet), but I'm also not very familiar with typescript. I suggest you bring in dpapad@ for review as well.
File chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts:
Patch Set #9, Line 156:
this.creditCard.cvc!I'm not a typescript expert, but IIUC `!` is the non-null assertion operator, i.e. tells the compiler that this value cannot be null. Why is that necessary here? (Indeed, it seems like checking for null is what you would want to be doing here, as well as checking that it's non-empty).
This is very strange formatting (to have the closing `)` on a newline); was this produced by Chromium's formatter, or did you manually format like this?
Patch Set #9, Line 167:
on orSuper nit: comma before the `or` 😄
`turned on, or 11/23 |`
File chrome/browser/resources/settings/autofill_page/payments_section.ts:
Patch Set #9, Line 660:
return this.i18nAdvanced('enableCvcStorageDeleteDataSublabel')Can you help me understand the need to use i18nAdvanced when there is no `opts` passed in? What does it do differently from `i18n` in that case?
File chrome/test/data/webui/settings/payments_section_test.ts:
Should this be `Cvc`, given you modified the above test to be named that way? (Here and other new test names).
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Vipul Koul.
Vipul Koul uploaded patch set #10 to this change.
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
If a credit card entry has cvv present, "CVC Saved" will be added to the sub-label of the card entry.
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Screenshot for cvv tag on credit card entry-
go/strpic/8a5bd15701a19ba75c7d59bbfd6149723177c384
Screenshot for cvv tag captioning on credit card entry-
https://screenshot.googleplex.com/8UmVMKTTJ7YEaUy
Bug:1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
---
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
8 files changed, 147 insertions(+), 12 deletions(-)
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Stephen McGruer.
5 comments:
File chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts:
Patch Set #9, Line 156:
this.creditCard.cvc!
I'm not a typescript expert, but IIUC `!` is the non-null assertion operator, i.e. […]
I added `!` to check cvc isn't null and has definite value-
If cvc is null, then `!` assert will be false.
If cvc is not null then, if check will make sure it's not empty
This is very strange formatting (to have the closing `)` on a newline); was this produced by Chromiu […]
`git cl format --js chrome/` did this. I tried to format it correctly but the formatter would reset to this!!
Patch Set #9, Line 167:
on or
Super nit: comma before the `or` 😄 […]
Done
File chrome/browser/resources/settings/autofill_page/payments_section.ts:
Patch Set #9, Line 660:
return this.i18nAdvanced('enableCvcStorageDeleteDataSublabel')
Can you help me understand the need to use i18nAdvanced when there is no `opts` passed in? What does […]
`enableCvcStorageDeleteDataSublabel` has a hyperlink in it which the regular `i18n` doesn't pick up (or my implementation of the hyperlink isn't picked up) but with `i18nAdvanced` it works.
Here is the label with hyperlink for your reference- https://source.chromium.org/chromium/chromium/src/+/main:components/autofill_payments_strings.grdp;l=998-1001;drc=d353b1bac40ab22cf7ea191e22bd41dadb0002a6
File chrome/test/data/webui/settings/payments_section_test.ts:
Should this be `Cvc`, given you modified the above test to be named that way? (Here and other new te […]
Yup should be `Cvc`. Thanks for catching it and updated the same!!
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Stephen McGruer (Gerrit)
Attention is currently required from: Vipul Koul.
Patch set 10:Code-Review +1
2 comments:
File chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts:
Patch Set #9, Line 156:
this.creditCard.cvc!
I added `!` to check cvc isn't null and has definite value- […]
Acknowledged
File chrome/browser/resources/settings/autofill_page/payments_section.ts:
Patch Set #9, Line 660:
return this.i18nAdvanced('enableCvcStorageDeleteDataSublabel')
`enableCvcStorageDeleteDataSublabel` has a hyperlink in it which the regular `i18n` doesn't pick up […]
Acknowledged
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Demetrios Papadopoulos.
Vipul Koul would like Demetrios Papadopoulos to review this change.
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
If a credit card entry has cvv present, "CVC Saved" will be added to the sub-label of the card entry.
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Screenshot for cvv tag on credit card entry-
go/strpic/8a5bd15701a19ba75c7d59bbfd6149723177c384
Screenshot for cvv tag captioning on credit card entry-
https://screenshot.googleplex.com/8UmVMKTTJ7YEaUy
Bug:1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
---
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
8 files changed, 147 insertions(+), 12 deletions(-)
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Demetrios Papadopoulos.
Can you please review this CL?
Thanks.
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Demetrios Papadopoulos (Gerrit)
Attention is currently required from: Vipul Koul.
6 comments:
File chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts:
Patch Set #10, Line 133:
// Convert string (e.g. '06') to number (e.g. 6).The code below seems to be doing the opposite, no? Meaning it converts a number like 6 to a string like '06'. Can you clarify?
File chrome/browser/resources/settings/autofill_page/payments_section.ts:
for (const card of this.creditCards) {
if (card.cvc!) {
return this.i18nAdvanced('enableCvcStorageDeleteDataSublabel')
.toString();
}
}How about leveraging Array's the find() method? For example as follows:
```
const card = this.creditCards.find(c => !!card.cvc);
return this.i18nAdvanced(card === null ?
'enableCvcStorageSublabel' : 'enableCvcStorageDeleteDataSublabel');
```Patch Set #10, Line 663:
.toString()Why is this needed? Something seems off here. Does the returned code include HTML markup? Shouldn't this return TrustedHTML instead, similar to other places where i18nAdvanced() is called?
File chrome/test/data/webui/settings/payments_section_card_rows_test.ts:
Patch Set #10, Line 492:
for (const cvcOnServerCard of [true, false]) {Instead of basically running two different tests inside a single test() call, use separate test() calles. For example
```
[true, false].forEach(cvcOnServerCard => {
test('verifyCvcTagPresentForCreditCard_' + cvcOnServerCard, ...);
});
```assertFalse(paymentsList[0]!.shadowRoot!
.querySelector<HTMLElement>('#summarySublabel')!.hidden);
assertFalse(paymentsList[1]!.shadowRoot!
.querySelector<HTMLElement>('#summarySublabel')!.hidden);Here and below: Don't assert directly on the `hidden` property. Use our more thorough isVisible() helper from [1].
Patch Set #10, Line 535:
document.body.innerHTML = window.trustedTypes!.emptyHTML;If you follow my previous comment, then you no longer need to clear the DOM here, and you can still rely on setup() running between each test.
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Vipul Koul.
Vipul Koul uploaded patch set #11 to this change.
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
If a credit card entry has cvv present, "CVC Saved" will be added to the sub-label of the card entry.
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Screenshot for cvv tag on credit card entry-
go/strpic/8a5bd15701a19ba75c7d59bbfd6149723177c384
Screenshot for cvv tag captioning on credit card entry-
https://screenshot.googleplex.com/8UmVMKTTJ7YEaUy
Bug:1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
---
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
8 files changed, 148 insertions(+), 13 deletions(-)
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Demetrios Papadopoulos.
6 comments:
File chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts:
Patch Set #10, Line 133:
// Convert string (e.g. '06') to number (e.g. 6).
The code below seems to be doing the opposite, no? Meaning it converts a number like 6 to a string l […]
Yes you are right and i missed this. I took this comment from the old implementation and ran with it (line 143-147 in base)
Updated the comment. Thanks for catching this Demetrios!!
File chrome/browser/resources/settings/autofill_page/payments_section.ts:
for (const card of this.creditCards) {
if (card.cvc!) {
return this.i18nAdvanced('enableCvcStorageDeleteDataSublabel')
.toString();
}
}
How about leveraging Array's the find() method? For example as follows: […]
Done, thanks for the suggestion!!
Patch Set #10, Line 663:
.toString()
Why is this needed? Something seems off here. […]
I assumed that the label on the toggle would only take string as an input so had converted the same.
This works too and updated the same. Thank you!
File chrome/test/data/webui/settings/payments_section_card_rows_test.ts:
Patch Set #10, Line 492:
for (const cvcOnServerCard of [true, false]) {
Instead of basically running two different tests inside a single test() call, use separate test() ca […]
Done
assertFalse(paymentsList[0]!.shadowRoot!
.querySelector<HTMLElement>('#summarySublabel')!.hidden);
assertFalse(paymentsList[1]!.shadowRoot!
.querySelector<HTMLElement>('#summarySublabel')!.hidden);
Here and below: Don't assert directly on the `hidden` property. […]
Done
Patch Set #10, Line 535:
document.body.innerHTML = window.trustedTypes!.emptyHTML;
If you follow my previous comment, then you no longer need to clear the DOM here, and you can still […]
Yup that worked. Thanks Demetrios!!
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Demetrios Papadopoulos (Gerrit)
Attention is currently required from: Vipul Koul.
Patch set 12:Code-Review +1
1 comment:
File chrome/test/data/webui/settings/payments_section_card_rows_test.ts:
Patch Set #12, Line 520:
const paymentsList = getLocalAndServerCreditCardListItems();FWIW, I think there is a larger issue with the payments related tests, where it is not clear which test is testing what and the responsibilities of each test are quite blurry.
The proper way of testing web components is to have isolated unit tests for each web component (1:1 relatinship between test file and web component). The tests here create a `settings-payments-section`, but then procced (below) to make assertions within the shadow DOM of a `settings-credit-card-list-entry` element.
This is a big code smell IMO, and it makes it unclear who tests what, which file should host which test, often makes tests more flaky (needing to dig through multiple layers of shadow DOM to accomplish its task).
Obviously this can't be fixed in this CL, but I strongly suggest looking into this issue and refactoring/rewriting tests as time allows.
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Demetrios Papadopoulos, Siyu An, Stephen McGruer, Vipul Koul.
Vipul Koul uploaded patch set #13 to this change.
The following approvals got outdated and were removed: Code-Review+1 by Demetrios Papadopoulos, Code-Review+1 by Siyu An, Code-Review+1 by Stephen McGruer
The change is no longer submittable: Code-Owners and Code-Review are unsatisfied now.
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
If a credit card entry has cvv present, "CVC Saved" will be added to the sub-label of the card entry.
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Screenshot for cvv tag on credit card entry-
go/strpic/8a5bd15701a19ba75c7d59bbfd6149723177c384
Screenshot for cvv tag captioning on credit card entry-
https://screenshot.googleplex.com/8UmVMKTTJ7YEaUy
Bug:1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
---
M chrome/browser/extensions/api/autofill_private/autofill_private_apitest.cc
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
9 files changed, 151 insertions(+), 13 deletions(-)
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Demetrios Papadopoulos, Siyu An, Stephen McGruer, Vipul Koul.
Vipul Koul uploaded patch set #14 to this change.
The following approvals got outdated and were removed: Commit-Queue+1 by Vipul Koul
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
If a credit card entry has cvv present, "CVC Saved" will be added to the sub-label of the card entry.
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Screenshot for cvv tag on credit card entry-
go/strpic/8a5bd15701a19ba75c7d59bbfd6149723177c384
Screenshot for cvv tag captioning on credit card entry-
https://screenshot.googleplex.com/8UmVMKTTJ7YEaUy
Bug:1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
---
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
8 files changed, 148 insertions(+), 13 deletions(-)
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Siyu An (Gerrit)
Attention is currently required from: Demetrios Papadopoulos, Stephen McGruer, Vipul Koul.
Patch set 14:Code-Review +1
1 comment:
File chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts:
Do you need this? Is this check specifically on whether this CVC is null or undefined?
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Demetrios Papadopoulos, Siyu An, Stephen McGruer.
2 comments:
File chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts:
Do you need this? Is this check specifically on whether this CVC is null or undefined?
Lol Stephen had asked the same question, so can you please refer to this- https://chromium-review.googlesource.com/c/chromium/src/+/4865647/comment/2208316f_425a8637/
Thanks Siyu!!
File chrome/test/data/webui/settings/payments_section_card_rows_test.ts:
Patch Set #12, Line 520:
const paymentsList = getLocalAndServerCreditCardListItems();
FWIW, I think there is a larger issue with the payments related tests, where it is not clear which t […]
Sorry trying to understand what you mean here.
So `createPaymentsSection()` creates a payment section but then we extract the `settings-credit-card-list-entry` via `getLocalAndServerCreditCardListItems()` for the credit card object list. IMO this seems like the correct thing to do-
1. Create the payments settings page
2. Extract the cards from the DOM of the above created page
3. Assert the cards for valuesI am sorry if i am aware of any standards used for testing front end code and can you please provide me with some documentation for the same?
Thanks!
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Demetrios Papadopoulos (Gerrit)
Attention is currently required from: Siyu An, Stephen McGruer, Vipul Koul.
File chrome/test/data/webui/settings/payments_section_card_rows_test.ts:
Patch Set #12, Line 520:
const paymentsList = getLocalAndServerCreditCardListItems();
. IMO this seems like the correct thing to do
If we are trying to test `settings-credit-card-list-entry` itself why are we not creating such an element directly? The standard practice is to "unit test" web components on their own.
Create the payments settings page
This is justified when trying to test the settings-settings-section itself (code that resides in that web component, not transitively in all other child web components). For example such a test should only check that settings-payment-section is rendering the expected number of items as it should. Unit testing the internals of such items does not belong in the same test.
I am sorry if i am aware of any standards used for testing front end code and can you please provide me with some documentation for the same?
We don't have dedicated documentation for this. The code serves as a form of documentation. See examples below where inividual UI elements are unit tested in isolation.
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Siyu An (Gerrit)
Attention is currently required from: Stephen McGruer, Vipul Koul.
1 comment:
File chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts:
Lol Stephen had asked the same question, so can you please refer to this- https://chromium-review. […]
I think what you want is probably "!!this.creditcard.cvc" which casts value to boolean, instead of this.
This is non-null assertion operator which tells the ts that you are sure this is not null when this inference is beyond its knowledge.
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Stephen McGruer, Vipul Koul.
Vipul Koul uploaded patch set #15 to this change.
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
If a credit card entry has cvv present, "CVC Saved" will be added to the sub-label of the card entry.
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Screenshot for cvv tag on credit card entry-
go/strpic/8a5bd15701a19ba75c7d59bbfd6149723177c384
Screenshot for cvv tag captioning on credit card entry-
https://screenshot.googleplex.com/8UmVMKTTJ7YEaUy
Bug:1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
---
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
8 files changed, 147 insertions(+), 13 deletions(-)
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Vipul Koul (Gerrit)
Attention is currently required from: Stephen McGruer.
1 comment:
File chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts:
I think what you want is probably "!!this.creditcard. […]
Got it and updated the same. Thanks Siyu!!
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Stephen McGruer (Gerrit)
Vipul Koul (Gerrit)
Patch set 16:Commit-Queue +2
Vipul Koul (Gerrit)
Attention is currently required from: Demetrios Papadopoulos.
1 comment:
File chrome/test/data/webui/settings/payments_section_card_rows_test.ts:
Patch Set #12, Line 520:
const paymentsList = getLocalAndServerCreditCardListItems();
> . IMO this seems like the correct thing to do […]
Got it, this should be a seperate a refactor to fix these. I'll try to do these in the future CLs!
Thanks for the explanation Demetrios!!
To view, visit change 4865647. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
15 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
[CVV Storage][Settings] Adding the checks for cvv tag on each card entry
If a credit card entry has cvv present, "CVC Saved" will be added to the sub-label of the card entry.
Design doc- go/pay-autofill-cvv-storage-chrome
UX mocks- go/autofill-cvv-storage-ux
Screenshot for cvv tag on credit card entry-
go/strpic/8a5bd15701a19ba75c7d59bbfd6149723177c384
Screenshot for cvv tag captioning on credit card entry-
https://screenshot.googleplex.com/8UmVMKTTJ7YEaUy
Bug: 1464441
Change-Id: I47bebc307ae6ae5775e2cd5f3821af4de8587402
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4865647
Reviewed-by: Siyu An <si...@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpa...@chromium.org>
Reviewed-by: Stephen McGruer <smcg...@chromium.org>
Commit-Queue: Vipul Koul <koul...@google.com>
Cr-Commit-Position: refs/heads/main@{#1214570}
---
M chrome/browser/resources/settings/autofill_page/credit_card_list_entry.ts
M chrome/browser/resources/settings/autofill_page/payments_section.html
M chrome/browser/resources/settings/autofill_page/payments_section.ts
M chrome/browser/ui/webui/settings/settings_localized_strings_provider.cc
M chrome/test/data/webui/settings/payments_section_card_rows_test.ts
M chrome/test/data/webui/settings/payments_section_test.ts
M components/autofill_payments_strings.grdp
A components/autofill_payments_strings_grdp/IDS_AUTOFILL_SETTINGS_PAGE_CVC_TAG_FOR_CREDIT_CARD_LIST_ENTRY.png.sha1
8 files changed, 147 insertions(+), 13 deletions(-)