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.

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

[MBL-59] Fix personal data export. #1836

Merged
merged 4 commits into from Aug 2, 2023
Merged

[MBL-59] Fix personal data export. #1836

merged 4 commits into from Aug 2, 2023

Conversation

ifosli
Copy link
Contributor

@ifosli ifosli commented Jul 24, 2023

📲 What

Update ExportDataEnvelope State types to match the types defined by the server. These types were changed on the server, but our app was not updated to handle the new values. Also update related logic to be more readable.

🤔 Why

Without this change, users are unable to request their personal data from the iOS app (unless they've previously successfully requested data before).

🛠 How

  • Update the State enum to match current fields defined by the server.
  • Use canRequestData helper everywhere it can be used and update method to return true if the state is expired, none, or failure.
  • Use server-defined processing states (ie assembling, assembled, and uploading) to determine if processing text/indicator should be shown.
  • Rename showPreparingDataAndCheckBackLaterText to preparingDataAndCheckBackLaterTextHidden to reflect how the field is used and to match current naming conventions.
  • Update some tests to set the state to none instead of expired, since that's now the default for users who haven't requested their data before.
  • Add unknown state with corresponding test by parsing the server state as a string and manually translating it to the enum (using unknown if we don't find a match). This isn't a nice general solution, but since we're planning to update this to GraphQL soon, it's good enough until then.

👀 See

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-07-24.at.12.22.59.mp4

✅ Acceptance criteria

  • Requesting personal data works, including for accounts that have never requested personal data before.

⏰ TODO

  • I would like to have a default state (ex unknown) that will be used if the state provided by the server can't be parsed, but I'm not sure how to do this.

@ifosli ifosli self-assigned this Jul 24, 2023
@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #1836 (c57f104) into main (7958ce3) will decrease coverage by 0.01%.
The diff coverage is 67.44%.

@@            Coverage Diff             @@
##             main    #1836      +/-   ##
==========================================
- Coverage   84.54%   84.53%   -0.01%     
==========================================
  Files        1273     1274       +1     
  Lines      115190   115216      +26     
  Branches    30656    30666      +10     
==========================================
+ Hits        97387    97400      +13     
- Misses      16734    16748      +14     
+ Partials     1069     1068       -1     
Files Changed Coverage Δ
KsApi/models/ExportDataEnvelope.swift 0.00% <0.00%> (ø)
KsApi/models/lenses/ExportDataEnvelopeLenses.swift 0.00% <0.00%> (ø)
...y/Views/Cells/SettingsPrivacyRequestDataCell.swift 86.95% <100.00%> (ø)
.../ViewModels/SettingsRequestDataCellViewModel.swift 91.95% <100.00%> (+1.47%) ⬆️
...Models/SettingsRequestDataCellViewModelTests.swift 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

Few clean up things, and if you want a default case for anything that doesn't fit the existing enum values use default as your final case.

enum Fruit {
case mango
case apple
case banana
case kiwi
case pear
}

switch fruit {
case .apple:
 print("only prints apple")
case .banana:
 print("only prints banana")
case .mango:
 print("only prints mango")
default:
  print("prints for all other fruit")
}

All info on Swift enums here.

I ran a quick smoke test on simulator - it seems to work just like in the video.

Developer can own the acceptance criteria, if there's specific "Developer Testing" you want done, just add that as a section. Example.

Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

gtg once cleanup and "unknown" state suggestions are done. requesting data works as expected on my end 👍

Update `ExportDataEnvelope` `State` types to align with the types defined by the server. Update related logic to be more readable. Also update some test cases to default to `none` instead of `expired`, now that `none` is the default state for users that have never requested their data before.
Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

good work! Just re-assign once ready for review again (you were probably already going to do this, I just pre-empted a little here.)

@msadoon
Copy link
Contributor

msadoon commented Jul 26, 2023

Run bin/format.sh . to fix format issues.

@msadoon
Copy link
Contributor

msadoon commented Jul 26, 2023

Oh and add a milestone - 5.10.0 please.

Update the ExportDataEnvelope `state` enum to rely on an underlying `stateString`. This allows a default `unknown` value to be used if the state from the server is missing or doesn't match a value in the enum.
@ifosli
Copy link
Contributor Author

ifosli commented Jul 27, 2023

Updated to my proposed easy way of handling unknown states. Please take another look if you haven't already (there aren't changes since yesterday, I just wanted to make sure the tests would pass) and let me know if you have any questions/concerns!

Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

Yea good call here, I can see this being a good general strategy for parsing enums from the server. Let's keep doing it.

KsApi/models/ExportDataEnvelope.swift Outdated Show resolved Hide resolved
@ifosli ifosli merged commit 6266d87 into main Aug 2, 2023
7 checks passed
@ifosli ifosli deleted the MBL-59 branch August 2, 2023 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants