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

FXIOS-731 ⁃ HTTP HEAD etags to cut down on data requests from Breach Alerts #7100

Merged
merged 14 commits into from
Aug 14, 2020

Conversation

vphong
Copy link
Contributor

@vphong vphong commented Aug 6, 2020

Relates to #6960. Caches breach data using FileManager; creates a new BreachAlertsClient function fetchEtag that specifically gets the Etag from the header of a given endpoint.

When loadBreaches is called:

  • If a breaches.json file exists:
    1. loads data from that file
      • if at least 3 days have passed since the file was last updated from the internet as saved in Profile.prefs
        1. refetch the data
        2. save the etag and date from the endpoint's header in user's Profile.prefs
        3. delete the old breaches.json to avoid data collisions and save the new data as breaches.json
      • else update the date and use data from local file
    2. decode the data and hand it off to a completion block
  • else:
    • fetch data from the Monitor endpoint
    • save the etag and date from the endpoint's header in user'sProfile.prefs
    • save data as breaches.json

┆Issue is synchronized with this Jira Task

@vphong vphong changed the base branch from main to vphong/breachalerts August 6, 2020 20:48
@data-sync-user data-sync-user changed the title Use headers to cut down on data requests from Breach Alerts FXIOS-731 ⁃ Use headers to cut down on data requests from Breach Alerts Aug 6, 2020
@mozillamobilebot
Copy link

mozillamobilebot commented Aug 6, 2020

SwiftLint found issues

Warnings

File Line Reason
LoginListViewModel.swift 30 Prefer empty collection over optional collection. (discouraged_optional_collection)
LoginListViewModel.swift 101 Prefer empty collection over optional collection. (discouraged_optional_collection)

Sentry check list

  • I understand that only .fatal events will be reported on release

  • The message param contains a string that will not create multiple events

Generated by 🚫 Danger

@garvankeeley garvankeeley changed the title FXIOS-731 ⁃ Use headers to cut down on data requests from Breach Alerts FXIOS-731 ⁃ HTTP HEAD etags to cut down on data requests from Breach Alerts Aug 7, 2020
@vphong vphong marked this pull request as ready for review August 7, 2020 15:55
Copy link
Contributor

@garvankeeley garvankeeley left a comment

Choose a reason for hiding this comment

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

Looking great, almost there. This is inherently tricky stuff, so I might be extra-pendantic, so please bear with me!
I'll ping you on slack.

Client/Frontend/Login Management/BreachAlertsClient.swift Outdated Show resolved Hide resolved
Client/Frontend/Login Management/BreachAlertsClient.swift Outdated Show resolved Hide resolved
Client/Frontend/Login Management/BreachAlertsClient.swift Outdated Show resolved Hide resolved
Client/Frontend/Login Management/BreachAlertsClient.swift Outdated Show resolved Hide resolved
self.breachAlertsClient.fetchData(endpoint: .breachedAccounts) { maybeData in
guard let data = maybeData.successValue else {
completion(Maybe(failure: BreachAlertsError(description: "failed to load breaches data")))
if FileManager.default.fileExists(atPath: self.cacheURL.path) {
Copy link
Contributor

Choose a reason for hiding this comment

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

guard instead of if pls

Copy link
Contributor Author

@vphong vphong Aug 7, 2020

Choose a reason for hiding this comment

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

This if is part of an if/else so is guard still necessary? else:

} else {
// first time loading breaches, so fetch as normal and hand off
self.fetchAndSaveBreaches(completion)
}
}

Not sure if guard would make it more readable but open to suggestions of course

Client/Frontend/Login Management/BreachAlertsManager.swift Outdated Show resolved Hide resolved
Client/Frontend/Login Management/BreachAlertsManager.swift Outdated Show resolved Hide resolved
Client/Frontend/Login Management/BreachAlertsClient.swift Outdated Show resolved Hide resolved
Client/Frontend/Login Management/BreachAlertsManager.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@kaylagalway kaylagalway left a comment

Choose a reason for hiding this comment

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

All good on my end. Thanks for the changes- great work here!

return nil
}
return URL(fileURLWithPath: path, isDirectory: true).appendingPathComponent("breaches.json")
}()
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Looks good!


self.breaches = decoded
// remove for release
self.breaches.insert(BreachRecord(
Copy link
Contributor

Choose a reason for hiding this comment

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

remove the mocks pls

@vphong vphong merged commit ab66183 into vphong/breachalerts Aug 14, 2020
@vphong vphong deleted the vphong/breachalerts-cache branch August 14, 2020 22:40
vphong added a commit that referenced this pull request Aug 17, 2020
* Base BreachAlertsManager with loadBreaches() and compareToBreaches() (#6696)

* Revert "Base BreachAlertsManager with loadBreaches() and compareToBreaches() (#6696)" (#6698)

This reverts commit 6110fd0.

* Base BreachAlertsManager class with loadBreaches() + compare() (#6699)

* Test BreachAlertsManager.loadBreaches() and compareToBreaches() (#6715)

* Refactor LoginListViewController to MVVM (#6779)

* Refactor LoginListViewController to MVVM (again) (#6871)

* Refactor LoginListViewController to MVVM
* New PR made because previous changes were not merged correctly

* Test LoginsList-related refactored classes (#6897)

* LoginsList test stubs

* fix test db deletion

* query test

* more view model tests

* test on properties set by setLogins

* open logins db

* LoginListSelectionHelper tests

* VM helper tests

* headers

* Delete LoginListDataSourceHelper.swift

* queue-ify loadLogins

* queue-ify tests

* computeSectionsFromLogin + revert queues

* remove tests requiring loadLogins to be called + cleanup

* review changes

* cleanup after renaming

* Incorporate BreachAlertsManager in to LoginsListViewController (#6934)

* Add breach alert icon to Logins List cells and display if item is breached (#6992)

* basic BreachAlerts surfacing

* UI update for breaches

* record breach IndexPaths

* rewrite findUserBreaches

* findUserBreaches refinement

* reload table after breaches are loaded to update UI

* rework cell reload method

* review changes

* rudimentary asset display

* positioning using custom cell

* hide icon and show when needed

* refinement

* positioning, vector size, additional mock data

* cleanup

* bug cleanup/polish

* convert array to set for performance

* forEach reloadRows optimization

* margins + breach reload optimization

Co-authored-by: Garvan Keeley <[email protected]>
Co-authored-by: Nishant Bhasin <[email protected]>

* Fix margins within LoginListTableViewCell (#7022)

* Update LoginListTableViewCell.swift and add custom stack views + containers

* FXIOS-710 ⁃ Create breach details view (#7041)

* stubbing

* stack views

* convert [BreachRecord] to set; basic view population

* formatting + string population

* UI polish

* commenting

* button implementation

* breach link malformation handling

* VoiceOver support

* more elegant url handling

* use delegate

* better delegation

* setup function for breach detail view

* re-add long login

* tests + comments + better accessibility support

* spacing + mock data redo

* UX updates (#7127)

* FXIOS-731 ⁃ HTTP HEAD etags to cut down on data requests from Breach Alerts (#7100)

* first pass

* etag & last accessed date integration

* test compatibility

* better completion handling

* cleanup

* review 1

* review 2

* Update BreachAlertsManager.swift

* formatting/test reformation

* fix logic errors

* Update BreachAlertsManager.swift

* fix logic

* remove mock data

* Lint

Co-authored-by: Garvan Keeley <[email protected]>
Co-authored-by: isabelrios <[email protected]>
Co-authored-by: isabel rios <[email protected]>
Co-authored-by: Daniela Arcese <[email protected]>
Co-authored-by: Vlad Dramaretskyi <[email protected]>
Co-authored-by: Haris Zaman <[email protected]>
Co-authored-by: noorhashem <[email protected]>
Co-authored-by: Edouard Oger <[email protected]>
Co-authored-by: Kayla Galway <[email protected]>
Co-authored-by: Nishant Bhasin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants