Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add FXIOS-9059 [Microsurvey] Redux implementation for survey #20322

Merged
merged 7 commits into from
May 23, 2024
Merged

Add FXIOS-9059 [Microsurvey] Redux implementation for survey #20322

merged 7 commits into from
May 23, 2024

Conversation

cyndichin
Copy link
Contributor

@cyndichin cyndichin commented May 17, 2024 •
edited

📜 Tickets

Jira ticket
Github issue

💡 Description

Add redux implementation for survey

  • Dismiss survey + prompt
  • Navigate to privacy policy page
  • Submit survey to trigger telemetry

📝 Checklist

You have to check all boxes before merging

  • Filled in the above information (tickets numbers and description of your work)
  • Updated the PR name to follow our PR naming guidelines
  • Wrote unit tests and/or ensured the tests suite is passing
  • When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • If needed, I updated documentation / comments for complex code and public methods
  • If needed, added a backport comment (example @Mergifyio backport release/v120)

Screenshots

Simulator.Screen.Recording.-.iPhone.15.-.2024-05-20.at.14.19.51.mp4

@cyndichin cyndichin force-pushed the cc/FXIOS-9059_add-redux-for-survey branch 2 times, most recently from 42227dc to 48fffbe Compare May 20, 2024 16:44
@cyndichin cyndichin requested a review from OrlaM May 20, 2024 16:51
@mobiletest-ci-bot
Copy link

mobiletest-ci-bot commented May 20, 2024 •
edited

Warnings
⚠️ Variable 'crossCircleFill' in Medium is out of alphabetical order.
⚠️ Variable 'cross' in Medium is out of alphabetical order.
Messages
📖 Project coverage: 32.5%
📖 Edited 22 files
📖 Created 6 files

Client.app: Coverage: 31.17

File Coverage
MicrosurveyPromptState.swift 86.67% ✅
BrowserViewController.swift 4.34% ⚠️
BrowserCoordinator.swift 81.06% ✅
MicrosurveyAction.swift 100.0% ✅
MicrosurveyCoordinator.swift 100.0% ✅
MicrosurveyViewController.swift 75.95% ✅
MicrosurveyTableViewCell.swift 0.0% ⚠️
MicrosurveyMiddleware.swift 32.5% ⚠️
ActiveScreenAction.swift 100.0% ✅
MicrosurveyTableHeaderView.swift 0.0% ⚠️
BrowserNavigationHandler.swift 100.0% ✅
MicrosurveyPromptMiddleware.swift 55.32% ✅
MicrosurveyPrompt.swift 0.0% ⚠️
AppState.swift 90.0% ✅
MicrosurveyConfirmationView.swift 0.0% ⚠️
MicrosurveyState.swift 100.0% ✅
MicrosurveyTableView.swift 50.0% ⚠️
ActiveScreenState.swift 89.0% ✅

Generated by 🚫 Danger Swift against b1ef11c

OrlaM
OrlaM approved these changes May 21, 2024
View reviewed changes
Copy link
Contributor

@OrlaM OrlaM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM so far, will take another look once it's out of draft.

You don't need to break this one up but in future try to keep your PR's small. Sometimes a large one is unavoidable but this one could have been a few eg:

  • The set up code (subscribing to redux, the initial state etc)
  • The coordinator logic
  • Each use case could be it's own PR (close, submit, tab notice)
    It doesn't need to be that many PRs just some suggestions for some of the boundaries it could be broken up along.

@cyndichin
Copy link
Contributor Author

LGTM so far, will take another look once it's out of draft.

You don't need to break this one up but in future try to keep your PR's small. Sometimes a large one is unavoidable but this one could have been a few eg:

  • The set up code (subscribing to redux, the initial state etc)
  • The coordinator logic
  • Each use case could be it's own PR (close, submit, tab notice)
    It doesn't need to be that many PRs just some suggestions for some of the boundaries it could be broken up along.

Thanks Orla! I'll keep that in mind for future PRs, also happy to break this one up if its easier to review as well. Wanted to use this PR as a discussing point for our meeting later, so will let you know when its out of draft and ready to review.

cyndichin
cyndichin commented May 21, 2024
View reviewed changes
firefox-ios/Client/Frontend/Browser/BrowserViewController/Views/BrowserViewController.swift Outdated
// TODO: FXIOS-8895: Create Microsurvey Modal View
print("CYN - FXIOS-8895: Create Microsurvey Modal View")
// CYN: Torn between passing model and initializing the mobile messaging in the middleware
guard let model = state.microsurveyState.model else { return }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add logging

@cyndichin cyndichin force-pushed the cc/FXIOS-9059_add-redux-for-survey branch from 8293c9e to 2b92225 Compare May 21, 2024 16:55
@cyndichin cyndichin force-pushed the cc/FXIOS-9059_add-redux-for-survey branch from 81f5c9d to 4e5bdc1 Compare May 21, 2024 17:08
@cyndichin
Copy link
Contributor Author

Middleware Tests will be added in a separate PR

@cyndichin cyndichin marked this pull request as ready for review May 21, 2024 17:24
@cyndichin cyndichin requested review from a team as code owners May 21, 2024 17:24
@cyndichin cyndichin requested review from jjSDET and OrlaM May 21, 2024 17:24
@cyndichin
Copy link
Contributor Author

@DanielDervishi @tusharC95 this PR is related to the microsurvey demo from the grooming meeting. Feel free to let me know if you have any questions. This PR should have been broken up, so its a bit big, but let me know if you would like a live walkthrough to clarify anything!

@cyndichin cyndichin force-pushed the cc/FXIOS-9059_add-redux-for-survey branch from 4e5bdc1 to b1ef11c Compare May 22, 2024 13:13
isabelrios
isabelrios approved these changes May 23, 2024
View reviewed changes
Copy link
Contributor

@isabelrios isabelrios left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looking at the XCUITest part

@isabelrios
Copy link
Contributor

Smoketest3 had a flaky test that passed in a re-try, general bitrise is ✅

@cyndichin cyndichin merged commit d0d8c24 into main May 23, 2024
9 of 10 checks passed
@cyndichin cyndichin deleted the cc/FXIOS-9059_add-redux-for-survey branch May 23, 2024 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isabelrios isabelrios isabelrios approved these changes

@OrlaM OrlaM OrlaM approved these changes

@jjSDET jjSDET Awaiting requested review from jjSDET jjSDET is a code owner automatically assigned from mozilla-mobile/fxios-automation-1

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cyndichin @mobiletest-ci-bot @isabelrios @OrlaM