Fix: issue 629 #714
Fix: issue 629 #714
Conversation
|
|
|
Thank you @aaryankotharii! I'll review and get back to you soon! |
There was a problem hiding this comment.
Thanks for this PR, @aaryankotharii, we really appreciate it! Great job with updating the tests to ensure quality! I have a few concerns with the layering and the addition of more state.
Also, please test that users who update are able to continue using the app with no interruptions. For example, regions are currently stored in UserDefaults and this PR changes it to storing on disk. Please run the app in a current App Store version, then run a version of the app with this PR and validate that the user can continuing the app without it being confused on the currently selected region.
Please ask questions if you have any, or are unclear about my comments!
aaryankotharii
force-pushed
the
fix/issue-629
branch
from
cfcae1c
to
6dec125
Compare
update
aaryankotharii
force-pushed
the
fix/issue-629
branch
from
6dec125
to
fe1f546
Compare
|
Hello @ualch9, I have made the following changes as requested above.
|
|
Thank you @aaryankotharii! Apologies for the delayed turnaround in reviewing, I'll get to this ASAP. |
|
no worries @ualch9, but there are a few migration issues I'm not sure how to address. for example, how to approach fetching custom regions, of existing users, which are stored in userdefaults ? |
Excellent question... @aaronbrethorst, do you have any insight on if custom regions are used enough for us to support a migration path? |
|
I would not worry about custom regions at this time. Thanks for checking! |
| @@ -204,17 +207,10 @@ public class RegionsService: NSObject, LocationServiceDelegate { | |||
|
|
|||
| // MARK: - Custom Regions | |||
| /// Adds the provided custom region to the RegionsService. | |||
| /// If an existing custom region with the same `regionIdentifier` exists, the new region replaces the existing region. | |||
There was a problem hiding this comment.
With this new file manager service, what happens if an existing custom region with the same regionIdentifier exists?
| static let currentRegionUserDefaultsKey = "OBACurrentRegionUserDefaultsKey" | ||
| static let regionsUpdatedAtUserDefaultsKey = "OBARegionsUpdatedAtUserDefaultsKey" | ||
| static let defaultRegionsPath = URL.applicationSupportDirectory.appendingPathComponent("default-regions.json") | ||
| static let customRegionsPath = URL.documentsDirectory.appendingPathComponent("Regions/custom-regions") |
| @@ -68,10 +70,9 @@ class RegionsServiceTests: OBATestCase { | |||
| stubRegions(dataLoader: dataLoader) | |||
There was a problem hiding this comment.
Please update the function signature comment to saved to disk instead of user defaults.
| @@ -83,26 +84,25 @@ class RegionsServiceTests: OBATestCase { | |||
| stubRegions(dataLoader: dataLoader) | |||
There was a problem hiding this comment.
Please update the function signature comment to saved to disk instead of user defaults.
|
@aaryankotharii - need anything from me in order to get this wrapped up? |
This PR migrates regions storage from UserDefaults to FileManager. Same file structure followed as mentioned in issue description.
fixes issue: #629
Code Changes
FileManagerProtocolto abstract FileManager operations for testability.FileManagerMockand updatedRegionsServiceTests.