Skip to content
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

Initial Plugin version #4

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Initial Plugin version #4

wants to merge 10 commits into from

Conversation

rnr
Copy link
Collaborator

@rnr rnr commented Jan 17, 2025

Added Initial code, github workflows and Tests

rnr and others added 8 commits January 10, 2025 13:40
Co-authored-by: Anton Yarmolenko <[email protected]>
* chore: added github workflows

* fix: scheme name

* chore: test linter error

* fix: linter error

---------

Co-authored-by: Anton Yarmolenko <[email protected]>
* chore: added tests

* chore: fixed warnings

* chore: fixed swiftlint warnings in tests

* chore: added unit_test workflow

---------

Co-authored-by: Anton Yarmolenko <[email protected]>
let package = Package(
name: "EDXMobileAnalytics",
platforms: [
.iOS(.v16)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we ditching iOS 15?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes openEdx was moved to iOS 16 in October here - before the plugin architecture was implemented

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we are still supporting iOS 15 in our app and as far as I understand, this package will be included in our app which will force the app to have deployment target of iOS 16 at least. I think it should be discussed in a Slack thread with wider audience so that everyone from business, product and dev side is aligned on the decision.

Copy link
Collaborator Author

@rnr rnr Jan 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this package will be included in the edX app, but after upstream develop (which is iOS 16 and swift 6) will only be merged into the 2u/develop marketplace. And yes, we could discuss this at the next tech meeting. Thanks.

Sources/EDXMobileAnalytics/Braze/BrazeListener.swift Outdated Show resolved Hide resolved
guard shouldListenNotification(userinfo: userInfo) else { return }

segmentAnalyticService?.receivedRemoteNotification(userInfo: userInfo)
deepLinkManager.processLinkFrom(userInfo: userInfo)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is push notification handling also in the scope of this repository/package?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as #4 (comment)

Sources/EDXMobileAnalytics/Braze/BrazeProvider.swift Outdated Show resolved Hide resolved
Sources/EDXMobileAnalytics/Braze/BrazeProvider.swift Outdated Show resolved Hide resolved
Tests/EDXMobileAnalyticsTests/BrazeListenerTests.swift Outdated Show resolved Hide resolved
Sources/EDXMobileAnalytics/EDXMobileAnalytics.swift Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
.github/workflows/xcodebuild.yml Outdated Show resolved Hide resolved
Co-Authored-By: Anton Yarmolenko <[email protected]>
@rnr
Copy link
Collaborator Author

rnr commented Jan 21, 2025

@mta452 your feedback was addressed and/or commented.
Ready for another pass. Thank you

@mta452
Copy link

mta452 commented Jan 22, 2025

@rnr I have added minor suggestions on the comments. Other than that it looks good to me. I'll approve it once we reach an understanding.

@rnr rnr closed this Jan 22, 2025
@rnr rnr reopened this Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants