Skip to content
This repository has been archived by the owner on Jun 1, 2024. It is now read-only.

[iOS] Analytics Screen #1793

Closed
mateusforgi opened this issue Dec 14, 2021 · 14 comments · Fixed by ZupIT/beagle-ios#31
Closed

[iOS] Analytics Screen #1793

mateusforgi opened this issue Dec 14, 2021 · 14 comments · Fixed by ZupIT/beagle-ios#31
Labels
android This issue directly affects structure and logic of Android project bug Something isn't working

Comments

@mateusforgi
Copy link

mateusforgi commented Dec 14, 2021

Description

On Beagle 1.10.1, the implementation for the analytics screen record is returning the remote url of the screen instead of the view controller identifier. This implementation is different from android where beagle is returning the identifier.

Android code:
https://github.com/ZupIT/beagle-android/blob/812a330777edd79a123495e9f678dc8f25a019f7/beagle/src/main/kotlin/br/com/zup/beagle/newanalytics/AnalyticsService.kt#L52

iOS code:
https://github.com/ZupIT/beagle-ios/blob/c44542d54f38a83bd935d5a66f0962b278745675/Sources/Beagle/Sources/Analytics/Service/RecordFactory.swift#L69

Steps To Reproduce

  1. Create a remote screen. Example: BeagleScreenViewController(.remote(.init(url: "http://localhost:8080/timeline")))

  2. Set the analyticsProvider on Beagle.dependencies.analyticsProvider

  3. Create record will be called with the screen variable as the remote url, instead of the screen identifier.

Expected Results

Create record should be called with the screen identifier on the screen variable.

Code example, screenshot, or link to a repository:

func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool {
        let dependencies = BeagleDependencies()
        dependencies.analyticsProvider = Analytics()
        dependencies.urlBuilder = UrlBuilder(baseUrl: baseURL)
        dependencies.networkClient = Network()
        Beagle.dependencies = dependencies
        let beagleController = BeagleScreenViewController(.remote(.init(url: "http://localhost:8080/timeline")))
       
        window = UIWindow(frame: UIScreen.main.bounds)
        window?.rootViewController = beagleController
        window?.backgroundColor = UIColor.white
        window?.makeKeyAndVisible()        
        return true
}

class Analytics: AnalyticsProvider {
 
    func createRecord(_ record: AnalyticsRecord) {
        switch record.type {
        case .action(let action):
           return 
        case .screen:
            if let screenName = record.screen {
                // REMOTE URL COMES AS THE SCREEN NAME
            }
        @unknown default:
            return
        }
    }
}
@mateusforgi mateusforgi added the bug Something isn't working label Dec 14, 2021
@github-actions
Copy link

👋 @mateusforgi
Thank you for raising an issue. We will investigate into the matter and get back to you as soon as possible.
Please make sure you have given us as much context as possible and that you have followed our contributing guidelines.
We will review it as soon as possible.

@hectorcustodiozup
Copy link
Contributor

Hi @mateusforgi,

Thank you for contributing with Beagle and for the thorough explanation on the issue. We will be on that asap.

@uziassantosferreira
Copy link
Contributor

this is a fact that could be discussed with the beagle core and explained better in the documentation since there are scenarios that use the identifier and others the url, it would be better to define a pattern and both platforms follow

@uziassantosferreira
Copy link
Contributor

scenarios that all this gets complicated and it's good to be discussed: if you use in the analytics provider the identifier of the view and not the URL, when I use the navigation should it be the URL or identifier to go back to the back screen?
What is best for the end developer? it would be nice to study and adjust both platforms

@Tiagoperes
Copy link
Contributor

Tiagoperes commented Dec 14, 2021

The correct behavior of this features is:

  1. Check whether the route is local or remote.
  2. If it's local, then use route.screen.identifier as the route.
  3. Otherwise, use route.url as the route.

I just checked the behavior of Web and Flutter and they're both implemented just like iOS. Android is actually the one with the unexpected behavior.

If the screen id is important to you, we can add a new field to the ScreenRecord named screenId. screenId would always have the value of screen.identifier if there is a screen with an id at the root node, otherwise, the key wouldn't exist in the record.

@mateusforgi Is it important for you to have the screen.identifier in the ScreenRecord if it exists?

@Tiagoperes Tiagoperes added the android This issue directly affects structure and logic of Android project label Dec 14, 2021
@mateusforgi
Copy link
Author

@uziassantosferreira the screen identifier would be the best solution for us. I am afraid that returning the remote URL can be described as a vulnerability as some mobile apps use encrypted URLs to avoid leaking the real URL.

@Tiagoperes the screenId would solve the issue for us, as it would give more control to our backend do the analytics without the need of a map from URL to page name on the frontend. However, I think it should be considered the fact that returning the URL might be a vulnerability.

@Tiagoperes
Copy link
Contributor

@mateusforgi if the URL is encrypted in the backend and decrypted in your HttpClient, this won't be a problem, because only the encrypted URL would be exposed.

In any case, you shouldn't base any security in the URL, no matter how much you try to hide it, it will always be visible for anyone inspecting the connection. For instance, any Android rooted device can see all the network traffic, just like you can check the network tab in a web browser.

@mateusforgi
Copy link
Author

@Tiagoperes When the navigation happens through a Beagle action it exposes the path not encrypted, right? I haven't done this test yet, but I noticed beagle only returns the path as the remote URL in that case to analytics.

Anyway, can you give a direction if the screen id or the screen identifier will be returned to analytics? We will move forward in case it is not possible.

@hectorcustodiozup
Copy link
Contributor

@mateusforgi I suggest we follow up with Tiago's idea, I have changed our web core to show you what the final payload would look like:
image

What's new:

  • Added rootId property to screen records. This property will return the value of the id of the current screen if it exists, otherwise it won't be added.

Does it cover the case for your? If so, I will add it to our backlog to be worked on all platforms.

@mateusforgi
Copy link
Author

@hectorcustodiozup the rootId solves the issue! We really appreciate it if you guys can add this field to the next release.

@Tiagoperes
Copy link
Contributor

We are going to add this in the next release @mateusforgi.

If there's any field in the final record you don't want to use, remember it goes through your analytics provider method createRecord. You can just remove the fields you don't want before sending it to the backend.

@Tiagoperes
Copy link
Contributor

@carlossteinzup @hectorcustodiozup can you guys implement this feature in iOS and Android?

@mateusforgi
Copy link
Author

@Tiagoperes Awesome! Thank you!

@hectorcustodiozup
Copy link
Contributor

@Tiagoperes @mateusforgi. Last week I could not work on this, but this one I will try to finish Web and add the changes for both iOS and Android.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
android This issue directly affects structure and logic of Android project bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants