Support IHateMoney QR codes#93
Conversation
magshee
force-pushed
the
fix/i-hate-money-qr-codes
branch
from
25a839f to
ba76d31
Compare
|
Thank you very much for the extra effort, I'm happy to see it working now! Unfortunately, during testing I finally got "the too many requests error" with my own cloud (no matter your new feature changes or my previous attempts to fix #81). I'd like to look into this first before continuing reviewing and testing you changes. A bit tight on capacity right now, but I'll try my best! |
No worries, take your time! |
|
Hi @magshee and happy WWDC! I catered to two issues seemingly more severe first and gladly none of those fixes touch any file you changed, so I'll be able to review and test your code regardless of that. This is next on my list and I'm sorry it took so long, still – or even more so, I really appreciate your contribution and will try my best to merge it soon. Then it becomes time to push all the latest improvements to the AppStore. Liquid Glass will also be mandatory from Xcode 27 onwards, so that's another small exercise before getting the update out there. But also a great opportunity for some UI fixes I honestly feel more at home with. 😄 |
| return Project(name: apiProject.name, password: "", token: invite.token, backend: ProjectBackend.iHateMoney, url: URL(string: invite.url)!, projectId: apiProject.id) | ||
| } | ||
|
|
||
| func fetchInvite(_ invite: InviteData?) async throws { |
There was a problem hiding this comment.
Methode erzeugt nur einen Log, sinnvoll im Produktivcode?
| } | ||
|
|
||
| extension URL { | ||
| func decodeIHateMoenyString() -> ProjectDataWithToken? { |
There was a problem hiding this comment.
Tippfehler bei Money lieber frühzeitig korrigieren
There was a problem hiding this comment.
Und, falls Funktion erhalten bleiben soll, host mit let anstatt var? :)
| @@ -44,11 +44,7 @@ class ProjectManager: ObservableObject { | |||
|
|
|||
| func openedByURL(url: URL) { | |||
| let data = url.decodeCospendString() | |||
There was a problem hiding this comment.
Gehört hier dann noch der Fall für IHM hin? decodeIHateMoneyString()
| self.isTestingSubject.send(.failure) | ||
| } | ||
| } | ||
| // NetworkService.shared.testProject(project) |
There was a problem hiding this comment.
Ich denke auch, der kommentierte Code darf bei dieser Gelegenheit gerne raus.
There was a problem hiding this comment.
Die drei extension URL Blöcke könnten zusammengefasst werden und die expliziten elementweisen Initialisierungen für ProjectDataWithToken / ProjectDataWithPassword dürften automatisch durch den Compiler generiert werden
There was a problem hiding this comment.
Viele Tests enden erfolgreich, werden aber ggf. mit if let still übersprungen. Was war der Gedanke dahinter? Das scheint mir die Effektivität der jew. Tests zu mindern. Wie wäre XCTUnwrap?
func testDecodeQRCode_httpsSchemeRoutesToMoneyBusterDecoder() tested den decoder nun nicht mehr, sondern ruft direkt die IHM decode Funktion auf (und dafür gibt es schon einen Test).
InteractionEngineer
left a comment
InteractionEngineer
left a comment
There was a problem hiding this comment.
Thank you for your patience! I am thrilled about the new functionalities, thanks for your work! I've had a look on the changes and left some comments that came to my mind – sorry, I fell back to German with those.
Regarding MoneyBuster: Unfortunately I don't have access to Android systems and assumed it must be a deep-link, like in iOS that opens within an app directly. If you see MoneyBuster works differently, please feel free to adjust our code.
One last important bit: Changing from token to projectId, or rather adding it and alter the previous token duty might / will affect historic app data. Were you able to test migration and see effects on projects added before your update? I confirmed it'll work really well with a blank database.
2 participants
Closes #90
This required a little more work than I had expected, but I’m really happy to made progress in this PR.
Here are the changes:
PROJECT_TOKENthat can be used a as Bearer token for authentication. For some reason, the project identifier was stored in a variable called token. I decided to store the project identifier in a new field and use the token variable for the IHateMoney token.Thoughts for further consideration:
AddProjectQRViewModelwere already commented. If they are not used/required, maybe consider removing themEventually there are still things in here that can be improved, looking forward to your feedback!