[MOB-2819] Removed 2nd tracking occurrance of onboarding page number #851

TimNowaczynski · 2025-02-19T16:56:52Z

https://ecosia.atlassian.net/browse/MOB-2819

My cat kinda deleted the PR Preset, but basically I removed two lines of code which seem to track the property in question.

I also ran tests and they actually failed in (and only in) AnalyticsSpyTest.swift but this seems to be already the case on the base banch. But before I mess something up, can you maybe look into that as well? Just to be safe :)

github-actions · 2025-02-19T16:57:39Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Possible Impact on Analytics Tracking The removal of the `.value(.init(integerLiteral: index))` line in two places may affect the analytics tracking functionality. Ensure that this change aligns with the intended behavior and does not disrupt any dependent analytics processes. let event = Structured(category: Category.intro.rawValue, action: Action.display.rawValue) .property(page.rawValue) track(event) } public func introClick(_ label: Label.Onboarding, page: Property.OnboardingPage?, index: Int) { guard let page else { return } let event = Structured(category: Category.intro.rawValue, action: Action.click.rawValue) .label(label.rawValue) .property(page.rawValue) track(event)

github-actions · 2025-02-19T16:57:48Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Verify impact of removing index tracking Ensure that removing the `.value(.init(integerLiteral: index))` does not affect downstream systems or analytics that rely on the `index` value being tracked, as this could lead to data inconsistencies or loss of critical information. firefox-ios/Ecosia/Analytics/Analytics.swift [225-228] let event = Structured(category: Category.intro.rawValue, action: Action.display.rawValue) .property(page.rawValue) + .value(.init(integerLiteral: index)) track(event) Suggestion importance[1-10]: 7 __ Why: The suggestion highlights a potential issue with removing the `.value(.init(integerLiteral: index))` call, which could affect downstream systems or analytics relying on the index value. While it does not propose a direct code change, it raises an important point that warrants verification to ensure no unintended consequences arise from this removal.	Medium

lucaschifino · 2025-02-20T13:20:08Z

I also ran tests and they actually failed in (and only in) AnalyticsSpyTest.swift but this seems to be already the case on the base branch. But before I mess something up, can you maybe look into that as well? Just to be safe :)

Just ran them locally and they passed, they are also ran on every PR with CI and your PR here looks good :)

I remember seeing some flakiness on those tests, which one was it? I think the menu ones sometimes fail 🤔

TimNowaczynski · 2025-02-20T13:26:59Z

I also ran tests and they actually failed in (and only in) AnalyticsSpyTest.swift but this seems to be already the case on the base branch. But before I mess something up, can you maybe look into that as well? Just to be safe :)

Just ran them locally and they passed, they are also ran on every PR with CI and your PR here looks good :)

I remember seeing some flakiness on those tests, which one was it? I think the menu ones sometimes fail 🤔

Okay, thanks for having a look :)

lucaschifino · 2025-02-20T13:31:20Z

firefox-ios/Ecosia/Analytics/Analytics.swift

@@ -225,7 +225,6 @@ open class Analytics {
        let event = Structured(category: Category.intro.rawValue,
                               action: Action.display.rawValue)
            .property(page.rawValue)
-            .value(.init(integerLiteral: index))


Can you also remove the index (at index: Int) from the function parameters since it's no longer needed?

I think that will also break the tests, so will require some refactoring there, but should be a nice learning opportunity 🙂

Sure, I'll do that 👍

TimNowaczynski · 2025-02-22T13:13:38Z

@lucaschifino Hey I just rebased and ran test again before wanting to address what you mentioned above. But like last week I can't successfully run our tests at the moment. It fails here:

... in `Client/Application/main.swift` on line 25 - did I maybe miss something I'll have to do before running our tests?

lucaschifino · 2025-02-24T08:10:35Z

@lucaschifino Hey I just rebased and ran test again before wanting to address what you mentioned above. But like last week I can't successfully run our tests at the moment. It fails here:
... in Client/Application/main.swift on line 25 - did I maybe miss something I'll have to do before running our tests?

@TimNowaczynski just checked out your branch and EcosiaTests ran successfully (screenshot below) 🤔 That error you shared seems very generic since it happens on the main executable file, maybe even while booting the app. Maybe it's some issue in your Xcode? If so I would suggest going over the steps here to start clean there.

If it still happens we can try to to debug better, is there a specific test case where this fails? Does it work if you run it on the simulator outside of the tests?

TimNowaczynski · 2025-02-24T08:47:01Z

Thanks, I'll check tomorrow 🤞

TimNowaczynski · 2025-03-04T13:11:50Z

@lucaschifino After running those steps you mentioned the tests failed again, then after re-running everything a 2nd time things went fine. So for me they seem to be flaky for some reason. But that's fine for me now, looks like I didn't break anything. I'll now rebase things and then I'm done I guess.

lucaschifino · 2025-03-04T13:29:53Z

@lucaschifino After running those steps tests failed again, then after re-running everything went fine. So for me they seem to be flaky for some reason. But that's fine for me now, looks like I didn't break anything. I'll now rebase things and then I'm done I guess.

@TimNowaczynski Makes sense! There's indeed some flakyness on Analytics tests, trying to run it a couple times is a good idea until we tackle that, hope it isn't too much of a problem 🙈

I think the one other thing missing would be this other comment I left a while ago, not sure if you were able to check it out already 🙂

… from tracking

WIP: remove tracking invocations

TimNowaczynski · 2025-03-05T11:43:19Z

@lucaschifino I'm pretty sure I removed too much now because tests fail for me in the testTrackMenuStatus method. At the first glance it seems unrelated to me, but it definitly is 🤔

Fixed line which should not have been removed

TimNowaczynski · 2025-03-05T11:48:39Z

@lucaschifino Sorry for pinging you again, but I found it 😓 :)

lucaschifino · 2025-03-05T15:39:21Z

@lucaschifino I'm pretty sure I removed too much now because tests fail for me in the testTrackMenuStatus method. At the first glance it seems unrelated to me, but it definitly is 🤔

No worries! I believe testTrackMenuStatus is one of the flaky ones... We have this tech debt ticket to tackle some of them, we probably should skip this one and add to the ticket to be looked at later. But if you managed to get unblocked then we're good for now 🙏

Edit: Btw, noticed on CI here on the PR some flaky tests were also failing, re-ran it a couple times to try and get it green.

lucaschifino

Looks great, thanks for the adjustments 👏

There's one last minor comment from my side.

firefox-ios/Client/Ecosia/UI/Onboarding/WelcomeTour.swift

Removed unused code fragment

TimNowaczynski · 2025-03-06T08:55:28Z

@lucaschifino Thanks! Should be all right now :)

lucaschifino

LGTM! Nice work 🎉 👏

The only thing missing is the CI being green, I just re-ran it as it failed again on the flaky test 🤞

TimNowaczynski requested a review from a team February 19, 2025 16:56

TimNowaczynski changed the title ~~[MOB-2819] Removed tracking 2nd occurrance of onboarding page number~~ [MOB-2819] Removed 2nd tracking occurrance of onboarding page number Feb 20, 2025

lucaschifino reviewed Feb 20, 2025

View reviewed changes

TimNowaczynski force-pushed the tn-mob-2819-remove-onboarding-analytics branch from 6ec081e to 0bbef71 Compare February 22, 2025 12:59

[MOB-2819] MOB-2819: Removed 2nd occurrance of onboarding page number…

8b1214e

… from tracking

TimNowaczynski force-pushed the tn-mob-2819-remove-onboarding-analytics branch from 0bbef71 to 8b1214e Compare March 4, 2025 13:55

[MOB-2819] MOB-2819

424f824

WIP: remove tracking invocations

[MOB-2819] Update AnalyticsSpyTests.swift

e1677ed

Fixed line which should not have been removed

lucaschifino reviewed Mar 5, 2025

View reviewed changes

firefox-ios/Client/Ecosia/UI/Onboarding/WelcomeTour.swift Show resolved Hide resolved

[MOB-2819] Update WelcomeTour.swift

41dd116

Removed unused code fragment

lucaschifino approved these changes Mar 6, 2025

View reviewed changes

TimNowaczynski merged commit d66b0ea into mob-3113-firefox-upgrade-133 Mar 6, 2025
3 checks passed

TimNowaczynski deleted the tn-mob-2819-remove-onboarding-analytics branch March 6, 2025 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MOB-2819] Removed 2nd tracking occurrance of onboarding page number #851

[MOB-2819] Removed 2nd tracking occurrance of onboarding page number #851

TimNowaczynski commented Feb 19, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025

github-actions bot commented Feb 19, 2025

lucaschifino commented Feb 20, 2025

TimNowaczynski commented Feb 20, 2025

lucaschifino Feb 20, 2025

TimNowaczynski Feb 21, 2025

TimNowaczynski commented Feb 22, 2025 •

edited

Loading

lucaschifino commented Feb 24, 2025 •

edited

Loading

TimNowaczynski commented Feb 24, 2025

TimNowaczynski commented Mar 4, 2025 •

edited

Loading

lucaschifino commented Mar 4, 2025

TimNowaczynski commented Mar 5, 2025 •

edited

Loading

TimNowaczynski commented Mar 5, 2025

lucaschifino commented Mar 5, 2025 •

edited

Loading

lucaschifino left a comment

TimNowaczynski commented Mar 6, 2025

lucaschifino left a comment

[MOB-2819] Removed 2nd tracking occurrance of onboarding page number #851

[MOB-2819] Removed 2nd tracking occurrance of onboarding page number #851

Conversation

TimNowaczynski commented Feb 19, 2025 • edited Loading

github-actions bot commented Feb 19, 2025

PR Reviewer Guide 🔍

github-actions bot commented Feb 19, 2025

PR Code Suggestions ✨

lucaschifino commented Feb 20, 2025

TimNowaczynski commented Feb 20, 2025

lucaschifino Feb 20, 2025

Choose a reason for hiding this comment

TimNowaczynski Feb 21, 2025

Choose a reason for hiding this comment

TimNowaczynski commented Feb 22, 2025 • edited Loading

lucaschifino commented Feb 24, 2025 • edited Loading

TimNowaczynski commented Feb 24, 2025

TimNowaczynski commented Mar 4, 2025 • edited Loading

lucaschifino commented Mar 4, 2025

TimNowaczynski commented Mar 5, 2025 • edited Loading

TimNowaczynski commented Mar 5, 2025

lucaschifino commented Mar 5, 2025 • edited Loading

lucaschifino left a comment

Choose a reason for hiding this comment

TimNowaczynski commented Mar 6, 2025

lucaschifino left a comment

Choose a reason for hiding this comment

TimNowaczynski commented Feb 19, 2025 •

edited

Loading

TimNowaczynski commented Feb 22, 2025 •

edited

Loading

lucaschifino commented Feb 24, 2025 •

edited

Loading

TimNowaczynski commented Mar 4, 2025 •

edited

Loading

TimNowaczynski commented Mar 5, 2025 •

edited

Loading

lucaschifino commented Mar 5, 2025 •

edited

Loading