[dani-zilla]

Closes: #11299
Closes: #11616

RFC / Technical Design (if applicable): RFC 0005
feature-flag: push_enabled_on_inbox_by_default

Description

Enables push on the inbox by default for accounts created through the new account creation process (email and OAuth/password)

Stacked PR Info

1. (This PR) e14827e — feat(settings) default push for inbox #11535
2. 3f53815 — feat(notification-settings): Adds Default Push to QR Code Imports #11666

AI Disclosure

Select one of the following (mandatory)

• This contribution does not include any changes created or assisted by AI.
• This contribution includes changes assisted by AI.
• This contribution includes changes created by AI.

Contribution Checklist

• I have read and affirm that my contribution adheres to Mozilla’s Community Participation Guidelines
• This contribution is in Kotlin where possible
• This contribution does not use merge commits
• This contribution adheres to the existing codestyle (run gradlew spotlessCheck to check and gradlew spotlessApply to format your source code; will be checked by CI).
• This contribution does not break existing unit tests (run gradlew testDebugUnitTest; will be checked by CI).
• This contribution includes tests for any new functionality, and maintains tests for any updated functionality.
• This contribution adheres to our Engineering process (RFC/Technical Design/ADR)
• This PR has a descriptive title and body that accurately outlines all changes made, and contains a reference to any issues that it fixes (e.g. Closes #XXX or Fixes #XXX).

[github-actions]

✅ Validation Passed: All report and feature-flag labels are correctly set.

[dani-zilla]

I still have a bit of cleanup to do here. It is functional, this works as intended now and can be tested. Although besides a little bit of formatting (I notice one class has been changed that I didn't otherwise touch and want to fix that), my current concern is testing the AccountCreator. Currently, our account creator test doesn't actually test that an account is created. The test that exists basically just tests the use case can work as a holder of a function and return the right types. It does not check that, given a set of account state criteria, does it produce a full user account?

So the question because how to test this. I don't think the answer is to refactor the executed use case code to make it directly testable. We'd have to mock quite a bit and create a few fakes to store the account, it would be an undertaking that would further delay this project. On the other hand, I was hoping for some advice on if we already have any tests in place that do something like this. Rather than testing the use case directly, they test account creation defaults. I'm going to look into this more today, but would also be welcome to any pointers.

[dani-zilla]

Looks like it's in good shape. I added the feature flag test changes as well as a new test in the folder settings tests to check that the modification is called by the use case.

[wmontwe]

A UseCase should be tested.

[wmontwe]

A UseCase should be tested.

[dani-zilla]

The closest I have for this right now is the SetPushForFolderUseCase should call setPushEnabled test. Which does test that the push enabled change function gets called when the function that uses the usecase is called.

I had wondered how to do the testing for this. I had looked at the CreateAccountTest and had initially thought about doing it there, but the created account here is mocked, and the function being tested is mocked as well. Basically it tests that the function can take a lambda and that the lambda spits out an account, but the test is designed to basically always do that. I wasn't sure what was being tested.

Obviously there's something more I could do here, but I'm a bit stumped on how we can write a meaningful test for this, given how mocked the account creation process and test is already. It seems like I could create a mock account, with mock folders, and mock folder settings, and make sure that the function changes that when the use case is called directly.

[dani-zilla]

Sorry to bump this, but do you have any suggestions for testing this? I could wrap it similar to the account creation use case, but doing so won't test the actual functionality. The other option is completely mocking or faking the data it accesses. The SetPushForFolderUseCase should call setPushEnabled test does ensure that it's called, but doesn't ensure it's changing the setting. I think we should probably do this with a large mockup of the user's settings and see that it's changed, but the way this is set up, that involves doing a lot of fake data object creation. I assume that's why we did the account creation use case testing the way we did. What would be the best way forward on it?

[wmontwe]

@dani-zilla To re-request a review, click the re-request icon (the circle button) next to the reviewer's name to flag the PR for reconsideration. This is particularly important if the person commenting is not officially assigned as a reviewer. Since this fix is urgent, using @mentions may also be helpful to ensure it gets seen quickly.

[wmontwe]

Although this use case involves minimal logic, returning a value if a type is set and null otherwise, it still constitutes business logic and should be unit tested to ensure correctness.

[dani-zilla]

@wmontwe I'm not against the idea of testing it. I think, outside of the SetPushForFolderUseCase should call setPushEnabled test, directly testing if the use cases are returning the right folder and also changing that folder's settings may require some pretty massive fakes. I assume that's why the account creation unit test doesn't actually test that a new account is created. One option is to say that, because the functionality is so hard to isolate in a test environment, it shouldn't be isolated as a use case. But if included directly in the account creation function called by the account creation use case would ensure that it's only tested manually.

[wmontwe]

@dani-zilla I agree that testing the full account creation flow with realistic storage fakes would be too complex. We do not need that level of detail for the folder lookup now. GetFolderIdsForTypeUseCase can be tested using a small fake account manager and a stubbed message store.

Additionally, SetPushForFolderUseCase does not require a test because it only delegates properties to the MessageStore. If a repository is implemented in the future, we could use it directly instead of the use case provided that no additional business logic is required.

[dani-zilla]

Let me rebase. I believe Rafael fixed those dependency checks in his PR already. Those aren't from this PR.

[dani-zilla]

Yup, that was all it was! Rebase fixed the dependency guard check, ready to review 👍

[wmontwe]

LGTM