[FeatureToggle] Add new autofill config for partial save trigger #724

dbajpeyi · 2024-12-17T14:44:46Z

Reviewer: @GioSensation
Asana: https://app.asana.com/0/72649045549333/1209005356472261/f

Description

Since Windows and Android are blocked out from regular autofill releases since https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/16.0.0 because, the implementation for partial saves is not ready yet. This PR adds the implementation behind a feature toggle partial_form_saves which will be disabled by default. This way we can continue releasing improvements on Android/Windows without autofill regression.

Added tests for each platform with the feature toggle,
Reverted form and formatters tests to match https://github.com/duckduckgo/duckduckgo-autofill/pull/702/files

Steps to test

Use deviantart.com to test the login and reset password scenario.

dbajpeyi · 2024-12-18T10:33:38Z

src/Form/Form.test.js

@@ -84,8 +84,8 @@ describe('Test the form class reading values correctly', () => {
    <input type="password" value="" autocomplete="new-password" />
    <button type="submit">Sign up</button>
 </form>`,
-            expHasValues: true,
-            expValues: { credentials: { username: 'testUsername' } },
+            expHasValues: false,


Tests (this and the others) reverted to match https://github.com/duckduckgo/duckduckgo-autofill/pull/702/files so I am confident we're good here.

https://github.com/duckduckgo/duckduckgo-autofill/pull/702/files#diff-6cc15f20dfd09f594b38d6cee7b2efe4b4882efd677b3769a03734403b427aeaR87-R99

dbajpeyi · 2024-12-18T10:35:14Z

src/Form/formatters.test.js

            const values = prepareFormValuesForStorage({
                credentials: { username: '[email protected]' },
                // @ts-ignore
                creditCards: {},
                // @ts-ignore
                identities: {},
            });
-            expect(values.credentials?.username).toBe('[email protected]');
+            expect(values.credentials).toBeUndefined();


Same as above: https://github.com/duckduckgo/duckduckgo-autofill/pull/702/files#diff-60dc4886b658f214d1d00b40a35ac1a859c2df894db27ca9338de1e1a8253549R64-R72

dbajpeyi · 2024-12-18T10:52:18Z

@GioSensation Looking at the test cases is a good start!

dbajpeyi · 2024-12-18T10:57:24Z

integration-test/helpers/mocks.webkit.js

@@ -244,6 +244,7 @@ export function createWebkitMocks(platform = 'macos') {
                                    password_generation: true,
                                    credentials_saving: true,
                                    inlineIcon_credentials: true,
+                                    partial_form_saves: true,


Setting this to true means iOS and macOS tests are working as expected, no change needed!

GioSensation

Just a nit plus a suggestion to use the expected future value on Android integration tests. Otherwise, it's looking good. Thanks for updating as suggested on MM.

GioSensation · 2024-12-18T11:50:20Z

integration-test/tests/save-prompts.android.spec.js

@@ -56,6 +56,7 @@ test.describe('Android Save prompts', () => {
                        androidStringReplacements({
                            featureToggles: {
                                credentials_saving: true,
+                                partial_form_saves: false,


In the future this will also be set to true, so I'm inclined to flip this right away and keep the tests working as they are expected to when the feature is actually enabled. Then I see below a test that checks that the feature flag is respected, so we're covered.

Good point, actually the feature toggle is already set to true now https://github.com/duckduckgo/privacy-configuration/pull/2593/files

GioSensation · 2024-12-18T11:51:41Z

src/DeviceInterface/InterfacePrototype.js

+            Boolean(values?.credentials?.username) &&
+            Boolean(this.settings.featureToggles.partial_form_saves);


Seems like we don't need to cast to a Boolean. Certainly not for the last one. Let's simplify. 🙂

Personally I don't prefer when the output type is not explicit enough. My reason for doing it this way was that we expect the values to act like boolean.

But I could've missed something, not arguing there 😄

It's getting philosophical, don't want to argue. My stance is: the language does that Boolean cast for you. You're just adding noise in the source code. Arguably, sometimes it can be useful, but casting a feature flag is just noise IMO. Anyway, don't want to block on this.

Kept Boolean in the username part because during debugging shouldTriggerPartialSave becomes an empty one, and has caught me off guard a couple of times. The last one is fair to change, updated :)

Thanks for the comments.

@dbajpeyi

Task/Issue URL: https://app.asana.com/0/1209009001482464/1209009001482464 Autofill Release: https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/16.1.0 ## Description Updates Autofill to version [16.1.0](https://github.com/duckduckgo/duckduckgo-autofill/releases/tag/16.1.0). ### Autofill 16.1.0 release notes ## What's Changed * [DeviceInterface] Call closeautofillparent only after openManage calls by @dbajpeyi in duckduckgo/duckduckgo-autofill#718 * [CredentialsImport] Call credentials flow call first, and then close parent by @dbajpeyi in duckduckgo/duckduckgo-autofill#723 * [FeatureToggle] Add new autofill config for partial save trigger by @dbajpeyi in duckduckgo/duckduckgo-autofill#724 **Full Changelog**: duckduckgo/duckduckgo-autofill@16.0.0...16.1.0 ## Steps to test This release has been tested during autofill development. For smoke test steps see [this task](https://app.asana.com/0/1198964220583541/1200583647142330/f). Co-authored-by: dbajpeyi <[email protected]>

dbajpeyi added 3 commits December 16, 2024 19:49

fix: call credentials flow call first, and then close parent

8f8c68b

chore: move partial save behind feature toggle

b96ed0a

Merge branch 'main' into dbajpeyi/chore/partial-save-feature-toggle

31fd22f

dbajpeyi marked this pull request as draft December 17, 2024 19:41

dbajpeyi marked this pull request as ready for review December 18, 2024 10:05

dbajpeyi changed the title ~~[AutofillConfig] Add new autofill config for partial save trigger~~ [FeatureToggle] Add new autofill config for partial save trigger Dec 18, 2024

dbajpeyi requested a review from GioSensation December 18, 2024 10:12

dbajpeyi force-pushed the dbajpeyi/chore/partial-save-feature-toggle branch from 75c0d19 to 5feeeb0 Compare December 18, 2024 10:29

dbajpeyi commented Dec 18, 2024

View reviewed changes

dbajpeyi force-pushed the dbajpeyi/chore/partial-save-feature-toggle branch 4 times, most recently from b0a6fbc to c887cf9 Compare December 18, 2024 10:51

dbajpeyi commented Dec 18, 2024

View reviewed changes

dbajpeyi force-pushed the dbajpeyi/chore/partial-save-feature-toggle branch from c887cf9 to 545d9aa Compare December 18, 2024 14:22

refactor: add feature toggle to support apple-only

29a049b

dbajpeyi force-pushed the dbajpeyi/chore/partial-save-feature-toggle branch from 545d9aa to 29a049b Compare December 18, 2024 14:24

GioSensation approved these changes Dec 18, 2024

View reviewed changes

chore:PR comments

24ecd5b

dbajpeyi merged commit 47c26dc into main Dec 18, 2024
1 check passed

dbajpeyi deleted the dbajpeyi/chore/partial-save-feature-toggle branch December 18, 2024 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FeatureToggle] Add new autofill config for partial save trigger #724

[FeatureToggle] Add new autofill config for partial save trigger #724

dbajpeyi commented Dec 17, 2024 •

edited

Loading

dbajpeyi Dec 18, 2024 •

edited

Loading

dbajpeyi Dec 18, 2024

dbajpeyi Dec 18, 2024

dbajpeyi commented Dec 18, 2024

dbajpeyi Dec 18, 2024 •

edited

Loading

GioSensation left a comment

GioSensation Dec 18, 2024

dbajpeyi Dec 18, 2024

GioSensation Dec 18, 2024

dbajpeyi Dec 18, 2024 •

edited

Loading

dbajpeyi Dec 18, 2024

GioSensation Dec 18, 2024

dbajpeyi Dec 18, 2024 •

edited

Loading

		Boolean(values?.credentials?.username) &&
		Boolean(this.settings.featureToggles.partial_form_saves);

[FeatureToggle] Add new autofill config for partial save trigger #724

[FeatureToggle] Add new autofill config for partial save trigger #724

Conversation

dbajpeyi commented Dec 17, 2024 • edited Loading

Description

Steps to test

dbajpeyi Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

dbajpeyi Dec 18, 2024

Choose a reason for hiding this comment

dbajpeyi Dec 18, 2024

Choose a reason for hiding this comment

dbajpeyi commented Dec 18, 2024

dbajpeyi Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

GioSensation left a comment

Choose a reason for hiding this comment

GioSensation Dec 18, 2024

Choose a reason for hiding this comment

dbajpeyi Dec 18, 2024

Choose a reason for hiding this comment

GioSensation Dec 18, 2024

Choose a reason for hiding this comment

dbajpeyi Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

dbajpeyi Dec 18, 2024

Choose a reason for hiding this comment

GioSensation Dec 18, 2024

Choose a reason for hiding this comment

dbajpeyi Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

dbajpeyi commented Dec 17, 2024 •

edited

Loading

dbajpeyi Dec 18, 2024 •

edited

Loading

dbajpeyi Dec 18, 2024 •

edited

Loading

dbajpeyi Dec 18, 2024 •

edited

Loading

dbajpeyi Dec 18, 2024 •

edited

Loading