Application Refactor for Merging OpenEBooks #1070
Conversation
# Conflicts: # Simplified/NYPLCatalogNavigationController.m # Simplified/NYPLCatalogUngroupedFeedViewController.m # Simplified/NYPLConfiguration.h # Simplified/NYPLConfiguration.m # Simplified/NYPLHoldsNavigationController.m # Simplified/NYPLMyBooksNavigationController.m # Simplified/NYPLMyBooksViewController.m # Simplified/NYPLReaderSettings.m # Simplified/NYPLReaderSettingsView.m # Simplified/NYPLReaderTOCViewController.m # Simplified/NYPLReaderViewController.m
ghost
requested review from
OpenEbooks-Info.plist
Outdated
| <key>NSCameraUsageDescription</key> | ||
| <string>Open eBooks requires access to the camera in order to scan your library card.</string> | ||
| <key>NSLocationWhenInUseUsageDescription</key> | ||
| <string>This service is available to New York State residents only. In order to determine your location, please allow access</string> |
There was a problem hiding this comment.
Presumably you need to provide a value for this string, but this particular string is inaccurate for Open eBooks. If you do need a value for this string, it would be better to have the equivalent of "this can't happen" so we can treat it as a bug if it ever shows up.
In the medium term we'll hopefully be removing the native card creator code and this can go away when that does.
There was a problem hiding this comment.
Can confirm they require this string even if we don't actually use location services. As per your suggestion, how about "Oops! Looks like OpenEBooks is accidentally requesting location services. Please report a bug!"
There was a problem hiding this comment.
Since there's no way to submit a bug within the iOS interface that I can find, I vote "Open eBooks never tracks or uses your location. Please take a screenshot and email it to info@openebooks.net so we can fix this error."
|
Just so we have a link, this is for https://jira.nypl.org/browse/SIMPLY-2062. Obviously I'm not qualified to review this whole thing but what I do understand brings up one major concern. The SimplyE mobile app asks for permission to check the patron's location, and the Open eBooks mobile app shouldn't ask for that permission. I noticed that you've got strings talking about the mobile permission in some kind of Open eBooks resource file, and I want to make sure that we don't build an app that asks for a new permission we don't need, potentially freaking out patrons. |
|
This PR was open more than 5 months ago and SimplyE has changed a lot since then. For instance the app delegate has been updated, but I see here it has been rewritten in swift. So we would need to integrate all those changes in. Same thing for Accounts.swift, NYPLConfiguration and probably others. I also see large sections of code being removed from Navigation controllers, but I don't know why. So I don't think I have enough context to review this. We should rebase this branch but since so much time has passed we would really need to revisit all these changes carefully. Otherwise we'll be in regressions hell. I would leave the Swift rewrites outside of this PR. The reason is it's extremely hard to review changes between 2 classes in different languages, and on top of that across different apps. We can probably use some of the code you wrote here and do those changes individually in a separate PR, and merge those first. Or, do the swift rewrite after the merge. The goal here should be to have as few changes as possible in order to minimize merge conflicts and more importantly regressions. |
- Don't add account selection button (top-left nav item in Catalogs, MyBooks, Holds) for OE. - Refactor migrations for SE and OE. Add migration for AdobeDRM user/device IDs. - Add preprocessor macros to identify SE / OE. - Add strings for OE Tutorial screens. - Add OE URLs in configuration file. - Refactor Settings classes for OE in Swift. SE still uses old implementation. - Rename OE icon to match SE's equivalent and ease launch process set-up. - Add generic VC presentation utility. Much of this code was taken from old PR for merging OE in SE: #1070
New classes taken from original OE PR: #1070 This leaves the SimplyE part as-is
New classes taken from original OE PR: #1070 This leaves the SimplyE part as-is
|
Closing since we shipped the refactored OE. |
What's this do?
[Description of what this PR does goes here]
Why are we doing this? (w/ JIRA link if applicable)
[Quick blurb about why the code is needed and Jira link goes here / Do these changes meet the business requirements of the story?]
How should this be tested? / Do these changes have associated tests?
[Description of any tests that need to be performed once merged goes here]
Dependencies for merging? Releasing to production?
[Description of any watchouts, dependencies, or issues we should be aware of goes here]
Has the application documentation been updated for these changes?