New issue
Advanced search Search tips

Issue 800829 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

In autofill settings, we should update existing data if possible.

Project Member Reported by se...@chromium.org, Jan 10 2018

Issue description

The current code creates a new model each time and then sets the values. This is dangerous because it's easy to forget some data.

This would also remove the need to pass the billing address id along, since it's not editable in the settings yet. 
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/36a935784c57ef847147c5a00219c45b59e17e97

commit 36a935784c57ef847147c5a00219c45b59e17e97
Author: sebsg <sebsg@chromium.org>
Date: Wed Jan 24 22:16:56 2018

When updating credit card in settings use existing card instance.

The current code creates a new card instance and sets all the values
form the settings page. It then replaces the existing card instance in
the DB. This is dangerous because it's easy to forget some. It would
also reset all the metadata associated with this card.

More specifically in this example, the settings don't have a way to set
the billing address id. Instead of passing the value along in the
settings to set it back on save, using the existing card and updating it
is much more simple.

Bug:  800829 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ia6ce196c16295961bb6536a609ea866f953e1590
Reviewed-on: https://chromium-review.googlesource.com/860200
Commit-Queue: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Reviewed-by: Jared Saul <jsaul@google.com>
Cr-Commit-Position: refs/heads/master@{#531710}
[modify] https://crrev.com/36a935784c57ef847147c5a00219c45b59e17e97/chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
[modify] https://crrev.com/36a935784c57ef847147c5a00219c45b59e17e97/chrome/browser/extensions/api/autofill_private/autofill_private_apitest.cc
[modify] https://crrev.com/36a935784c57ef847147c5a00219c45b59e17e97/chrome/browser/extensions/api/autofill_private/autofill_util.cc
[modify] https://crrev.com/36a935784c57ef847147c5a00219c45b59e17e97/chrome/common/extensions/api/autofill_private.idl
[modify] https://crrev.com/36a935784c57ef847147c5a00219c45b59e17e97/chrome/test/data/extensions/api_test/autofill_private/test.js
[modify] https://crrev.com/36a935784c57ef847147c5a00219c45b59e17e97/third_party/closure_compiler/externs/autofill_private.js

Comment 2 by se...@chromium.org, Jan 24 2018

Status: Fixed (was: Started)

Comment 3 by se...@chromium.org, Jan 24 2018

Labels: Merge-Request-65

Comment 4 by se...@chromium.org, Jan 24 2018

Labels: -Pri-3 Pri-2
Project Member

Comment 5 by sheriffbot@chromium.org, Jan 25 2018

Labels: -Merge-Request-65 Hotlist-Merge-Approved Merge-Approved-65
Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 6 by gov...@chromium.org, Jan 26 2018

Pls merge your change to M65 branch 3325 ASAP so we can pick it up for next M65 dev release. Thank you.
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 26 2018

Labels: -merge-approved-65 merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/72895d2bb2e8cfc768afd961b6abd592c76139ec

commit 72895d2bb2e8cfc768afd961b6abd592c76139ec
Author: sebsg <sebsg@chromium.org>
Date: Fri Jan 26 18:15:43 2018

Merge-65 When updating creditcard in settings use existing card instance

The current code creates a new card instance and sets all the values
form the settings page. It then replaces the existing card instance in
the DB. This is dangerous because it's easy to forget some. It would
also reset all the metadata associated with this card.

More specifically in this example, the settings don't have a way to set
the billing address id. Instead of passing the value along in the
settings to set it back on save, using the existing card and updating it
is much more simple.

TBR=sebsg@chromium.org

(cherry picked from commit 36a935784c57ef847147c5a00219c45b59e17e97)

Bug:  800829 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ia6ce196c16295961bb6536a609ea866f953e1590
Reviewed-on: https://chromium-review.googlesource.com/860200
Commit-Queue: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Toni Barzic <tbarzic@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Reviewed-by: Jared Saul <jsaul@google.com>
Cr-Original-Commit-Position: refs/heads/master@{#531710}
Reviewed-on: https://chromium-review.googlesource.com/888263
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#116}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/72895d2bb2e8cfc768afd961b6abd592c76139ec/chrome/browser/extensions/api/autofill_private/autofill_private_api.cc
[modify] https://crrev.com/72895d2bb2e8cfc768afd961b6abd592c76139ec/chrome/browser/extensions/api/autofill_private/autofill_private_apitest.cc
[modify] https://crrev.com/72895d2bb2e8cfc768afd961b6abd592c76139ec/chrome/browser/extensions/api/autofill_private/autofill_util.cc
[modify] https://crrev.com/72895d2bb2e8cfc768afd961b6abd592c76139ec/chrome/common/extensions/api/autofill_private.idl
[modify] https://crrev.com/72895d2bb2e8cfc768afd961b6abd592c76139ec/chrome/test/data/extensions/api_test/autofill_private/test.js
[modify] https://crrev.com/72895d2bb2e8cfc768afd961b6abd592c76139ec/third_party/closure_compiler/externs/autofill_private.js

Sign in to add a comment