Feature/clever support#1
Conversation
# Conflicts: # Cartfile.resolved
…entication details to use a specific authentication method
ettore
left a comment
ettore
left a comment
There was a problem hiding this comment.
The general approach makes sense to me, I did not follow everything that's happening, and some changes ill require a lot of testing. I would recommend some things:
- do you think you can split this into smaller PRs?
- even better, could some of the refactors / swift-rewrites here be done in individual PR? That way we could merge this code to develop incrementally and ship it in several versions of SimplyE. This would limit possible breakages greatly.
- otherwise, this PR would definitely need to go into a feature branch and test it thoroughly before being merged down.
I think @kyles_ep should also take a look because he has more history on this code base and app than myself.
In any case thanks for these refactors!
| struct Authentication { | ||
| @objc(AccountDetailsAuthentication) | ||
| @objcMembers | ||
| class Authentication: NSObject, Codable, NSCoding { |
There was a problem hiding this comment.
Can you keep the classname the same between Swift and Objc? Easy to rename the Swift one if needed. This way the codebase would be much easier to browse IMO.
There was a problem hiding this comment.
AccountDetailsAuthentication because Authentication struct is nested inside AccountDetails struct. I wasn't able to access this struct otherwise in objc, but I might take a second look into this.
struct AccountDetails {
struct Authentication {
}
}
There was a problem hiding this comment.
I don't think it's possible to use nested types in Objective-C. And I'm afraid of name collision in case there is another Authentication type in future. I'd leave it as it currently is.
|
|
||
| - (UIView *)tableView:(UITableView *)__unused tableView viewForFooterInSection:(NSInteger)section | ||
| { | ||
| // something's wrong, it gets called every refresh cycle when scrolling |
There was a problem hiding this comment.
feel free to file a ticket for later. We add TODO: SIMPLY-123 comments to track it back.
There was a problem hiding this comment.
I managed to solve the same issue for header by adding a strong reference to returned view by storing it in view controller, but it didn't seem to work in this case. I'll need to prepare some steps to replicate and will create a ticket :)
| private lazy var _authorizationIdentifier: KeychainVariable<String> = StorageKey.authorizationIdentifier | ||
| .keyForLibrary(uuid: libraryUUID) | ||
| .asKeychainVariable(with: accountInfoLock) | ||
| private lazy var _adobeToken: KeychainVariable<String> = StorageKey.adobeToken |
There was a problem hiding this comment.
did you test this is backward compatible once a user upgrades to this code? This set of changes would require a meticulous QA effort. @kyles_ep knows more about this area of the code.
There was a problem hiding this comment.
Besides token, pin and barcode, other variables SHOULD work on par with old logic, I'll do more tests to verify it
|
just wondering, this PR is now obsolete, correct? Since it's superseded by the 2 more recent PRs I think? |
yes, that's right. The newer PRs are based on this code, but some of the things changed meantime |
2 participants
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?
Did someone actually run this code to verify it works?