Updated pagination to allow transaction id #91
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Migrated to UV
Reviewer's GuideThis PR extends transaction pagination to accept string IDs and performs a full migration to UV for build, test, lint, and publishing workflows—moving metadata into pyproject.toml, replacing legacy CI, and standardizing code style. ER diagram for project metadata migration to pyproject.tomlerDiagram
PROJECT ||--o{ DEPENDENCY_GROUPS : contains
PROJECT {
string name
string version
string description
string license
string requires-python
string[] classifiers
string[] authors
string readme
}
DEPENDENCY_GROUPS {
string name
string[] packages
}
PROJECT ||--o{ URLS : has
URLS {
string homepage
string repository
string issues
string documentation
}
Class diagram for updated Transaction.fetch paginationclassDiagram
class Transaction {
+fetch(auth: Authentication, account_id: str, since: datetime | str = None, before: datetime = None, expand = None, limit = 30) List[Transaction]
}
class Authentication
Transaction --> Authentication: uses
note for Transaction "'since' parameter now accepts datetime or string (transaction ID) for pagination"
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `monzo/endpoints/attachment.py:142-145` </location>
<code_context>
file_url = urlparse(url)
_, file_extension = splitext(url)
if file_extension not in SUPPORTED_ATTACHMENT_EXTENSIONS:
- raise MonzoGeneralError('Unsupported file type')
+ raise MonzoGeneralError("Unsupported file type")
</code_context>
<issue_to_address>
**suggestion:** File extension check may fail for uppercase or mixed-case extensions.
Normalize the file extension to lowercase before checking against SUPPORTED_ATTACHMENT_EXTENSIONS to ensure all valid cases are accepted.
```suggestion
_, file_extension = splitext(url)
file_extension = file_extension.lower()
if file_extension not in SUPPORTED_ATTACHMENT_EXTENSIONS:
raise MonzoGeneralError("Unsupported file type")
file_type = SUPPORTED_ATTACHMENT_EXTENSIONS[file_extension]
```
</issue_to_address>
### Comment 2
<location> `monzo/endpoints/attachment.py:146` </location>
<code_context>
- raise MonzoGeneralError('Unsupported file type')
+ raise MonzoGeneralError("Unsupported file type")
file_type = SUPPORTED_ATTACHMENT_EXTENSIONS[file_extension]
if file_url.netloc:
file_type = Attachment._upload_file(auth=auth, url=url, file_type=file_type)
</code_context>
<issue_to_address>
**question:** Ambiguity in file upload logic for remote URLs.
Please clarify the purpose of using the return value from _upload_file as file_type, and ensure it represents the correct file URL rather than a MIME type.
</issue_to_address>
### Comment 3
<location> `monzo/endpoints/attachment.py:141-150` </location>
<code_context>
- raise MonzoAuthenticationError('Unable to refresh without a refresh token')
+ logging.debug("Unable to fetch new token without a refresh token")
+ raise MonzoAuthenticationError("Unable to refresh without a refresh token")
data = {
- 'grant_type': 'refresh_token',
- 'client_id': self._client_id,
</code_context>
<issue_to_address>
**issue (bug_risk):** Passing file_url object instead of string in request data.
Since file_url is a ParseResult, passing it directly may not work if a string is required. Use url or file_url.geturl() to ensure compatibility.
</issue_to_address>
### Comment 4
<location> `monzo/endpoints/transaction.py:537-541` </location>
<code_context>
+ data["expand"] = expand[0]
if since:
- data['since'] = format_date(since)
+ if isinstance(since, datetime):
+ data["since"] = format_date(since)
+ elif isinstance(since, str):
+ data["since"] = since
if before:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Accepting both datetime and str for 'since' parameter may lead to inconsistent formatting.
Validate the string format or convert all inputs to the expected date format before sending to the API.
```suggestion
if since:
if isinstance(since, datetime):
data["since"] = format_date(since)
elif isinstance(since, str):
# Validate ISO 8601 format (YYYY-MM-DD or YYYY-MM-DDTHH:MM:SSZ)
import re
iso8601_regex = r"^\d{4}-\d{2}-\d{2}(T\d{2}:\d{2}:\d{2}Z)?$"
if not re.match(iso8601_regex, since):
raise ValueError("The 'since' parameter must be a datetime or a string in ISO 8601 format (YYYY-MM-DD or YYYY-MM-DDTHH:MM:SSZ).")
data["since"] = since
```
</issue_to_address>
### Comment 5
<location> `monzo/endpoints/transaction.py:544-545` </location>
<code_context>
if before:
- data['before'] = format_date(before)
+ data["before"] = format_date(before)
if limit:
- data['limit'] = min(limit, 100)
- path = '/transactions'
+ data["limit"] = min(limit, 100)
+ path = "/transactions"
res = auth.make_request(path=path, data=data)
</code_context>
<issue_to_address>
**nitpick:** Limit is capped at 100, but no warning or error is raised if exceeded.
Please add a warning or update the documentation to clarify that requests over 100 are capped, so users are aware of this behavior.
</issue_to_address>
### Comment 6
<location> `monzo/endpoints/transaction.py:85` </location>
<code_context>
+ self._created: datetime = create_date(transaction_data["created"])
+ self._currency: str = transaction_data["currency"]
+ self._dedupe_id: str = transaction_data["dedupe_id"]
+ self._decline_reason: str = transaction_data.get("decline_reason", "")
+ self._description: str = transaction_data["description"]
+ self._fees: Dict[str, Any] = transaction_data["fees"]
</code_context>
<issue_to_address>
**suggestion:** Default value for decline_reason may mask None values.
Consider using None as the default for decline_reason to clearly differentiate between missing values and an explicit empty reason.
```suggestion
self._decline_reason: Optional[str] = transaction_data.get("decline_reason", None)
```
</issue_to_address>
### Comment 7
<location> `monzo/endpoints/feed_item.py:83-85` </location>
<code_context>
parameters_clean = {}
- for parameter in FEED_ITEM_PARAMS[self._feed_type.lower()]['required']:
+ for parameter in FEED_ITEM_PARAMS[self._feed_type.lower()]["required"]:
if parameter not in self._params:
- raise MonzoArgumentError(f'{parameter} is a required parameter for self._feed_type.lower()')
+ raise MonzoArgumentError(f"{parameter} is a required parameter for self._feed_type.lower()")
parameters_clean[parameter] = self._params[parameter]
</code_context>
<issue_to_address>
**nitpick:** Error message for missing required parameter is unclear.
Instead of referencing 'self._feed_type.lower()', use the actual feed type value in the error message to improve clarity.
</issue_to_address>
### Comment 8
<location> `monzo/endpoints/feed_item.py:126-133` </location>
<code_context>
url: Optional URL for feed item
"""
- feed_item = FeedItem(auth=auth, account_id=account_id, feed_type=feed_type, params=params, url=url)
+ feed_item = FeedItem(
+ auth=auth,
+ account_id=account_id,
+ feed_type=feed_type,
+ params=params,
+ url=url,
+ )
feed_item._create()
return feed_item
</code_context>
<issue_to_address>
**suggestion:** FeedItem is created and then _create is called; consider combining for clarity.
Consider moving the creation logic into the constructor or using a factory method to streamline object instantiation and avoid the need for a separate _create call.
Suggested implementation:
```python
feed_item = FeedItem(
auth=auth,
account_id=account_id,
feed_type=feed_type,
params=params,
url=url,
)
return feed_item
```
You will also need to:
1. Move the logic from the `_create` method into the `FeedItem` constructor, so that creation happens automatically when a `FeedItem` is instantiated.
2. Remove or refactor the `_create` method if it is no longer needed elsewhere.
3. If `_create` returns a value or has important side effects, ensure those are handled in the constructor or consider implementing a `FeedItem.create(...)` factory method instead.
</issue_to_address>
### Comment 9
<location> `monzo/endpoints/receipt.py:91` </location>
<code_context>
+ "tax": self._tax,
+ "unit": self._unit,
}
sub_item_list: List[Any] = [sub_item.as_dict() for sub_item in self._sub_items]
- item['sub_items'] = sub_item_list
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential for _sub_items to be None or not initialized.
Initialize _sub_items as an empty list if it's None to prevent list comprehension errors.
</issue_to_address>
### Comment 10
<location> `monzo/endpoints/receipt.py:365-374` </location>
<code_context>
+ for item in receipt_data["items"]:
</code_context>
<issue_to_address>
**issue:** No check for missing 'sub_items' key in item dict.
Using item.get('sub_items', []) will prevent KeyError if 'sub_items' is missing.
</issue_to_address>
### Comment 11
<location> `monzo/endpoints/account.py:177` </location>
<code_context>
+ res = auth.make_request(path="/accounts", data=data)
account_list = []
- for account_item in res['data']['accounts']:
+ for account_item in res["data"]["accounts"]:
account = Account(
auth=auth,
</code_context>
<issue_to_address>
**issue:** No check for missing 'accounts' key in response data.
Using get("accounts", []) will prevent a KeyError if the key is missing.
</issue_to_address>
### Comment 12
<location> `monzo/endpoints/balance.py:97-102` </location>
<code_context>
- res = auth.make_request(path='/balance', data=data)
+ data = {"account_id": account_id}
+ res = auth.make_request(path="/balance", data=data)
return Balance(
auth=auth,
- balance=res['data']['balance'],
- total_balance=res['data']['total_balance'],
- currency=res['data']['currency'],
- spend_today=res['data']['spend_today'],
+ balance=res["data"]["balance"],
+ total_balance=res["data"]["total_balance"],
+ currency=res["data"]["currency"],
+ spend_today=res["data"]["spend_today"],
)
</code_context>
<issue_to_address>
**issue:** No check for missing keys in balance response data.
Using direct key access may cause a KeyError if any expected fields are missing. Please validate the response or use .get() with default values before creating the Balance object.
</issue_to_address>
### Comment 13
<location> `monzo/endpoints/webhooks.py:121` </location>
<code_context>
+ res = auth.make_request(path="/webhooks", data=data)
webhooks = []
- for webhook_item in res['data']['webhooks']:
+ for webhook_item in res["data"]["webhooks"]:
webhook = Webhook(
auth=auth,
</code_context>
<issue_to_address>
**issue:** No check for missing 'webhooks' key in response data.
Using res["data"].get("webhooks", []) will prevent a KeyError if the key is missing.
</issue_to_address>
### Comment 14
<location> `tests/test_endpoints.py:260-42` </location>
<code_context>
+ def test_multiple_transaction(
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test coverage for transaction pagination using transaction_id.
Please add tests for pagination using transaction_id, including cases with valid, invalid, and missing transaction_id values.
</issue_to_address>
### Comment 15
<location> `tests/test_payload.py:191-51` </location>
<code_context>
+ (WhoAmI, "get", "WhoAmI", "WhoAmI", {}),
+ ],
+ )
def test_fetch_payload(
- self,
- endpoint,
- http_method: str,
- data_filename: str,
- response_filename: str,
- extra_data: Dict[str, str],
- mocker
+ self,
+ endpoint,
+ http_method: str,
+ data_filename: str,
+ response_filename: str,
+ extra_data: Dict[str, str],
+ mocker,
):
"""
</code_context>
<issue_to_address>
**suggestion (testing):** No test for transaction fetch with transaction_id pagination.
Add a test to ensure transaction fetch requests correctly handle the transaction_id pagination parameter, including payload and response validation.
Suggested implementation:
```python
(Webhook, "get", "Webhooks", "WebhooksNone", {"account_id": "acc_123ABC"}),
(Webhook, "get", "Webhooks", "WebhooksOne", {"account_id": "acc_123ABC"}),
# Test for transaction fetch with transaction_id pagination
(Transaction, "get", "TransactionPaginated", "TransactionPaginated", {"account_id": "acc_123ABC", "transaction_id": "txn_456DEF"}),
(WhoAmI, "get", "WhoAmI", "WhoAmI", {}),
```
1. You will need to create the mock data files `TransactionPaginated.json` in both the `mock_requests` and `mock_responses` directories, representing the request payload and expected response for a paginated transaction fetch.
2. Ensure the `Transaction` handler supports the `transaction_id` parameter in its payload construction.
3. If the test function does not already validate the payload and response for pagination, add assertions to check that the `transaction_id` is present in the payload and that the response matches the expected paginated format.
</issue_to_address>
### Comment 16
<location> `docs/source/conf.py:26` </location>
<code_context>
"""
Configuration file for the Sphinx documentation builder.
This file only contains a selection of the most common options. For a full
list see the documentation:
https://www.sphinx-doc.org/en/master/usage/configuration.html
"""
import datetime
import os
import sys
from configparser import ConfigParser
from typing import List
# -- Path setup --------------------------------------------------------------
sys.path.insert(0, os.path.abspath("../"))
config = ConfigParser()
config.read("../../setup.cfg")
# -- Project information -----------------------------------------------------
copyright_year = datetime.datetime.now().year
author = config["metadata"]["author"]
project = config["metadata"]["name"]
copyright = f"{copyright_year}, {author}"
# The full version, including alpha/beta/rc tags
release = config["metadata"]["version"]
# -- General configuration ---------------------------------------------------
# Add any Sphinx extension module names here, as strings. They can be
# extensions coming with Sphinx (named 'sphinx.ext.*') or your custom
# ones.
extensions: List[str] = [
"sphinx.ext.autodoc",
"sphinx.ext.coverage",
"sphinx.ext.doctest",
]
# Add any paths that contain templates here, relative to this directory.
templates_path: List[str] = ["_templates"]
# List of patterns, relative to the source directory, that match files and
# directories to ignore when looking for source files.
# This pattern also affects html_static_path and html_extra_path.
exclude_patterns: List[str] = []
# -- Options for HTML output -------------------------------------------------
# The theme to use for HTML and HTML Help pages. See the documentation for
# a list of builtin themes.
#
html_theme = "sphinx_rtd_theme"
# Add any paths that contain custom static files (such as style sheets) here,
# relative to this directory. They are copied after the builtin static files,
# so a file named "default.css" will overwrite the builtin "default.css".
html_static_path = ["_static"]
</code_context>
<issue_to_address>
**issue (code-quality):** Don't assign to builtin variable `copyright` ([`avoid-builtin-shadow`](https://docs.sourcery.ai/Reference/Default-Rules/comments/avoid-builtin-shadow/))
<br/><details><summary>Explanation</summary>Python has a number of `builtin` variables: functions and constants that
form a part of the language, such as `list`, `getattr`, and `type`
(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:
```python
list = [1, 2, 3]
```
However, this is considered poor practice.
- It will confuse other developers.
- It will confuse syntax highlighters and linters.
- It means you can no longer use that builtin for its original purpose.
How can you solve this?
Rename the variable something more specific, such as `integers`.
In a pinch, `my_list` and similar names are colloquially-recognized
placeholders.</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Owner
Author
|
Fixes #89 |
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- I noticed there aren’t any tests covering the new string-based
sinceparameter on Transaction.fetch—consider adding a test to verify that passing a transaction ID string correctly includes it in the API request. - Tests are casting credential values to str/int in many places; to reduce boilerplate, either have Handler.fetch return correctly typed values or make Authentication accept raw types more gracefully.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- I noticed there aren’t any tests covering the new string-based `since` parameter on Transaction.fetch—consider adding a test to verify that passing a transaction ID string correctly includes it in the API request.
- Tests are casting credential values to str/int in many places; to reduce boilerplate, either have Handler.fetch return correctly typed values or make Authentication accept raw types more gracefully.
## Individual Comments
### Comment 1
<location> `monzo/endpoints/attachment.py:142-145` </location>
<code_context>
file_url = urlparse(url)
_, file_extension = splitext(url)
if file_extension not in SUPPORTED_ATTACHMENT_EXTENSIONS:
- raise MonzoGeneralError('Unsupported file type')
+ raise MonzoGeneralError("Unsupported file type")
file_type = SUPPORTED_ATTACHMENT_EXTENSIONS[file_extension]
if file_url.netloc:
</code_context>
<issue_to_address>
**suggestion:** Consider normalizing file extensions before checking support.
Not normalizing extensions may cause valid files like '.JPG' to be rejected. Normalize extensions before checking support to avoid this issue.
```suggestion
_, file_extension = splitext(url)
normalized_extension = file_extension.lower()
if normalized_extension not in SUPPORTED_ATTACHMENT_EXTENSIONS:
raise MonzoGeneralError("Unsupported file type")
file_type = SUPPORTED_ATTACHMENT_EXTENSIONS[normalized_extension]
```
</issue_to_address>
### Comment 2
<location> `monzo/endpoints/attachment.py:141-149` </location>
<code_context>
- raise MonzoGeneralError('Unsupported file type')
+ raise MonzoGeneralError("Unsupported file type")
file_type = SUPPORTED_ATTACHMENT_EXTENSIONS[file_extension]
if file_url.netloc:
file_type = Attachment._upload_file(auth=auth, url=url, file_type=file_type)
data = {
</code_context>
<issue_to_address>
**suggestion:** Clarify logic for file uploads based on URL type.
Distinguish more clearly between local file paths and remote URLs to prevent incorrect upload behavior.
```suggestion
file_url = urlparse(url)
_, file_extension = splitext(url)
if file_extension not in SUPPORTED_ATTACHMENT_EXTENSIONS:
raise MonzoGeneralError("Unsupported file type")
file_type = SUPPORTED_ATTACHMENT_EXTENSIONS[file_extension]
# Distinguish between local file paths and remote URLs
if not file_url.netloc:
# Local file path: upload the file and get the new URL
file_type = Attachment._upload_file(auth=auth, url=url, file_type=file_type)
else:
# Remote URL: use the URL directly, no upload needed
pass
data = {
```
</issue_to_address>
### Comment 3
<location> `monzo/endpoints/attachment.py:195-196` </location>
<code_context>
+ method="POST",
data=data,
)
# TODO upload file
- return response['data']['file_url']
+ return response["data"]["file_url"]
</code_context>
<issue_to_address>
**issue (bug_risk):** File upload logic is not implemented.
The file upload functionality is missing. Please implement it or raise NotImplementedError if not yet supported.
</issue_to_address>
### Comment 4
<location> `tests/test_endpoints.py:260-42` </location>
<code_context>
+ def test_multiple_transaction(
</code_context>
<issue_to_address>
**suggestion (testing):** No test for error conditions or edge cases in transaction pagination.
Please add tests for invalid and non-existent transaction IDs, as well as for cases where both a datetime and a string are passed to 'since', to improve coverage of error handling.
</issue_to_address>
### Comment 5
<location> `docs/source/conf.py:26` </location>
<code_context>
"""
Configuration file for the Sphinx documentation builder.
This file only contains a selection of the most common options. For a full
list see the documentation:
https://www.sphinx-doc.org/en/master/usage/configuration.html
"""
import datetime
import os
import sys
from configparser import ConfigParser
from typing import List
# -- Path setup --------------------------------------------------------------
sys.path.insert(0, os.path.abspath("../"))
config = ConfigParser()
config.read("../../setup.cfg")
# -- Project information -----------------------------------------------------
copyright_year = datetime.datetime.now().year
author = config["metadata"]["author"]
project = config["metadata"]["name"]
copyright = f"{copyright_year}, {author}"
# The full version, including alpha/beta/rc tags
release = config["metadata"]["version"]
# -- General configuration ---------------------------------------------------
# Add any Sphinx extension module names here, as strings. They can be
# extensions coming with Sphinx (named 'sphinx.ext.*') or your custom
# ones.
extensions: List[str] = [
"sphinx.ext.autodoc",
"sphinx.ext.coverage",
"sphinx.ext.doctest",
]
# Add any paths that contain templates here, relative to this directory.
templates_path: List[str] = ["_templates"]
# List of patterns, relative to the source directory, that match files and
# directories to ignore when looking for source files.
# This pattern also affects html_static_path and html_extra_path.
exclude_patterns: List[str] = []
# -- Options for HTML output -------------------------------------------------
# The theme to use for HTML and HTML Help pages. See the documentation for
# a list of builtin themes.
#
html_theme = "sphinx_rtd_theme"
# Add any paths that contain custom static files (such as style sheets) here,
# relative to this directory. They are copied after the builtin static files,
# so a file named "default.css" will overwrite the builtin "default.css".
html_static_path = ["_static"]
</code_context>
<issue_to_address>
**issue (code-quality):** Don't assign to builtin variable `copyright` ([`avoid-builtin-shadow`](https://docs.sourcery.ai/Reference/Default-Rules/comments/avoid-builtin-shadow/))
<br/><details><summary>Explanation</summary>Python has a number of `builtin` variables: functions and constants that
form a part of the language, such as `list`, `getattr`, and `type`
(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:
```python
list = [1, 2, 3]
```
However, this is considered poor practice.
- It will confuse other developers.
- It will confuse syntax highlighters and linters.
- It means you can no longer use that builtin for its original purpose.
How can you solve this?
Rename the variable something more specific, such as `integers`.
In a pinch, `my_list` and similar names are colloquially-recognized
placeholders.</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
petermcd
deleted the
89-since-can-be-an-object-id-as-well-as-a-datetime
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updated pagination to allow transaction id
Migrated to UV
Summary by Sourcery
Enable pagination by transaction id in transaction.fetch and migrate project to use UV tooling with PEP621 configuration; unify code formatting and CI workflows across the repository.
New Features:
Enhancements:
Build:
CI:
Documentation:
Tests: