duckduckgo / iOS

DuckDuckGo iOS Application
https://itunes.apple.com/us/app/duckduckgo-privacy-browser/id663592361?mt=8
Apache License 2.0
1.82k stars 414 forks source link

Integrate RemoteMessaging with NewTabPage #3017

Closed dus7 closed 3 months ago

dus7 commented 3 months ago

Task/Issue URL: https://app.asana.com/0/1206226850447395/1207703703130204/f Tech Design URL: CC:

Description:

Adds RemoteMessaging capability to new NewTabPage layout.

PixelFiring protocol got reorganized and extended, allowing to abstract this functionality and make depending logic testable. So far it was used only in AdAttributionPixelReporter.

Steps to test this PR:

  1. Change RemoteMessagingClient.endpoint value to https://www.jsonblob.com/api/1257306235471781888.
  2. Restart app.
  3. Make sure Internal User flag is set and NewTabPage sections are enabled in debug menu.
  4. Test functionality of all 5 messages.
  5. Check if proper pixels are fired on dismiss, appear, close and actions.
  6. Verify if message does not appear twice after it's dismissed.

Definition of Done (Internal Only):

Device Testing:


Internal references:

Software Engineering Expectations Technical Design Template

github-actions[bot] commented 3 months ago
Warnings
:warning: PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by :no_entry_sign: dangerJS against b03af69de0ab54aa96b3f84a26400f68e13662e8

dus7 commented 3 months ago

Not sure how compatible it is with the PixelKit work?

@federicocappelli any thoughts on this? Do you think it clashes with PixelKit in any way?

federicocappelli commented 3 months ago

Not sure how compatible it is with the PixelKit work?

@federicocappelli any thoughts on this? Do you think it clashes with PixelKit in any way?

Hi @dus7, from my understanding you wanted to:

  1. Add swift concurrency to the Pixel.fire(...) to fire a pixel synchronously instead of the usual fire-and-forget
  2. Be able to mock pixel firing for testing purposes Correct?

Your implementation seems good BUT I'm a bit worried about the constant diverging of our approaches across platforms, this could have been a good occasion for adopting PixelKit (only for .appleAdAttribution), adding swift concurrency to it and using the tools in PixelKitTestingUtilities for your unit tests.

My point is that I would like to see more collaboration and various teams working on shared tools instead of each of us re-implementing its own thing. This code becomes immediately tech debt, and another blocker, as soon someone tries to adopt PixelKit in iOS.

dus7 commented 3 months ago

Thanks @federicocappelli, this makes sense to me. Completely agreed we should not be diverging our approaches and it wasn't my goal at all while writing this. I wasn't aware about the existence of PixelKitTestingUtilities, nor there's any usage of PixelKit yet in iOS app so I just went and solved my own problem which has now escalated to multiple places :).

I'll merge this PR and add a task for myself to add swift concurrency to PixelKit and refactor this code to use PixelKit instead.

federicocappelli commented 3 months ago

Thanks @federicocappelli, this makes sense to me. Completely agreed we should not be diverging our approaches and it wasn't my goal at all while writing this. I wasn't aware about the existence of PixelKitTestingUtilities, nor there's any usage of PixelKit yet in iOS app so I just went and solved my own problem which has now escalated to multiple places :).

I'll merge this PR and add a task for myself to add swift concurrency to PixelKit and refactor this code to use PixelKit instead.

Perfectly understandable! Thank you for putting in the effort!