Apple Pay - Donate form API calls #4619

tonisevener · 2023-09-09T02:36:37Z

Phabricator:
https://phabricator.wikimedia.org/T345847

Notes

This PR adds the ability to do a few API calls for the donate form:

Fetching the donate config file.
Calling FR Tech's payment networks endpoint. (Still in development)
Calling FRTech's submit payment endpoint. (Still in development)

Since these are pretty basic calls*, I thought this was a good chance to spin up a new service in WKData and avoid the loop back to the app for networking. This new service is called WKBasicService.

*FR Tech endpoints may eventually need an API key, but I don't have those details yet.

I am also fetching the config and payment methods endpoints at the same time as our current announcements endpoint fetch. I may modify it slightly in the future to trigger once our future campaign config call goes through, to reduce unnecessary calls.

Note: b3aea1b will come in handy while the FR Tech endpoints are still in development. So no need to revert before merging, but the goal is to eventually revert this commit.

Test Steps

In the app and trigger an announcements fetch (generally just a background/foreground, then pull to refresh is needed on Explore). Confirm config values are printed out.
Confirm unit tests pass.

+ plus rename URL extension file

…s for posting

…ncementsContentSource

staykids

There's a big chunk of work to review here. Here's just very superficial stuff from my first pass. Will dig deeper in next pass.

staykids · 2023-09-26T00:34:07Z

WKData/Sources/WKData/Extensions/Int+HttpStatusCodes.swift

+import Foundation
+
+extension Int {
+    var isHttpSuccess: Bool {


I like the intention, and appreciate it only internal to WKData, but I wonder if this really needs to extend all Ints over just being in some generalized HTTP status code utility.

Good idea - I moved this into a HTTPURLResponse extension.

staykids · 2023-09-26T00:38:56Z

WKData/Sources/WKData/Extensions/URL+API.swift

+        var components = URLComponents()
+        components.scheme = "https"
+        components.path = "/w/api.php"


Would it make sense to generalize out these repeated constructions into a WMFAPIPath-ish from NSURL+WMFLinkParsing thing (BaseMediaWikiAPIPathComponents?)?

Done (I think) - let me know if it's what you had in mind!

To clarify, I think even pulling the repeated scheme would be useful. Like:

private static var baseMediaWikiAPIPathComponents: URLComponents { var components = URLComponents() components.scheme = "https" components.path = "/w/api.php" return components } static func mediaWikiAPIURL(project: WKProject) -> URL? { var components = baseMediaWikiAPIPathComponents switch project { ... }

Sorry I missed this one - will make a note to put it in a later PR. Thanks!

staykids · 2023-09-26T00:40:02Z

WKData/Sources/WKData/Models/Donate/WKDonateConfig.swift

+    public let currencyMinimums: [String: Decimal]
+    public let currencyMaximums: [String: Decimal]
+    public let currencyAmounts7: [String: [Decimal]]


I understand these currently match the API, but just wanted to mention the same note as in Slack about potentially more precise naming here if those suggestions still agree with you.

Done! I updated the mock json and the config at donate.wikimedia.org...so far I haven't changed the config at test.wikipedia.org. I will EOD today once I know FR Tech isn't testing against it.

staykids · 2023-09-26T00:41:18Z

WKData/Sources/WKData/Models/Donate/WKPaymentSubmissionResponse.swift

+    let response: Response
+}
+
+/// {"response":{"status":"Success","gateway_transaction_id":"PVLSL6JZ4NK2WN82","order_id":"1235.1"}}


staykids · 2023-09-26T00:41:41Z

WKData/Sources/WKData/Service/WKBasicService.swift

+import Foundation
+
+/// Use this service for the most basic networking service calls. It does not handle authentication.
+///


staykids · 2023-09-26T00:45:35Z

WMF Framework/FeatureFlags.swift

@@ -10,4 +10,11 @@ public struct FeatureFlags {
        return true
    }

+    public static var applePayEnabled: Bool {


Minor, but what do you think about something like applePayDonationEnabled instead? On first read applePayEnabled connoted the availability of Apple Pay itself to me, rather than our intent of Apple Pay as a donation option.

Done (twice actually, sorry). I went with the more general donorExperienceImrovementsEnabled - I'm hoping to be able to wrap up all of our improvements in the next few weeks around that for easy rollback, not just the native form.

👍🏽 new name works great, much more precise about what it is

staykids

@tonisevener More notes below

staykids · 2023-09-26T23:22:07Z

WKData/Sources/WKData/Data Controllers/WKDonateDataController.swift

+            switch result {
+            case .success(let response):
+                guard response.response.status == "Success" else {
+                    return


Missing a completion handler call in this failure case.

Great catch! Done.

staykids · 2023-09-26T23:28:57Z

WKData/Sources/WKData/Extensions/Dictionary+API.swift

+            guard
+                let encodedName = key.addingPercentEncoding(withAllowedCharacters: CharacterSet.urlQueryComponentAllowed),
+                let encodedValue = (value as? String)?.addingPercentEncoding(withAllowedCharacters: CharacterSet.urlQueryComponentAllowed) else {
+                    continue


Unsure if we want to continue our pattern of assert(...) in the new frameworks, but this may be a good candidate. Any key/value parameter pair failing to be properly URL encoded means the entire parameter dictionary is invalid, right?

WKData/Sources/WKData/Extensions/HTTPURLResponse+Extensions.swift

staykids · 2023-09-26T23:42:05Z

WKData/Sources/WKData/Service/Protocol/WKURLSession.swift

+import Foundation
+
+public protocol WKURLSession {
+    func wkDataTask(with request: URLRequest, completionHandler: @escaping @Sendable (Data?, URLResponse?, Error?) -> Void) -> WKURLSessionDataTask


My limited understanding of some of the new Swift concurrency changes (actors/actor isolation/et al) makes it unclear to me if the @Sendable here and elsewhere is needed or not.

Yeah - I am also unsure if it's entirely necessary. Since WKURLSession is meant to be a basic wrapper for easier unit testing (in WKBasicServiceTests.swift), it felt correct to directly match the signature of the URLSession method it wraps:

open func dataTask(with request: URLRequest, completionHandler: @escaping @Sendable (Data?, URLResponse?, Error?) -> Void) -> URLSessionDataTask

I'm open to alternative ways to mock this if it feels icky. This workaround came after first trying to mock via a URLSession subclass, and these types of warnings appeared - https://stackoverflow.com/questions/74406895/subclassing-urlsession-init-was-deprecated-in-ios-13-0.

staykids · 2023-09-26T23:47:55Z

WKData/Sources/WKData/Service/WKBasicService.swift

+import Foundation
+
+/// Use this service for the most basic networking service calls. It does not handle authentication.
+public final class WKBasicService: WKService {


Are you intending WKService to serve some more general purpose than effectively performing network requests? If not, WKNetworkService, WKBasicNetworkService, and WKNetworkServiceEnvironment read as far more precise type identifiers to me.

Nope, only for performing network requests. Back when I worked on wikimedia/wikipedia-ios-data#6 I was hoping that "service" would become synonymous with a class that does networking (i.e. deals with a UrlSession internally) in our minds. I felt like adding "network" in the name might feel redundant or suggest that there are other types of services that do not involve networking. Not a big deal to me though, I've been in the weeds on these classes so far so if the name change makes more sense to fresh eyes I'm happy to change it. Do you think I should add it to other related structs/enums as well? (WKMediaWikiNetworkServiceRequest, WKNetworkServiceRequestMethod, etc?)

I can certainly appreciate the desire to keep it short (WKService over WKNetworkService) but from my first read of this work it didn't quite click for me. I think I mentally expect Service as something more generalized, and there being specializations of it – like what if there's some local data service, or some string localization service? That's just my personal take!

For time sake let's leave this as-is and pick this conversation back up in the future as WKData expands in functionality – sound good?

…parameters

staykids · 2023-09-27T19:00:26Z

WKData/Sources/WKData/Service/WKBasicService.swift

+import Foundation
+
+/// Use this service for the most basic networking service calls. It does not handle authentication.
+public final class WKBasicService: WKService {


I can certainly appreciate the desire to keep it short (WKService over WKNetworkService) but from my first read of this work it didn't quite click for me. I think I mentally expect Service as something more generalized, and there being specializations of it – like what if there's some local data service, or some string localization service? That's just my personal take!

For time sake let's leave this as-is and pick this conversation back up in the future as WKData expands in functionality – sound good?

Add feature flag

76916e0

tonisevener added the Dependent PR PR is dependent on another PR - merge dependent PR first and update branch before merging label Sep 9, 2023

tonisevener changed the title ~~[HOLD] Apple Pay - Fetch payment networks and Donate form config~~ [HOLD] Apple Pay - Fetch payment networks and donate form config Sep 9, 2023

tonisevener marked this pull request as draft September 18, 2023 21:47

tonisevener mentioned this pull request Sep 18, 2023

Apple Pay - Form UI #4629

Merged

Base automatically changed from monorepo to main September 19, 2023 22:30

tonisevener added 18 commits September 22, 2023 12:56

Add json resources

85a3a07

Add WKServiceEnvironment, add methods for creating donate API URLs

0f37f7f

+ plus rename URL extension file

Move WKData protocols into Protocol subfolder, tweak service protocol…

7fbb310

…s for posting

Move feature-specific models into subfolders

e23e324

Add WKURLSession wrapper for assistance in unit testing

63e3073

Add extensions needed for basic service

3b33078

Add WKBasicService and WKBasicServiceRequest

421971c

Add WKMockURLSession and unit tests for WKBasicService

20295db

Add basicService and serviceEnvironment properties to WKDataEnvironment

deec94c

Add models for donateConfig, paymentMethods, and submitPayment API calls

bbb5b71

Add WKDonateDataController for making donation-related API calls

c315364

Add WKMockDonateBasicService and unit tests for WKDonateDataController

7ebe664

Set the correct WKDataEnvironment service environment upon launch

b49f212

Fetch donate configs when announcements are fetched

b6b436b

Add missed posting protocol conformance

cdf54df

Merge branch 'main' into apple-pay-1

fdf2151

Remove double header setting

312faba

Fix compile failure - add fetchDonateConfig helper method to WMFAnnou…

a1c1e3f

…ncementsContentSource

tonisevener force-pushed the apple-pay-1 branch 2 times, most recently from 980e910 to f5fce65 Compare September 22, 2023 20:01

tonisevener removed the Dependent PR PR is dependent on another PR - merge dependent PR first and update branch before merging label Sep 22, 2023

Better brand name handling

a82a1f8

tonisevener force-pushed the apple-pay-1 branch from f5fce65 to b018187 Compare September 22, 2023 20:15

tonisevener changed the title ~~[HOLD] Apple Pay - Fetch payment networks and donate form config~~ Apple Pay - Donate form API calls Sep 22, 2023

tonisevener marked this pull request as ready for review September 22, 2023 20:16

tonisevener requested review from a team and mazevedofs and removed request for a team September 22, 2023 20:16

tonisevener added 2 commits September 22, 2023 15:44

Remove extra extension file

f4fe16e

Temporary changes for testing

b3aea1b

tonisevener force-pushed the apple-pay-1 branch from b018187 to b3aea1b Compare September 22, 2023 20:45

tonisevener mentioned this pull request Sep 25, 2023

Apple Pay - finalizing authorization sheet and thank you toast presentation #4641

Merged

tonisevener requested review from staykids and removed request for mazevedofs September 25, 2023 19:15

staykids self-assigned this Sep 25, 2023

Merge branch 'main' into apple-pay-1

d4bdfde

staykids reviewed Sep 26, 2023

View reviewed changes

tonisevener added 7 commits September 26, 2023 13:46

PR Feedback - http response extension for status code success flag

cc2e9cd

PR Feedback - base api path components

880b5b9

PR feedback - unnecessary comments

d5b60e9

PR Feedback - rename feature flag

7da6760

PR Feedback - improve donate config key names

96a531a

Merge branch 'main' into apple-pay-1

105b610

PR Feedback - rename feature flag again

c023c78

staykids reviewed Sep 26, 2023

View reviewed changes

staykids removed their assignment Sep 26, 2023

tonisevener added 3 commits September 27, 2023 08:01

PR Feedback - missing completion handler

729ff24

PR Feedback - add assertion failure for failure to encode dictionary …

664e9f2

…parameters

Merge branch 'main' into apple-pay-1

c7453a2

staykids self-assigned this Sep 27, 2023

staykids approved these changes Sep 27, 2023

View reviewed changes

staykids removed their assignment Sep 27, 2023

staykids merged commit 9a0966f into main Sep 27, 2023

staykids deleted the apple-pay-1 branch September 27, 2023 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apple Pay - Donate form API calls #4619

Apple Pay - Donate form API calls #4619

tonisevener commented Sep 9, 2023 •

edited

Loading

staykids left a comment

staykids Sep 26, 2023

tonisevener Sep 26, 2023

staykids Sep 26, 2023

tonisevener Sep 26, 2023

staykids Sep 26, 2023

tonisevener Sep 27, 2023

staykids Sep 26, 2023

tonisevener Sep 26, 2023

staykids Sep 26, 2023

staykids Sep 26, 2023

staykids Sep 26, 2023

tonisevener Sep 26, 2023

staykids Sep 26, 2023

staykids left a comment

staykids Sep 26, 2023

tonisevener Sep 27, 2023

staykids Sep 26, 2023

tonisevener Sep 27, 2023

staykids Sep 26, 2023

tonisevener Sep 27, 2023

staykids Sep 26, 2023

tonisevener Sep 27, 2023

staykids Sep 27, 2023

staykids Sep 27, 2023

Apple Pay - Donate form API calls #4619

Apple Pay - Donate form API calls #4619

Conversation

tonisevener commented Sep 9, 2023 • edited Loading

Notes

Test Steps

staykids left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staykids left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonisevener commented Sep 9, 2023 •

edited

Loading