-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(GiniHealthSDK): Brand ingredient configuration #766
base: main
Are you sure you want to change the base?
Conversation
…selection and review screen based on configuration IPC-507
… added in GiniInternalPaymentSDK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @razvancapra
Please find some notes below:
...lPaymentSDK/Sources/GiniInternalPaymentSDK/PaymentComponents/PaymentComponentViewModel.swift
Show resolved
Hide resolved
@@ -16,6 +16,17 @@ public enum GiniLocalization: String, CaseIterable { | |||
case de = "de" | |||
} | |||
|
|||
/** | |||
An enumeration representing the visibility type of an ingredient brand. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This service implements the `ClientConfigurationServiceProtocol` and provides methods to retrieve configuration data from a specified API domain. | ||
*/ | ||
public final class ClientConfigurationService: ClientConfigurationServiceProtocol { | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
Please follow the comment style that you use in the past
`/**
Initializes a new instance of GiniMerchant.
This initializer creates a GiniMerchant instance by first constructing a Client object with the provided client credentials (id, secret, domain)
- Parameters:
- id: The client ID provided by Gini when you register your application. This is a unique identifier for your application.
- secret: The client secret provided by Gini alongside the client ID. This is used to authenticate your application to the Gini API.
- domain: The domain associated with your client credentials. This is used to scope the client credentials to a specific domain.
- pinningConfig: Configuration for certificate pinning. Format ["PinnedDomains" : ["PublicKeyHashes"]]
- logLevel: The log level. `LogLevel.none` by default.
*/`
import Foundation | ||
|
||
extension ClientConfigurationServiceProtocol { | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion]: Please use the comment format
/**
Initializes a new instance of GiniMerchant.
This initializer creates a GiniMerchant instance by first constructing a Client object with the provided client credentials (id, secret, domain)
- Parameters:
- id: The client ID provided by Gini when you register your application. This is a unique identifier for your application.
- secret: The client secret provided by Gini alongside the client ID. This is used to authenticate your application to the Gini API.
- domain: The domain associated with your client credentials. This is used to scope the client credentials to a specific domain.
- pinningConfig: Configuration for certificate pinning. Format ["PinnedDomains" : ["PublicKeyHashes"]]
- logLevel: The log level. `LogLevel.none` by default.
*/
Protocol for client configuration service | ||
*/ | ||
public protocol ClientConfigurationServiceProtocol: AnyObject { | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion]: Please use the comment format :
/**
Initializes a new instance of GiniMerchant.
This initializer creates a GiniMerchant instance by first constructing a Client object with the provided client credentials (id, secret, domain)
- Parameters:
- id: The client ID provided by Gini when you register your application. This is a unique identifier for your application.
- secret: The client secret provided by Gini alongside the client ID. This is used to authenticate your application to the Gini API.
- domain: The domain associated with your client credentials. This is used to scope the client credentials to a specific domain.
- pinningConfig: Configuration for certificate pinning. Format ["PinnedDomains" : ["PublicKeyHashes"]]
- logLevel: The log level. `LogLevel.none` by default.
*/
*/ | ||
public struct ClientConfiguration: Codable { | ||
/** | ||
Creates a new `ClientConfiguration` instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion]: Please use the comment format :
/**
Initializes a new instance of GiniMerchant.
This initializer creates a GiniMerchant instance by first constructing a Client object with the provided client credentials (id, secret, domain)
- Parameters:
- id: The client ID provided by Gini when you register your application. This is a unique identifier for your application.
- secret: The client secret provided by Gini alongside the client ID. This is used to authenticate your application to the Gini API.
- domain: The domain associated with your client credentials. This is used to scope the client credentials to a specific domain.
- pinningConfig: Configuration for certificate pinning. Format ["PinnedDomains" : ["PublicKeyHashes"]]
- logLevel: The log level. `LogLevel.none` by default.
*/
- parameter comunicationTone: A configuration that indicates the comunication tone of the texts. Defaults to `nil | ||
- parameter ingredientBrandType: A configuration that indicates the presence of the ingredient brand. Defaults to `nil`. | ||
*/ | ||
public init(clientID: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Clarification]: one of our customers is using AlternativeTokenSource and there is no clientID, how will we handle that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question. We need to ask backend team.
@@ -126,6 +127,7 @@ public final class PaymentComponentsController: BottomSheetsProviderProtocol, Gi | |||
self.stringsProvider = giniHealth | |||
setupObservers() | |||
loadPaymentProviders() | |||
fetchAndConfigureClientConfiguration() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fetchAndConfigureClientConfiguration() | |
fetchAndUpdateClientConfiguration() |
@@ -169,7 +170,8 @@ final class PaymentComponentsControllerTests: XCTestCase { | |||
poweredByGiniStrings: giniHealth.poweredByGiniStrings, | |||
moreInformationConfiguration: giniHealth.moreInformationConfiguration, | |||
moreInformationStrings: giniHealth.moreInformationStrings, | |||
urlOpener: URLOpener(MockUIApplication(canOpen: false))) | |||
urlOpener: URLOpener(MockUIApplication(canOpen: false)), | |||
clientConfiguration: nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Clarification]: why do we always pass clientConfiguration: nil
in tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have any tests for client configuration. Should we create some?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@razvancapra please check a tiny suggestion: you removed accidentally whitespace and it's good to go in QA
@@ -153,7 +154,12 @@ public final class GiniHealthConfiguration: NSObject { | |||
/** | |||
Custom localization configuration for localizable strings. | |||
*/ | |||
public var customLocalization: GiniLocalization? | |||
public var customLocalization: GiniLocalization? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public var customLocalization: GiniLocalization? | |
public var customLocalization: GiniLocalization? |
IPC-507