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

[FC-0072] iOS Mobile Plugin Architecture #528

Conversation

IvanStepanok
Copy link
Contributor

@IvanStepanok IvanStepanok commented Oct 11, 2024

Plugin System Integration for Open edX iOS Mobile Application

This PR introduces a plugin architecture to the Open edX mobile app for iOS platforms. By modularizing functionalities such as analytics and external dependencies into plugins, this enhancement aims to improve the app's extensibility and scalability. It allows for company-specific customizations without the need to modify the core codebase.

Key Changes:

  • Plugin Interface and Manager:

    • Implemented a protocol-based plugin system using OEXFoundation, enabling developers to create and integrate custom plugins seamlessly.
    • Added a PluginManager to handle the initialization and management of plugins within the application.
  • Core Application Update:

    • Restructured the app to focus on essential functions, delegating extended features like analytics to plugins.
    • Removed hard-coded dependencies on FirebaseAnalytics and FullStory from the core project.
  • Firebase Analytics Plugin:

    • Integrated FirebaseAnalytics using the new OEXFirebaseAnalytics plugin, conforming to the analytics protocols defined in OEXFoundation.
    • This allows for easier swapping or addition of analytics services in the future.
  • Library Migration to OEXFoundation:

    • Moved common libraries and dependencies to the OEXFoundation package for centralized management:
      • Alamofire (Networking)
      • Kingfisher (Image Loading)
      • SwiftUI-Introspect (UI Enhancements)
      • Swinject (Dependency Injection)
      • SwiftLintPlugins (Linting)

Objectives:

  • Extensibility:

    • Enables adding new features and services without altering the core application code.
    • Facilitates the development and integration of third-party plugins by the community.
  • Customization:

    • Allows organizations to build and configure their own plugins to meet specific business needs.
    • Supports company-specific implementations of analytics, payment systems, and more.
  • Separation of Concerns:

    • Simplifies maintenance by isolating features into self-contained modules.
    • Enhances code readability and manageability by reducing dependencies in the core project.
  • Scalability:

    • Provides a scalable architecture that can grow with additional plugins and features over time.
    • Improves the app's ability to adapt to new technologies and services.

Additional Notes:

  • Plugin Development:

    • Developers can create custom plugins by implementing the protocols defined in OEXFoundation.
    • Plugins can be distributed as Swift Packages, making them easy to integrate and share.
  • Version Consistency:

    • Ensured that the versions of OEXFoundation used in the core app and plugins are consistent to maintain compatibility.

This update makes the Open edX iOS mobile app more adaptable to various business needs, fostering a sustainable and flexible framework for ongoing development. By adopting a plugin-based architecture, the app is better positioned for future enhancements and contributions from the developer community.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 11, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Oct 11, 2024

Thanks for the pull request, @IvanStepanok!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/openedx-mobile-maintainers. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@IvanStepanok IvanStepanok force-pushed the feat/connect-OEXFoundation-to-project branch from 2aa83ba to 16fd879 Compare November 4, 2024 12:55
@IvanStepanok IvanStepanok changed the title Feat: connect OEXFoundation to project [FC-0072] iOS Mobile Plugin Architecture Nov 4, 2024
@IvanStepanok IvanStepanok marked this pull request as ready for review November 4, 2024 14:08
@@ -9,6 +9,7 @@ import SwiftyMocky
import XCTest
@testable import Core
@testable import Profile
import OEXFoundation
import Alamofire
Copy link
Contributor

@forgotvas forgotvas Nov 4, 2024

Choose a reason for hiding this comment

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

If you moved Alamofire to OEXFoundation i think then we should remove import that framework from main base code. Need to implement accessors for alamofire requests. For my side it would be logic that you just import OEXFoundation and able to use Alamofire's methods. Accessors could be named same as Alamofire's methods.

Same for other moved frameworks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion! We discussed the idea of creating wrappers for all libraries, including Alamofire, and concluded that at this stage, it would add complexity and slow down development.
The main reasons we decided to stick with direct imports for now are:

Need to cover all functionalities — creating wrappers for all required methods would take significant time, and in the future, it could slow down development as each new need would require adding a wrapper first.

Potential loss of flexibility — using libraries directly provides immediate access to all their functionalities. With wrappers, access to new or specialized functions may be limited, making development more challenging.

Increased maintenance costs — every new library version might require updating wrappers and verifying compatibility, leading to additional maintenance overhead.

We’ll definitely revisit this idea if a more isolated architecture is needed, but for now, we’ve decided to keep direct imports to ensure greater flexibility and development speed.

@forgotvas
Copy link
Contributor

And one more - we need to think about procedure or maybe PR format style when you want to make changes to OEXFoundation and upstream code at same time. Maybe we just should add link each to other, maybe something else. Just to reviewer could see for what feature author made changes in OEXFoundation.

@volodymyr-chekyrta
Copy link
Contributor

And one more - we need to think about procedure or maybe PR format style when you want to make changes to OEXFoundation and upstream code at same time. Maybe we just should add link each to other, maybe something else. Just to reviewer could see for what feature author made changes in OEXFoundation.

Makes sense 👍
The first thing to be introduced is a PR to the foundation with an idea/abstraction, followed by a PR to the application (with a link to the original PR to the foundation)

forgotvas
forgotvas previously approved these changes Nov 5, 2024
@@ -13,7 +13,6 @@ public extension Notification.Name {
static let onCourseEnrolled = Notification.Name("onCourseEnrolled")
static let onblockCompletionRequested = Notification.Name("onblockCompletionRequested")
static let onTokenRefreshFailed = Notification.Name("onTokenRefreshFailed")
static let onActualVersionReceived = Notification.Name("onActualVersionReceived")
Copy link
Contributor

Choose a reason for hiding this comment

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

only this one is moved to OEXFoundation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

import Foundation
import Core
import OEXFoundation
import OEXFirebaseAnalytics
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this import here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

import UIKit
import UserNotifications
import OEXFirebaseAnalytics
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this import here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -7,6 +7,8 @@

import Foundation
import Core
import OEXFoundation
import OEXFirebaseAnalytics
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this OEXFirebaseAnalytics import here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OEXFoundation needed for API class
OEXFirebaseAnalytics removed

config_folder = config_settings.get(self.CONFIG_MAPPINGS, {}).get(config['env_config'])
if config_folder:
# replace fullstory flag
project_file_string = self.replace_fullstory_flag(project_file_string, config_directory, name, config_folder, errors_texts)
Copy link
Contributor

Choose a reason for hiding this comment

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

@IvanStepanok Maybe it would be better to comment out that line, rather than removing the whole ‘for’ loop?
Without this part, the whole set_flags_from_mobile_config(self): function is meaningless. We could leave this as an example how to work with other flags in project file. WDYT? Thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, thanks!

@IvanStepanok
Copy link
Contributor Author

@rnr Thanks for review, ready for the next part!

@volodymyr-chekyrta volodymyr-chekyrta merged commit 5645bce into openedx:develop Nov 5, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants