Skip to content

Conversation

@mahiarirani
Copy link
Member

This pull request refactors the Application class in lib/AppInfo/Application.php to improve modularity by introducing traits for handling dependencies and telemetry, and a new BaseApp class to centralize shared functionality. It replaces inline logic with reusable components, simplifying the codebase and improving maintainability.

Refactoring for modularity:

  • Introduction of BaseApp class:

    • Created a new abstract class BaseApp in lib/AppInfo/BaseApp.php to serve as the base for application classes, centralizing container and logger initialization.
  • Dependency management via Dependency trait:

    • Added a Dependency trait in lib/AppInfo/Dependency.php to handle app installation, enabling, and loading, including checks for required database tables.
  • Telemetry management via Telemetry trait:

    • Added a Telemetry trait in lib/AppInfo/Telemetry.php to encapsulate telemetry-related logic, including Sentry initialization and configuration.

Code cleanup and simplification:

  • Refactored Application class:

    • Replaced the Application class's inheritance from App with BaseApp and removed redundant methods like enableFilesExternalApp, loadTelemetry, and configureTelemetry. These functionalities are now handled by the Dependency and Telemetry traits. [1] [2] [3] [4]
  • New exception class for dependency errors:

    • Added AppDependencyException in lib/Exception/AppDependencyException.php to provide a dedicated exception for app dependency-related issues.

- add: base app class
- add: trait for dependency and telemetry
- fix: install dependecny app with better management
- refactor: clean app class
- add: exception for installation issues
@mahiarirani mahiarirani changed the title feat(app): abstract application feat(app): install dependency application Jun 5, 2025
@mahiarirani mahiarirani requested a review from Copilot June 5, 2025 06:35
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the application to improve modularity by introducing shared functionality through a new BaseApp class and reusable traits for dependency and telemetry management.

  • Introduces a new BaseApp class to centralize container and logger setup.
  • Implements Dependency and Telemetry traits to handle app installation and telemetry configuration, respectively.
  • Adds a dedicated exception class for dependency errors.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
lib/Exception/AppDependencyException.php New exception class for handling dependency errors
lib/AppInfo/Telemetry.php Added Telemetry trait for initializing and configuring telemetry
lib/AppInfo/Dependency.php Added Dependency trait to manage app installation, enabling, and loading
lib/AppInfo/BaseApp.php New abstract class for centralizing shared application functionality
lib/AppInfo/Application.php Refactored to extend BaseApp and utilize new traits
Comments suppressed due to low confidence (3)

lib/Exception/AppDependencyException.php:7

  • The class extends BaseException, but there is no import statement for BaseException. Consider adding 'use OCA\Files_External_Ethswarm\Exception\BaseException;' at the top.
class AppDependencyException extends BaseException {

lib/AppInfo/Telemetry.php:36

  • The Telemetry trait references Application::TELEMETRY_MINIMUM_SUPPORTED_NEXTCLOUD_VERSION and Application::TELEMETRY_URL, which have been removed from the Application class. Consider relocating these constants to BaseApp or another shared location and updating the references accordingly.
 $isSupported = version_compare($currentNextcloudVersion, Application::TELEMETRY_MINIMUM_SUPPORTED_NEXTCLOUD_VERSION, '>=');

lib/AppInfo/Telemetry.php:40

  • There is a discrepancy between the value set (false) in the subsequent line and the log message that states telemetry is being set to true. Update either the value or the log message to resolve this inconsistency.
if ('' === $config->getSystemValue('telemetry.enabled') && $isSupported) {

Copy link
Collaborator

@JoaoSRaposo JoaoSRaposo left a comment

Choose a reason for hiding this comment

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

Teste on tech-check in today :)

@JoaoSRaposo JoaoSRaposo merged commit 2198c72 into stage Jun 5, 2025
2 checks passed
@JoaoSRaposo JoaoSRaposo deleted the feature/install-app-dependecy branch June 5, 2025 13:38
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.

3 participants