Adds organisation API support #30
Conversation
Apart from operating a single charge station, the Charge Amps API also allows controlling (part) of an organization's fleet of charge stations and users. Since this is most likely an uncommon usecase, I placed it behind a feature flag "organisations". This does not add support for CLI flags for accessing the API, but would be a nice future addition.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes in this pull request introduce several enhancements to the Charge Amps Python module. Notably, a new section in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OrganisationClient
participant API
User->>OrganisationClient: Request to get organisations
OrganisationClient->>API: GET /organisations
API-->>OrganisationClient: Return list of organisations
OrganisationClient-->>User: Return organisations list
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (6)
chargeamps/organisation.py (3)
53-56: Simplify list constructions using list comprehensionsIn several methods, you are building lists using loops where list comprehensions can enhance readability and conciseness.
For example, in
get_organisation_chargepoints:payload = await response.json() - res = [] - for charge_point in payload: - res.append(ChargePoint.model_validate(charge_point)) - return res + return [ChargePoint.model_validate(cp) for cp in payload]Apply similar changes in the following methods:
get_organisation_chargepoint_statuses(lines 66-69)get_organisation_rfids(lines 140-143)get_organisation_users(lines 186-189)Also applies to: 66-69, 140-143, 186-189
222-222: Replace print statement with proper loggingUsing
- print(payload) + self._logger.debug(f"Payload: {payload}")
215-220: Align password length validation condition and error messageThe condition checks if
len(password) >= 8, but the error message states "must exceed 8 characters," implyinglen(password) > 8. To avoid confusion, align the condition with the error message.Option 1: Change the condition to match the error message:
- if len(password) >= 8: + if len(password) > 8:Option 2: Update the error message to match the condition:
else: raise ValueError( - "The provided password is too short, must exceed 8 characters" + "The provided password is too short, must be at least 8 characters" )README.md (1)
11-13: Specify the language identifier in fenced code blocksThe fenced code blocks on lines 11-13 and 18-19 do not specify a language, which affects syntax highlighting and readability. This issue is flagged by Markdownlint.
Add the language identifier (e.g.,
bash) to the code blocks:```bash pip install chargeamps```diff ```bash pip install chargeamps["organisations"]Also applies to: 18-19 <details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.35.0)</summary> 11-11: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </blockquote></details> <details> <summary>chargeamps/external.py (2)</summary><blockquote> `49-49`: **Consider thread-safety implications of the _user field** The `_user` field maintains state that could lead to race conditions in concurrent usage scenarios. Consider making this field immutable or implementing proper synchronization. --- `78-78`: **Ensure consistent state management across token operations** The `_user` field is managed across multiple locations. Consider centralizing this logic to prevent inconsistencies. Consider creating a dedicated method for user state management: ```python def _clear_auth_state(self): """Clear all authentication-related state.""" self._token = None self._user = None self._refresh_token = None self._token_expire = 0 def _update_auth_state(self, response_payload: dict): """Update authentication state from response payload.""" self._token = response_payload["token"] self._user = response_payload["user"] self._refresh_token = response_payload["refreshToken"] # Update token expiration...Also applies to: 96-96, 108-108
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
README.md(1 hunks)chargeamps/external.py(7 hunks)chargeamps/models.py(2 hunks)chargeamps/organisation.py(1 hunks)examples/org_test.py(1 hunks)examples/test.py(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.35.0)
README.md
11-11: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
17-17: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
chargeamps/organisation.py (1)
123-125:
Fix incorrect usage of await payload.json() in loop
In the get_organisation_charging_sessions method, you're calling await payload.json() inside the loop, but payload is already a JSON-decoded object. This will raise an error.
Update the loop to iterate directly over payload:
- for session in await payload.json():
+ for session in payload:Likely invalid or redundant comment.
examples/test.py (1)
8-8: Change looks good
The update to read from "config.json" instead of "credentials.json" ensures consistency with other example scripts.
chargeamps/models.py
Outdated
| def feature_required(feature_flag): | ||
| def decorator(cls): | ||
| if feature_flag: | ||
| return cls | ||
| else: | ||
| raise ImportError(f"Feature '{feature_flag}' is not enabled") | ||
|
|
||
| return decorator |
There was a problem hiding this comment.
Fix the feature_required decorator to correctly enforce feature flags
The current implementation of the feature_required decorator checks if feature_flag is truthy, but since feature_flag is a non-empty string (e.g., "organisations"), this condition will always be true. As a result, the feature gating doesn't function as intended. Instead, the decorator should check whether the feature is enabled in the application settings or configuration.
Consider modifying the decorator to check a global settings object or configuration that maintains the status of feature flags. For example:
def feature_required(feature_flag):
def decorator(cls):
- if feature_flag:
+ if settings.FEATURE_FLAGS.get(feature_flag, False):
return cls
else:
raise ImportError(f"Feature '{feature_flag}' is not enabled")
return decoratorEnsure that you import the settings module or wherever the feature flags are defined.
Committable suggestion skipped: line range outside the PR's diff.
chargeamps/organisation.py
Outdated
| async def revoke_organisation_rfid(self, org_id: str, rfid: Rfid) -> None: | ||
| """Revoke an RFID tag""" | ||
| request_uri = f"/api/{API_VERSION}/organisations/{org_id}/rfids/revoke" | ||
| rfid_id = rfid.rfid | ||
| payload = rfid_id.toJson() | ||
| await self._put(request_uri, json=payload) |
There was a problem hiding this comment.
Fix incorrect use of toJson() on rfid_id in revoke_organisation_rfid
The variable rfid_id is a string, and strings do not have a toJson() method. This will raise an AttributeError.
Modify the code to construct the payload correctly:
rfid_id = rfid.rfid
- payload = rfid_id.toJson()
+ payload = {"rfid": rfid_id}
await self._put(request_uri, json=payload)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def revoke_organisation_rfid(self, org_id: str, rfid: Rfid) -> None: | |
| """Revoke an RFID tag""" | |
| request_uri = f"/api/{API_VERSION}/organisations/{org_id}/rfids/revoke" | |
| rfid_id = rfid.rfid | |
| payload = rfid_id.toJson() | |
| await self._put(request_uri, json=payload) | |
| async def revoke_organisation_rfid(self, org_id: str, rfid: Rfid) -> None: | |
| """Revoke an RFID tag""" | |
| request_uri = f"/api/{API_VERSION}/organisations/{org_id}/rfids/revoke" | |
| rfid_id = rfid.rfid | |
| payload = {"rfid": rfid_id} | |
| await self._put(request_uri, json=payload) |
chargeamps/organisation.py
Outdated
| rfid: [Rfid] = None, | ||
| password: str = None, | ||
| ) -> User: | ||
| request_uri = f"/api/{API_VERSION}/organisations/{org_id}/users" | ||
| payload = {} | ||
|
|
||
| if first_name: | ||
| payload["firstName"] = first_name | ||
| if last_name: | ||
| payload["lastName"] = last_name | ||
| if email: | ||
| payload["email"] = email | ||
| if mobile: | ||
| payload["mobile"] = mobile | ||
| if rfid: | ||
| payload["rfidTags"] = rfid.model_dump(by_alias=True) | ||
| if password: |
There was a problem hiding this comment.
Correct handling of rfid parameter in add_organisation_user
The parameter rfid is annotated as [Rfid], which is invalid syntax. Additionally, when adding it to the payload, you're calling model_dump on a list, which will cause an error.
Update the type annotation and adjust the payload construction:
- rfid: [Rfid] = None,
+ rfid: Optional[list[Rfid]] = None,
...
if rfid:
- payload["rfidTags"] = rfid.model_dump(by_alias=True)
+ payload["rfidTags"] = [tag.model_dump(by_alias=True) for tag in rfid]Ensure you import Optional and list from the typing module.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rfid: [Rfid] = None, | |
| password: str = None, | |
| ) -> User: | |
| request_uri = f"/api/{API_VERSION}/organisations/{org_id}/users" | |
| payload = {} | |
| if first_name: | |
| payload["firstName"] = first_name | |
| if last_name: | |
| payload["lastName"] = last_name | |
| if email: | |
| payload["email"] = email | |
| if mobile: | |
| payload["mobile"] = mobile | |
| if rfid: | |
| payload["rfidTags"] = rfid.model_dump(by_alias=True) | |
| if password: | |
| rfid: Optional[list[Rfid]] = None, | |
| password: str = None, | |
| ) -> User: | |
| request_uri = f"/api/{API_VERSION}/organisations/{org_id}/users" | |
| payload = {} | |
| if first_name: | |
| payload["firstName"] = first_name | |
| if last_name: | |
| payload["lastName"] = last_name | |
| if email: | |
| payload["email"] = email | |
| if mobile: | |
| payload["mobile"] = mobile | |
| if rfid: | |
| payload["rfidTags"] = [tag.model_dump(by_alias=True) for tag in rfid] | |
| if password: |
chargeamps/organisation.py
Outdated
| def verify_rfid( | ||
| self, rfid: str, rfid_format: str, rfid_length: str, rfid_dec_format_length: str | ||
| ) -> dict[str, str]: | ||
| result = {} | ||
| result["rfid"] = rfid | ||
| if rfid_format == "Hex": | ||
| # TODO: Should actual `rfid` param be verified against the length here? | ||
| if not rfid_length: | ||
| return result | ||
| elif rfid_format == "Dec" or rfid_format == "ReverseDec": | ||
| if rfid_dec_format_length: | ||
| result["rfidDecimalFormatLength"] = rfid_dec_format_length | ||
| else: | ||
| self._logger.error("Invalid RFID format") | ||
| return {} | ||
|
|
||
| if rfid_length in {4, 7, 10}: | ||
| result["rfidLength"] = rfid_length | ||
| else: | ||
| self._logger.error("RFID length invalid, should be either 4, 7 or 10 bytes") | ||
| return {} | ||
|
|
||
| result["rfidFormat"] = rfid_format | ||
| return result | ||
|
|
There was a problem hiding this comment.
Correct parameter types and logic in verify_rfid method
The parameters rfid_length and rfid_dec_format_length are used as integers in comparisons, but are typed as str in the method signature. This can lead to type errors during runtime. Additionally, there's a TODO comment regarding RFID length verification.
Update the parameter types to int and implement the RFID length validation. Here's the corrected method signature and adjustments:
def verify_rfid(
- self, rfid: str, rfid_format: str, rfid_length: str, rfid_dec_format_length: str
+ self, rfid: str, rfid_format: str, rfid_length: int = None, rfid_dec_format_length: int = None
) -> dict[str, str]:Adjust the logic within the method to handle None values and perform appropriate integer comparisons.
Would you like assistance in implementing the RFID length validation logic or updating the method accordingly?
Committable suggestion skipped: line range outside the PR's diff.
chargeamps/organisation.py
Outdated
| async def add_organisation_rfid( | ||
| self, org_id: str, rfid: Rfid, rfid_dec_format_length: int = None | ||
| ) -> RfidTag: | ||
| """Add a new RFID tag to the organisation""" | ||
| request_uri = f"/api/{API_VERSION}/organisations/{org_id}/rfids" | ||
| payload = rfid.model_dump(by_alias=True) | ||
| if rfid_dec_format_length: | ||
| payload["rfidDecimalFormatLength"] = rfid_dec_format_length | ||
| await self._put(request_uri, json=payload) | ||
|
|
There was a problem hiding this comment.
Handle missing return value in add_organisation_rfid
The method add_organisation_rfid is expected to return an RfidTag object, but currently, it doesn't return anything.
Modify the method to process the response and return the new RfidTag:
await self._put(request_uri, json=payload)
+ response = await self._put(request_uri, json=payload)
+ payload = await response.json()
+ return RfidTag.model_validate(payload)Committable suggestion skipped: line range outside the PR's diff.
examples/org_test.py
Outdated
| if __name__ == "__main__": | ||
| # asyncio.run(test()) | ||
| loop = asyncio.get_event_loop() | ||
| loop.run_until_complete(test()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use asyncio.run() instead of manual event loop handling
The manual event loop handling is deprecated. Use asyncio.run() for better maintainability.
- loop = asyncio.get_event_loop()
- loop.run_until_complete(test())
+ asyncio.run(test())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if __name__ == "__main__": | |
| # asyncio.run(test()) | |
| loop = asyncio.get_event_loop() | |
| loop.run_until_complete(test()) | |
| if __name__ == "__main__": | |
| # asyncio.run(test()) | |
| asyncio.run(test()) |
| chargepoints = await client.get_organisation_chargepoints() | ||
| print(chargepoints) | ||
|
|
||
| status = await client.get_chargepoint_status(chargepoints[0].id) | ||
| print("Status:", status) | ||
|
|
||
| for c in status.connector_statuses: | ||
| settings = await client.get_chargepoint_connector_settings( | ||
| c.charge_point_id, c.connector_id | ||
| ) | ||
| print("Before:", settings) | ||
| settings.max_current = 11 | ||
| await client.set_chargepoint_connector_settings(settings) | ||
| print("After:", settings) | ||
|
|
There was a problem hiding this comment.
Add error handling for API operations
The script performs critical operations (fetching status, modifying settings) without proper error handling. This could lead to partial updates or silent failures.
Example implementation:
+async def modify_connector_settings(client, chargepoint_id, connector_id, new_current):
+ try:
+ settings = await client.get_chargepoint_connector_settings(
+ chargepoint_id, connector_id
+ )
+ print(f"Current settings: {settings}")
+
+ settings.max_current = new_current
+ await client.set_chargepoint_connector_settings(settings)
+ print(f"Settings updated successfully: {settings}")
+ except Exception as e:
+ print(f"Failed to update settings: {e}")
+ raiseCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Again, example is not meant to be run for more than just trying out the module. It is kept similar to the existing example
There was a problem hiding this comment.
@yarcod, understood. I'll note that the example scripts in the examples/ directory are intended for trying out the module and are kept similar to existing examples.
✏️ Learnings added
Learnt from: yarcod
PR: kirei/python-chargeamps#30
File: examples/org_test.py:14-28
Timestamp: 2024-12-17T22:35:19.341Z
Learning: In the `examples/` directory, Python scripts like `org_test.py` are intended primarily for trying out the module and are kept similar to existing examples, and may not include comprehensive error handling.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| c.charge_point_id, c.connector_id | ||
| ) | ||
| print("Before:", settings) | ||
| settings.max_current = 11 |
There was a problem hiding this comment.
💡 Codebase verification
Hardcoded value is inconsistent with configurable design
The codebase already implements a configurable approach for max_current through:
- CLI argument
--currentin chargeamps/cli.py - Optional field in models.py (
max_current: Optional[float]) - Different values used across test files (11A in org_test.py, 6A in test.py)
The hardcoded value of 11 in org_test.py should follow the same configurable pattern used elsewhere in the codebase.
🔗 Analysis chain
Avoid hardcoding configuration values
The max_current value of 11 is hardcoded. Consider making this configurable to prevent accidental modifications.
Let me check if this value is defined elsewhere or if there's a configuration mechanism in place.
Let me check the configuration file and CLI argument handling to understand how max_current is typically configured.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if this value is used consistently across the codebase
rg "max_current\s*=\s*11" --type py
Length of output: 92
Script:
#!/bin/bash
# Search for max_current usage and definition patterns
rg "max_current" --type py -A 2
# Look for potential configuration files
fd -e yaml -e yml -e json -e ini -e conf -e cfg -e toml
# Check if there's a settings class definition
ast-grep --pattern 'class $_ {
$$$
max_current
$$$
}'
Length of output: 1065
Script:
#!/bin/bash
# Check the config file content
cat examples/config.json
# Look at the CLI argument parser setup
rg -B 5 "dest=\"max_current\"" chargeamps/cli.py
Length of output: 374
There was a problem hiding this comment.
Not sure why just this function needed more detailed documentation, but similarly to above I will keep both the docstring and error handling consistent with the rest of the module.
There was a problem hiding this comment.
@yarcod, understood. Thank you for keeping both the docstring and error handling consistent with the rest of the module.
✏️ Learnings added
Learnt from: yarcod
PR: kirei/python-chargeamps#30
File: examples/org_test.py:25-25
Timestamp: 2024-12-17T22:35:45.702Z
Learning: In the `python-chargeamps` project, when suggesting improvements to docstrings or error handling, ensure recommendations are consistent with existing coding practices in the codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
chargeamps/external.py
Outdated
| async def get_logged_in_user(self) -> User: | ||
| """Get authenticated user info""" | ||
| user_id = self._user["id"] | ||
|
|
||
| request_uri = f"/api/{API_VERSION}/users/{user_id}" | ||
| response = await self._get(request_uri) | ||
| payload = await response.json() | ||
|
|
||
| return User.model_validate(payload) |
There was a problem hiding this comment.
Improve error handling and type safety in get_logged_in_user
The method assumes self._user is a dict and contains an 'id' key without validation.
async def get_logged_in_user(self) -> User:
"""Get authenticated user info"""
+ if not self._user or not isinstance(self._user, dict):
+ raise ValueError("No user is currently logged in")
+
+ user_id = self._user.get("id")
+ if not user_id:
+ raise ValueError("Invalid user data: missing id")
- user_id = self._user["id"]
request_uri = f"/api/{API_VERSION}/users/{user_id}"
- response = await self._get(request_uri)
- payload = await response.json()
-
- return User.model_validate(payload)
+ try:
+ response = await self._get(request_uri)
+ payload = await response.json()
+ return User.model_validate(payload)
+ except Exception as e:
+ self._logger.error(f"Failed to get user info: {e}")
+ raiseCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Since the user class variable is of type User, and id of that class is not optional, there will always be an id there as long as a user has been assigned. Will add a check for if there is currently a user logged in though.
Regarding the error handling, this follows the same design pattern as elsewhere in the module. While good to have, I think it is better to keep the design consistent across the module and instead add error handling separately at a later stage if deemed necessary.
There was a problem hiding this comment.
@yarcod, understood. Thank you for clarifying that self._user is of type User and always contains an id when a user is assigned. Adding a check for whether a user is currently logged in makes sense.
Regarding error handling, I acknowledge your point about maintaining design consistency across the module. Implementing error handling uniformly in a future update is a reasonable approach.
✏️ Learnings added
Learnt from: yarcod
PR: kirei/python-chargeamps#30
File: chargeamps/external.py:243-251
Timestamp: 2024-12-17T22:36:01.618Z
Learning: In `chargeamps/external.py`, `self._user` is of type `User`, and `id` is not optional; thus, `self._user["id"]` will always exist when a user is assigned.
Learnt from: yarcod
PR: kirei/python-chargeamps#30
File: chargeamps/external.py:243-251
Timestamp: 2024-12-17T22:36:01.618Z
Learning: In `chargeamps/external.py`, error handling follows a consistent design pattern across the module; additional error handling should be added separately to maintain consistency.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| async def get_partner(self, charge_point_id: str) -> Partner: | ||
| """Get partner details""" | ||
| request_uri = f"/api/{API_VERSION}/chargepoints/{charge_point_id}/partner" | ||
| response = await self._get(request_uri) | ||
| payload = await response.json() | ||
| return Partner.model_validate(payload) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and documentation for get_partner method
The method needs better error handling and documentation of possible exceptions.
async def get_partner(self, charge_point_id: str) -> Partner:
- """Get partner details"""
+ """Get partner details for a specific charge point.
+
+ Args:
+ charge_point_id: The ID of the charge point
+
+ Returns:
+ Partner: The partner details
+
+ Raises:
+ HTTPException: If the API request fails
+ ValidationError: If the response data is invalid
+ """
request_uri = f"/api/{API_VERSION}/chargepoints/{charge_point_id}/partner"
- response = await self._get(request_uri)
- payload = await response.json()
- return Partner.model_validate(payload)
+ try:
+ response = await self._get(request_uri)
+ payload = await response.json()
+ return Partner.model_validate(payload)
+ except Exception as e:
+ self._logger.error(f"Failed to get partner details: {e}")
+ raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def get_partner(self, charge_point_id: str) -> Partner: | |
| """Get partner details""" | |
| request_uri = f"/api/{API_VERSION}/chargepoints/{charge_point_id}/partner" | |
| response = await self._get(request_uri) | |
| payload = await response.json() | |
| return Partner.model_validate(payload) | |
| async def get_partner(self, charge_point_id: str) -> Partner: | |
| """Get partner details for a specific charge point. | |
| Args: | |
| charge_point_id: The ID of the charge point | |
| Returns: | |
| Partner: The partner details | |
| Raises: | |
| HTTPException: If the API request fails | |
| ValidationError: If the response data is invalid | |
| """ | |
| request_uri = f"/api/{API_VERSION}/chargepoints/{charge_point_id}/partner" | |
| try: | |
| response = await self._get(request_uri) | |
| payload = await response.json() | |
| return Partner.model_validate(payload) | |
| except Exception as e: | |
| self._logger.error(f"Failed to get partner details: {e}") | |
| raise |
There was a problem hiding this comment.
Not sure why just this function needed more detailed documentation, but similarly to above I will keep both the docstring and error handling consistent with the rest of the module.
There was a problem hiding this comment.
@yarcod, understood. Maintaining consistency in docstrings and error handling across the module makes sense. Thank you for clarifying.
✏️ Learnings added
Learnt from: yarcod
PR: kirei/python-chargeamps#30
File: chargeamps/external.py:218-223
Timestamp: 2024-12-17T22:36:20.401Z
Learning: In `chargeamps/external.py`, ensure that docstrings and error handling remain consistent with the rest of the module, avoiding the addition of detailed documentation or extensive error handling to individual functions unless it's applied uniformly.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
Please fix issues and mark as ready for review |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
chargeamps/organisation.py (1)
99-99: Remove unnecessary f-string prefixThe f-string on line 99 does not contain any placeholders, so the
fprefix is unnecessary and can be removed.Apply this diff to fix the issue:
- raise ValueError(f"RFID length must be 7 bytes if the (default) format type 'Hex' is set.") + raise ValueError("RFID length must be 7 bytes if the (default) format type 'Hex' is set.")🧰 Tools
🪛 Ruff (0.8.2)
99-99: f-string without any placeholders
Remove extraneous
fprefix(F541)
chargeamps/external.py (1)
243-254: Simplifyget_logged_in_userby returning cached user dataSince
self._usernow holds a validatedUserinstance, you can simplify theget_logged_in_usermethod by returning the cached user data without making an additional API call.Apply this diff to streamline the method:
async def get_logged_in_user(self) -> User: """Get authenticated user info""" if not self._user: raise ValueError("No user is currently logged in") - user_id = self._user.id - request_uri = f"/api/{API_VERSION}/users/{user_id}" - response = await self._get(request_uri) - payload = await response.json() - return User.model_validate(payload) + return self._user
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
chargeamps/external.py(7 hunks)chargeamps/models.py(2 hunks)chargeamps/organisation.py(1 hunks)examples/org_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/org_test.py
🧰 Additional context used
🪛 Ruff (0.8.2)
chargeamps/organisation.py
99-99: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (2)
chargeamps/organisation.py (1)
84-106: Correct parameter types in verify_rfid method
The parameters rfid_length and rfid_dec_format_length are typed as str | None, but they are used as integers in comparisons. This can lead to type errors at runtime. Update the parameter types to int | None for consistency.
🧰 Tools
🪛 Ruff (0.8.2)
99-99: f-string without any placeholders
Remove extraneous f prefix
(F541)
chargeamps/models.py (1)
18-24: Fix the feature_required decorator to correctly enforce feature flags
The current implementation of the feature_required decorator only allows the "organisations" feature, regardless of actual feature flag settings. This does not correctly enforce feature flags as intended.
Consider modifying the decorator to check a configuration or settings object that maintains the status of feature flags. For example:
def feature_required(feature_flag):
def decorator(cls):
- if feature_flag == "organisations":
+ if settings.FEATURE_FLAGS.get(feature_flag, False):
return cls
else:
raise ImportError(f"Feature '{feature_flag}' is not enabled")
return decoratorEnsure you import the settings module or define where FEATURE_FLAGS are stored.
| """Get organisation's registered users""" | ||
| query_params = {} |
There was a problem hiding this comment.
Pass query parameters in get_organisation_user
The query_params dictionary is constructed but not passed to the _get method. As a result, any query parameters are not being included in the request.
Apply this diff to fix the issue:
- response = await self._get(request_uri)
+ response = await self._get(request_uri, params=query_params)Committable suggestion skipped: line range outside the PR's diff.
| @@ -100,6 +105,7 @@ async def _ensure_token(self) -> None: | |||
| response_payload = await response.json() | |||
|
|
|||
| self._token = response_payload["token"] | |||
| self._user = response_payload["user"] | |||
There was a problem hiding this comment.
Ensure consistent typing of self._user
The instance variable self._user is annotated as User but is assigned a dictionary from response_payload["user"]. This can lead to type inconsistencies and potential errors.
Apply this diff to validate and assign the user data correctly:
- self._user = response_payload["user"]
+ self._user = User.model_validate(response_payload["user"])This ensures self._user is always a User instance.
Additionally, in the get_logged_in_user method, you can return self._user directly:
- user_id = self._user["id"]
- request_uri = f"/api/{API_VERSION}/users/{user_id}"
- response = await self._get(request_uri)
- payload = await response.json()
- return User.model_validate(payload)
+ return self._userCommittable suggestion skipped: line range outside the PR's diff.
Apart from operating a single charge station, the Charge Amps API also allows controlling (part) of an organisation's fleet of charge stations and users. Since this is most likely an uncommon usecase, I placed it behind a feature flag "organisations".
This does not add support for CLI flags for accessing the API, but would be a nice future addition.
Summary by CodeRabbit
Release Notes
New Features
OrganisationClientclass for managing organization-related API interactions.OrganisationClientfor retrieving and manipulating organization data.Bug Fixes
Documentation
OrganisationClient.Chores
credentials.jsontoconfig.jsonin the testing scripts.