-
Notifications
You must be signed in to change notification settings - Fork 42
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
Migrate PayPalWebClient Analytics Logic Into PayPalWebAnalytics Class #300
base: main
Are you sure you want to change the base?
Conversation
class CardAnalytics( | ||
private val analyticsService: AnalyticsService | ||
) { | ||
internal class CardAnalytics(private val analyticsService: AnalyticsService) { |
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.
Marking analytics related classes internal
. I forgot to add this in the previous PR so adding it here now.
@@ -5,7 +5,6 @@ import android.content.Intent | |||
import android.util.Log | |||
import androidx.activity.ComponentActivity | |||
import com.paypal.android.corepayments.CoreConfig | |||
import com.paypal.android.corepayments.PayPalSDKError |
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.
How come you don't need this now?
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.
Looks like this import was only used in a function declaration e.g.
private fun notifyWebCheckoutFailure(error: PayPalSDKError, orderId: String?) {}
We removed those methods and the import got flagged by detekt
as no longer needed.
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.
Is it not needed because the launcher imports PayPalWebCheckoutError
? I think I got it.
Summary of changes
PayPalWebAnalytics
class to encapsulate analytics notification logicChecklist
Added a changelog entryAuthors