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

feat: Jim Mobile support #185

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

Conversation

myTselection
Copy link

@myTselection myTselection commented Jan 29, 2025

Hi, when I wanted to create a new integration to support JimMobile phone subscriptions in HA, I noticed the backend is the same as Mobile Vikings. So I didn't want to redo the excellent work you did on this and decided to just extend your code to get the option to connect to Mobile Vikings or JimMobile. Some of the sensors weren't useful for JimMobile, so those have been disabled. But overall most is very compatibel and works very well for me.

Please let me know if any other changes would be expected.

Schermafbeelding 2025-01-29 230532

Schermafbeelding 2025-01-29 231311

Summary by CodeRabbit

Release Notes

  • New Features

    • Added support for Jim Mobile mobile phone subscription services.
    • Introduced mobile platform selection during configuration.
    • New sensors for prepaid bundles information.
  • Improvements

    • Enhanced error handling and data retrieval.
    • Updated documentation to reflect multi-platform support, including specific functionalities for Mobile Vikings and Jim Mobile.
    • Improved configuration flow with platform-specific options.
  • Version

    • Upgraded to version 2.0.0.

Copy link
Contributor

coderabbitai bot commented Jan 29, 2025

Walkthrough

This pull request introduces comprehensive support for both Mobile Vikings and Jim Mobile mobile phone subscription services in the Home Assistant integration. The changes span across multiple files, including the README, configuration flow, client implementation, and translation files. The integration now allows users to select and configure their mobile platform during setup, with platform-specific authentication, sensor filtering, and documentation updates.

Changes

File Change Summary
README.md Updated title, description, and sensor documentation to include Jim Mobile support
custom_components/mobile_vikings/__init__.py Updated module docstring and added mobile_platform parameter to client initialization
custom_components/mobile_vikings/binary_sensor.py Added mobile_platforms attribute to sensor descriptions, enhanced platform-specific filtering
custom_components/mobile_vikings/client.py Updated authentication and request handling to support multiple mobile platforms
custom_components/mobile_vikings/config_flow.py Added mobile platform selection to configuration flow
custom_components/mobile_vikings/const.py Introduced new constants for Jim Mobile authentication and URLs
custom_components/mobile_vikings/entity.py Modified device info generation based on mobile platform
custom_components/mobile_vikings/manifest.json Bumped version to v2.0.0
custom_components/mobile_vikings/models.py Added mobile_platform to configuration data structure
custom_components/mobile_vikings/sensor.py Enhanced sensor descriptions with mobile platform support
custom_components/mobile_vikings/translations/en.json Updated labels and descriptions to include Jim Mobile references
custom_components/mobile_vikings/translations/fr.json Updated labels and descriptions to include Jim Mobile references
custom_components/mobile_vikings/translations/nl.json Updated labels and descriptions to include Jim Mobile references

Possibly related PRs

Suggested Labels

docs, fix

Poem

🐰 Hopping through mobile platforms wide,
Vikings and Jim, now side by side!
Sensors dancing, config flow bright,
Integration's magic takes its flight!
A rabbit's code, both smart and bold! 🚀

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (11)
custom_components/mobile_vikings/client.py (4)

8-19: Remove trailing whitespace in import statements.

Lines 9-17 contain trailing whitespace, flagged by static analysis. Removing these trailing spaces improves code cleanliness.

Apply this diff to remove the trailing whitespace:

-from .const import (
-    BASE_URL,␣
-    BASE_URL_JIMMOBILE,␣
-    CLIENT_ID,␣
-    CLIENT_ID_JIMMOBILE,␣
-    CLIENT_SECRET,␣
-    CLIENT_SECRET_TAG,␣
-    CLIENT_SECRET_TAG_JIMMOBILE,␣
-    CLIENT_SECRET_VALUE_JIMMOBILE,␣
-    MOBILE_VIKINGS,␣
-    JIM_MOBILE
-)
+from .const import (
+    BASE_URL,
+    BASE_URL_JIMMOBILE,
+    CLIENT_ID,
+    CLIENT_ID_JIMMOBILE,
+    CLIENT_SECRET,
+    CLIENT_SECRET_TAG,
+    CLIENT_SECRET_TAG_JIMMOBILE,
+    CLIENT_SECRET_VALUE_JIMMOBILE,
+    MOBILE_VIKINGS,
+    JIM_MOBILE
+)
🧰 Tools
🪛 Ruff (0.8.2)

9-9: Trailing whitespace

Remove trailing whitespace

(W291)


10-10: Trailing whitespace

Remove trailing whitespace

(W291)


11-11: Trailing whitespace

Remove trailing whitespace

(W291)


12-12: Trailing whitespace

Remove trailing whitespace

(W291)


13-13: Trailing whitespace

Remove trailing whitespace

(W291)


14-14: Trailing whitespace

Remove trailing whitespace

(W291)


15-15: Trailing whitespace

Remove trailing whitespace

(W291)


16-16: Trailing whitespace

Remove trailing whitespace

(W291)


17-17: Trailing whitespace

Remove trailing whitespace

(W291)


72-75: Conditional client credentials look good; consider a more scalable approach.

The logic correctly assigns credentials based on the mobile platform. For future expansions, you might consider a dictionary-based approach to reduce duplication.


157-160: Enhance platform handling in URL selection.

Currently, it defaults to BASE_URL_JIMMOBILE if not MOBILE_VIKINGS. If more platforms are added in the future, using a mapping (dictionary) might be more extensible.


227-229: Returned error message for Jim Mobile loyalty points.

Returning a static error might suffice here, but you could raise an exception or return None if this is an unsupported feature.

custom_components/mobile_vikings/entity.py (1)

41-41: Add fallback for unknown mobile platforms.

The platform-specific logic looks good, but consider adding a default case for unknown platforms to make the code more robust.

-            manufacturer=NAME if self.mobilePlatform == MOBILE_VIKINGS else JIM_MOBILE,
-            configuration_url=WEBSITE if self.mobilePlatform == MOBILE_VIKINGS else WEBSITE_JIMMOBILE,
+            manufacturer=NAME if self.mobilePlatform == MOBILE_VIKINGS else (JIM_MOBILE if self.mobilePlatform == JIM_MOBILE else "Unknown"),
+            configuration_url=WEBSITE if self.mobilePlatform == MOBILE_VIKINGS else (WEBSITE_JIMMOBILE if self.mobilePlatform == JIM_MOBILE else None),

Also applies to: 52-53

custom_components/mobile_vikings/config_flow.py (2)

16-18: Remove trailing whitespace.

Remove trailing whitespace from the import statements.

-    SelectSelector, 
-    SelectSelectorConfig, 
-    SelectSelectorMode
+    SelectSelector,
+    SelectSelectorConfig,
+    SelectSelectorMode
🧰 Tools
🪛 Ruff (0.8.2)

16-16: Trailing whitespace

Remove trailing whitespace

(W291)


17-17: Trailing whitespace

Remove trailing whitespace

(W291)


80-83: Simplify unique_id generation logic.

The current implementation has duplicated string formatting. Consider simplifying it.

-                if user_input['mobilePlatform'] == JIM_MOBILE:
-                    await self.async_set_unique_id(f"{DOMAIN}_{JIM_MOBILE}_{user_input[CONF_USERNAME]}")
-                else:
-                    await self.async_set_unique_id(f"{DOMAIN}_{user_input[CONF_USERNAME]}")
+                platform_prefix = f"{JIM_MOBILE}_" if user_input['mobilePlatform'] == JIM_MOBILE else ""
+                await self.async_set_unique_id(f"{DOMAIN}_{platform_prefix}{user_input[CONF_USERNAME]}")
custom_components/mobile_vikings/binary_sensor.py (2)

148-149: Fix typo in variable name.

There's a typo in the variable name mobilePlatofrm.

-    mobilePlatofrm = coordinator.client.mobilePlatform
+    mobilePlatform = coordinator.client.mobilePlatform

22-22: Remove unused import.

The JIM_MOBILE import is not used in this file.

-from .const import DOMAIN, MOBILE_VIKINGS, JIM_MOBILE
+from .const import DOMAIN, MOBILE_VIKINGS
🧰 Tools
🪛 Ruff (0.8.2)

22-22: .const.JIM_MOBILE imported but unused

Remove unused import: .const.JIM_MOBILE

(F401)

custom_components/mobile_vikings/sensor.py (1)

541-541: Consider extracting debug prefix to a constant.

The debug prefix "Skipping {sensor_type.key}-{sensor_type.translation_key} for mobile platform {mobilePlatform}" is repeated. Consider extracting it to a constant for better maintainability.

Add at the top of the file:

+DEBUG_SKIP_SENSOR = "Skipping {key}-{translation_key} for mobile platform {platform}"

Then use it in the code:

-    _LOGGER.debug(f"Skipping {sensor_type.key}-{sensor_type.translation_key} for mobile platform {mobilePlatofrm}")
+    _LOGGER.debug(DEBUG_SKIP_SENSOR.format(
+        key=sensor_type.key,
+        translation_key=sensor_type.translation_key,
+        platform=mobilePlatform
+    ))

Also applies to: 553-553

README.md (1)

109-141: Consider improving the support column format.

The support column could be more visually appealing and easier to scan.

Consider using emojis or formatting:

-| Support |
-| ------- |
-| MV & JM |
-| MV      |
+| Support    |
+| ---------- |
+| MV+JM ✅   |
+| MV only 📱 |
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d3d9339 and 8c92dc4.

⛔ Files ignored due to path filters (2)
  • custom_components/mobile_vikings/__pycache__/client.cpython-312.pyc is excluded by !**/*.pyc
  • images/brand/jimmobile.png is excluded by !**/*.png
📒 Files selected for processing (13)
  • README.md (4 hunks)
  • custom_components/mobile_vikings/__init__.py (2 hunks)
  • custom_components/mobile_vikings/binary_sensor.py (9 hunks)
  • custom_components/mobile_vikings/client.py (8 hunks)
  • custom_components/mobile_vikings/config_flow.py (4 hunks)
  • custom_components/mobile_vikings/const.py (1 hunks)
  • custom_components/mobile_vikings/entity.py (3 hunks)
  • custom_components/mobile_vikings/manifest.json (1 hunks)
  • custom_components/mobile_vikings/models.py (1 hunks)
  • custom_components/mobile_vikings/sensor.py (29 hunks)
  • custom_components/mobile_vikings/translations/en.json (3 hunks)
  • custom_components/mobile_vikings/translations/fr.json (3 hunks)
  • custom_components/mobile_vikings/translations/nl.json (3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
custom_components/mobile_vikings/binary_sensor.py

22-22: .const.JIM_MOBILE imported but unused

Remove unused import: .const.JIM_MOBILE

(F401)

custom_components/mobile_vikings/client.py

9-9: Trailing whitespace

Remove trailing whitespace

(W291)


10-10: Trailing whitespace

Remove trailing whitespace

(W291)


11-11: Trailing whitespace

Remove trailing whitespace

(W291)


12-12: Trailing whitespace

Remove trailing whitespace

(W291)


13-13: Trailing whitespace

Remove trailing whitespace

(W291)


14-14: Trailing whitespace

Remove trailing whitespace

(W291)


15-15: Trailing whitespace

Remove trailing whitespace

(W291)


16-16: Trailing whitespace

Remove trailing whitespace

(W291)


17-17: Trailing whitespace

Remove trailing whitespace

(W291)

custom_components/mobile_vikings/config_flow.py

16-16: Trailing whitespace

Remove trailing whitespace

(W291)


17-17: Trailing whitespace

Remove trailing whitespace

(W291)

🔇 Additional comments (22)
custom_components/mobile_vikings/client.py (4)

33-33: Validate the new mobilePlatform parameter.

Although the constructor now takes a mobilePlatform argument, there is no validation for invalid platform values. Consider adding explicit checks or raising a dedicated exception if an unsupported platform is passed.


44-45: Good docstring clarification.

These lines accurately describe the new parameter for specifying the mobile platform.


53-53: No concerns on assigning mobilePlatform.

This assignment is straightforward and aligns with the new constructor parameter.


85-86: Properly inserting dynamic credentials.

These lines correctly use the chosen client ID and secret for authentication. No issues found.

Also applies to: 96-97

custom_components/mobile_vikings/models.py (1)

14-14: New mobilePlatform field is properly added.

Including mobilePlatform in the typed dict aligns with the platform-based logic elsewhere.

custom_components/mobile_vikings/const.py (5)

20-21: Platforms for Mobile Vikings and Jim Mobile.

Introducing these constants is correct for multi-platform handling.


29-30: Client secret tag definition.

No issues found. This is a straightforward constant for OAuth2.


36-37: Repurposing string fields for Jim Mobile credentials.

Using "grant_type" and "password" for Jim Mobile is unusual but presumably aligns with the Jim Mobile API requirements. Confirm it meets your service endpoints’ expectations.


39-39: Separate Jim Mobile client ID and client secret.

Defining distinct constants for Jim Mobile is clear and maintains separation from Mobile Vikings data.

Also applies to: 42-42


47-49: Correct base and website URLs for Jim Mobile.

These constants neatly add Jim Mobile references. No issues found.

Also applies to: 54-54

custom_components/mobile_vikings/sensor.py (2)

25-25: LGTM! Well-structured type-safe implementation.

The addition of the mobile_platforms attribute to MobileVikingsSensorDescription is a clean way to support platform-specific sensors.

Also applies to: 48-48


70-71: Improved null-safety in data access patterns.

The addition of explicit null checks (data is not None) before accessing dictionary keys makes the code more robust against runtime errors.

Also applies to: 84-85, 98-99

custom_components/mobile_vikings/manifest.json (1)

14-14: LGTM! Appropriate version bump.

The version bump to v2.0.0 correctly follows semantic versioning principles for this major feature addition of JimMobile support.

custom_components/mobile_vikings/translations/en.json (1)

15-15: LGTM! Comprehensive translation updates.

The changes consistently integrate JimMobile across all relevant messages and add necessary new translations for the platform selection and bundles info.

Also applies to: 19-20, 93-95, 134-134

custom_components/mobile_vikings/translations/nl.json (1)

15-15: LGTM! Consistent Dutch translations.

The Dutch translations accurately mirror the English changes while maintaining proper Dutch language conventions and terminology.

Also applies to: 19-20, 93-95, 134-134

custom_components/mobile_vikings/translations/fr.json (3)

15-15: LGTM: Platform mobile translation added.

The translation for the new mobile platform selector is concise and clear.


93-95: LGTM: Bundle info translation added.

The translation for the prepaid bundle information is clear and consistent with the French language style.


134-134: LGTM: Password description updated.

The password description has been appropriately updated to include JimMobile.

README.md (4)

5-8: LGTM: JimMobile logo added.

The JimMobile logo has been added with consistent styling matching the Mobile Vikings logo.


10-10: LGTM: Title and description updated.

The title and description clearly indicate support for both Mobile Vikings and JimMobile services.

Also applies to: 12-12


17-23: LGTM: Feature list clarified.

The feature list now clearly indicates which features are exclusive to Mobile Vikings.


169-173: LGTM: Code origin updated.

The code origin section has been appropriately updated to reflect both Mobile Vikings and JimMobile support.

custom_components/mobile_vikings/__init__.py Outdated Show resolved Hide resolved
custom_components/mobile_vikings/binary_sensor.py Outdated Show resolved Hide resolved
custom_components/mobile_vikings/sensor.py Outdated Show resolved Hide resolved
custom_components/mobile_vikings/translations/fr.json Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
custom_components/mobile_vikings/binary_sensor.py (2)

22-22: Remove unused import.

The JIM_MOBILE constant is imported but not used in this file.

-from .const import DOMAIN, MOBILE_VIKINGS, JIM_MOBILE
+from .const import DOMAIN, MOBILE_VIKINGS
🧰 Tools
🪛 Ruff (0.8.2)

22-22: .const.JIM_MOBILE imported but unused

Remove unused import: .const.JIM_MOBILE

(F401)


Line range hint 48-132: Reduce code duplication in sensor definitions.

The sensor definitions share a lot of common code, particularly in the lambda functions for device information. Consider extracting these into reusable functions.

Example refactor:

def get_device_name(data: dict) -> str:
    return "Subscription"

def get_device_identifier(data: dict) -> str:
    return f"Subscription {data.get('id', '')}"

def get_model(data: dict) -> str:
    return (
        f"{(data.get('sim') or {}).get('msisdn', '')} - "
        f"{safe_get(data, ['product', 'descriptions', 'title'], default='Unknown Product')}"
    )

SUBSCRIPTION_SENSOR_TYPES = (
    MobileVikingsBinarySensorDescription(
        # ... other fields ...
        device_name_fn=get_device_name,
        device_identifier_fn=get_device_identifier,
        model_fn=get_model,
        # ... other fields ...
    ),
    # ... other sensors ...
)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8c92dc4 and 49404d5.

📒 Files selected for processing (4)
  • custom_components/mobile_vikings/__init__.py (3 hunks)
  • custom_components/mobile_vikings/binary_sensor.py (9 hunks)
  • custom_components/mobile_vikings/sensor.py (29 hunks)
  • custom_components/mobile_vikings/translations/fr.json (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • custom_components/mobile_vikings/init.py
  • custom_components/mobile_vikings/sensor.py
  • custom_components/mobile_vikings/translations/fr.json
🧰 Additional context used
🪛 Ruff (0.8.2)
custom_components/mobile_vikings/binary_sensor.py

22-22: .const.JIM_MOBILE imported but unused

Remove unused import: .const.JIM_MOBILE

(F401)

🔇 Additional comments (3)
custom_components/mobile_vikings/binary_sensor.py (3)

45-45: LGTM! Well-designed platform filtering mechanism.

The addition of the mobile_platforms attribute with proper type hints enables clean platform-specific sensor filtering.


148-148: LGTM! Clean implementation of platform-specific sensor filtering.

The platform filtering logic is well-implemented with proper debug logging for skipped sensors.

Also applies to: 156-158


75-75: Consider which sensors should be shared with Jim Mobile.

All sensors are currently restricted to MOBILE_VIKINGS platform. If any of these alerts are also applicable to Jim Mobile, consider including both platforms:

mobile_platforms=(MOBILE_VIKINGS, JIM_MOBILE)

Also applies to: 104-104, 132-132

Copy link
Owner

Choose a reason for hiding this comment

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

Can you use snake_case (PEP 8) for your namings? So mobilePlatform needs to be mobile_platform

Copy link
Author

Choose a reason for hiding this comment

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

will update it later today

Copy link
Owner

Choose a reason for hiding this comment

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

No rush :-)

Copy link
Author

Choose a reason for hiding this comment

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

all done

@geertmeersman
Copy link
Owner

Thanks! will have a look at it.
Regarding authentication, did you reach out ot the MV / Jim team to get application credentials? I can do it too if you want

custom_components/mobile_vikings/const.py Fixed Show fixed Hide fixed
custom_components/mobile_vikings/const.py Fixed Show fixed Hide fixed
custom_components/mobile_vikings/const.py Fixed Show fixed Hide fixed
custom_components/mobile_vikings/const.py Fixed Show resolved Hide resolved
@myTselection
Copy link
Author

Thanks! will have a look at it. Regarding authentication, did you reach out ot the MV / Jim team to get application credentials? I can do it too if you want

didn't need to, no client_secret is used for JIM mobile it seems...But if you prefer feel free to do so.

@geertmeersman geertmeersman changed the title Jim Mobile support feat: Jim Mobile support Jan 30, 2025
@geertmeersman geertmeersman added major A change requiring a major version bump feat A new feature labels Jan 30, 2025
…be mobile_platform

cleanup const for unused secrete and comments
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (3)
custom_components/mobile_vikings/binary_sensor.py (1)

22-22: Remove unused import.

The JIM_MOBILE import is not used in this file.

-from .const import DOMAIN, MOBILE_VIKINGS, JIM_MOBILE
+from .const import DOMAIN, MOBILE_VIKINGS
🧰 Tools
🪛 Ruff (0.8.2)

22-22: .const.JIM_MOBILE imported but unused

Remove unused import: .const.JIM_MOBILE

(F401)

custom_components/mobile_vikings/client.py (2)

8-19: Fix trailing whitespace in imports.

Remove trailing whitespace after the imported constants.

-from .const import (
-    BASE_URL, 
-    BASE_URL_JIMMOBILE, 
-    CLIENT_ID, 
-    CLIENT_ID_JIMMOBILE, 
-    CLIENT_SECRET, 
-    CLIENT_SECRET_TAG, 
-    CLIENT_SECRET_TAG_JIMMOBILE, 
-    CLIENT_SECRET_VALUE_JIMMOBILE, 
-    MOBILE_VIKINGS, 
-    JIM_MOBILE
-)
+from .const import (
+    BASE_URL,
+    BASE_URL_JIMMOBILE,
+    CLIENT_ID,
+    CLIENT_ID_JIMMOBILE,
+    CLIENT_SECRET,
+    CLIENT_SECRET_TAG,
+    CLIENT_SECRET_TAG_JIMMOBILE,
+    CLIENT_SECRET_VALUE_JIMMOBILE,
+    MOBILE_VIKINGS,
+    JIM_MOBILE
+)
🧰 Tools
🪛 Ruff (0.8.2)

9-9: Trailing whitespace

Remove trailing whitespace

(W291)


10-10: Trailing whitespace

Remove trailing whitespace

(W291)


11-11: Trailing whitespace

Remove trailing whitespace

(W291)


12-12: Trailing whitespace

Remove trailing whitespace

(W291)


13-13: Trailing whitespace

Remove trailing whitespace

(W291)


14-14: Trailing whitespace

Remove trailing whitespace

(W291)


15-15: Trailing whitespace

Remove trailing whitespace

(W291)


16-16: Trailing whitespace

Remove trailing whitespace

(W291)


17-17: Trailing whitespace

Remove trailing whitespace

(W291)


227-229: Consider improving error handling for unsupported features.

Instead of returning a dictionary with an error message, consider raising a NotImplementedError or a custom exception for unsupported features.

-        if self.mobile_platform == JIM_MOBILE:
-            # not existing for Jim Mobile
-            return {'error': 'not existing for Jim Mobile'}
+        if self.mobile_platform == JIM_MOBILE:
+            raise NotImplementedError("Loyalty points are not supported for Jim Mobile")
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 49404d5 and 9d0a5c9.

📒 Files selected for processing (11)
  • custom_components/mobile_vikings/__init__.py (2 hunks)
  • custom_components/mobile_vikings/binary_sensor.py (9 hunks)
  • custom_components/mobile_vikings/client.py (8 hunks)
  • custom_components/mobile_vikings/config_flow.py (4 hunks)
  • custom_components/mobile_vikings/const.py (1 hunks)
  • custom_components/mobile_vikings/entity.py (3 hunks)
  • custom_components/mobile_vikings/models.py (1 hunks)
  • custom_components/mobile_vikings/sensor.py (29 hunks)
  • custom_components/mobile_vikings/translations/en.json (3 hunks)
  • custom_components/mobile_vikings/translations/fr.json (3 hunks)
  • custom_components/mobile_vikings/translations/nl.json (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
  • custom_components/mobile_vikings/models.py
  • custom_components/mobile_vikings/init.py
  • custom_components/mobile_vikings/entity.py
  • custom_components/mobile_vikings/translations/en.json
  • custom_components/mobile_vikings/const.py
  • custom_components/mobile_vikings/translations/fr.json
  • custom_components/mobile_vikings/sensor.py
  • custom_components/mobile_vikings/translations/nl.json
🧰 Additional context used
🪛 Ruff (0.8.2)
custom_components/mobile_vikings/binary_sensor.py

22-22: .const.JIM_MOBILE imported but unused

Remove unused import: .const.JIM_MOBILE

(F401)

custom_components/mobile_vikings/client.py

9-9: Trailing whitespace

Remove trailing whitespace

(W291)


10-10: Trailing whitespace

Remove trailing whitespace

(W291)


11-11: Trailing whitespace

Remove trailing whitespace

(W291)


12-12: Trailing whitespace

Remove trailing whitespace

(W291)


13-13: Trailing whitespace

Remove trailing whitespace

(W291)


14-14: Trailing whitespace

Remove trailing whitespace

(W291)


15-15: Trailing whitespace

Remove trailing whitespace

(W291)


16-16: Trailing whitespace

Remove trailing whitespace

(W291)


17-17: Trailing whitespace

Remove trailing whitespace

(W291)

custom_components/mobile_vikings/config_flow.py

16-16: Trailing whitespace

Remove trailing whitespace

(W291)


17-17: Trailing whitespace

Remove trailing whitespace

(W291)

🔇 Additional comments (8)
custom_components/mobile_vikings/config_flow.py (3)

16-18: LGTM! Import statements are correctly organized.

The new SelectSelector imports are properly added for implementing the platform selection dropdown.

🧰 Tools
🪛 Ruff (0.8.2)

16-16: Trailing whitespace

Remove trailing whitespace

(W291)


17-17: Trailing whitespace

Remove trailing whitespace

(W291)


33-33: LGTM! Configuration data structure is properly extended.

The mobile_platform field is correctly added to DEFAULT_ENTRY_DATA to store the selected platform.


89-94: LGTM! Platform selection is well implemented.

The platform selection dropdown is properly implemented using SelectSelector with appropriate options.

custom_components/mobile_vikings/binary_sensor.py (2)

45-45: LGTM! Sensor description is properly extended.

The mobile_platforms attribute is correctly added to enable platform-specific sensor filtering.


156-158: LGTM! Platform filtering is well implemented.

The sensor filtering logic correctly checks for platform compatibility before adding sensors.

custom_components/mobile_vikings/client.py (3)

Line range hint 33-53: LGTM! Client initialization is properly extended.

The mobile_platform parameter is correctly added to the constructor with proper documentation.


72-75: LGTM! Authentication is properly adapted.

The authentication logic correctly handles platform-specific client IDs and secrets.


157-160: LGTM! URL selection is well implemented.

The platform-specific base URL selection is correctly implemented.

Comment on lines +80 to 84
if user_input['mobile_platform'] == JIM_MOBILE:
await self.async_set_unique_id(f"{DOMAIN}_{JIM_MOBILE}_{user_input[CONF_USERNAME]}")
else:
await self.async_set_unique_id(f"{DOMAIN}_{user_input[CONF_USERNAME]}")
self._abort_if_unique_id_configured()
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

The unique ID implementation needs to be updated to prevent conflicts

The review comment is correct. The current implementation could cause conflicts if a user has accounts with the same username on both platforms. The codebase shows evidence of supporting both platforms simultaneously, and there are several sensors that work with both MOBILE_VIKINGS and JIM_MOBILE platforms. For consistency and to prevent potential conflicts, the unique ID should include the platform name for both cases.

The suggested fix in the review comment is the correct approach:

await self.async_set_unique_id(f"{DOMAIN}_{user_input['mobile_platform']}_{user_input[CONF_USERNAME]}")
🔗 Analysis chain

Verify the unique ID generation logic.

The unique ID generation now includes the platform for JIM_MOBILE but not for MOBILE_VIKINGS. This might cause conflicts if a user has accounts with the same username on both platforms.

Consider using the platform in the unique ID for both cases:

-                if user_input['mobile_platform'] == JIM_MOBILE:
-                    await self.async_set_unique_id(f"{DOMAIN}_{JIM_MOBILE}_{user_input[CONF_USERNAME]}")
-                else:
-                    await self.async_set_unique_id(f"{DOMAIN}_{user_input[CONF_USERNAME]}")
+                await self.async_set_unique_id(f"{DOMAIN}_{user_input['mobile_platform']}_{user_input[CONF_USERNAME]}")
🏁 Scripts executed

The following scripts were executed for the analysis:

Error: No shell scripts found


Script:

#!/bin/bash
# Search for MOBILE_VIKINGS constant and its usage
rg "MOBILE_VIKINGS" -A 2 -B 2

# Search for unique ID related code
rg "async_set_unique_id" -A 3 -B 3

# Look for config flow implementations
ast-grep --pattern 'class $_(ConfigFlow):'

Length of output: 14186

CLIENT_ID = "orIbwDyPep4Cju3CksWC4AXiIezY6JVHDix8I9pL"

# Client secret for OAuth2 authentication
CLIENT_SECRET = "QquFECtjqYev6DgApjrBsOzTPFwCEv8DTBQOBMSNs77YtwIzxPGagNhDpwwt8wxOwP8B4nd4gCTvVZfuWccTfKCPSh1xVruqHjrFBs1fH4Y8lSSRcw7PPL1QlcZwAY24"

CLIENT_SECRET_TAG = "client_secret"

Check notice

Code scanning / Bandit

Possible hardcoded password: 'client_secret' Note

Possible hardcoded password: 'client_secret'
CLIENT_ID = "orIbwDyPep4Cju3CksWC4AXiIezY6JVHDix8I9pL"

# Client secret for OAuth2 authentication
CLIENT_SECRET = "QquFECtjqYev6DgApjrBsOzTPFwCEv8DTBQOBMSNs77YtwIzxPGagNhDpwwt8wxOwP8B4nd4gCTvVZfuWccTfKCPSh1xVruqHjrFBs1fH4Y8lSSRcw7PPL1QlcZwAY24"

CLIENT_SECRET_TAG = "client_secret"

CLIENT_SECRET_TAG_JIMMOBILE = "grant_type"

Check notice

Code scanning / Bandit

Possible hardcoded password: 'grant_type' Note

Possible hardcoded password: 'grant_type'
CLIENT_SECRET_TAG = "client_secret"

CLIENT_SECRET_TAG_JIMMOBILE = "grant_type"
CLIENT_SECRET_VALUE_JIMMOBILE = "password"

Check notice

Code scanning / Bandit

Possible hardcoded password: 'password' Note

Possible hardcoded password: 'password'
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat A new feature major A change requiring a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants