Conversation
[Style] Multi-file edits on running `ruff format` [Chore] Ensure all local ci checks pass successfully. [Commands] ➜ hatch run check ➜ hatch run ci [Changes] - Add `mypy` config to silence `import-not-found` errors on tests Signed-off-by: Eugene Mwangi <mwangi.em37@gmail.com>
Tell git not to ignore UV's generated lockfile. [Reason] Ensure immutable snapshot for library dep tree for both development and distribution. [Commands] ➜ uv sync --extra dev --extra test Signed-off-by: Eugene Mwangi <mwangi.em37@gmail.com>
Links to the Mpesa docs resulted in a 404 not found. This was because of `APIS` instead of `apis` being used in the URL [Changes] - Update all broken links with correct URL Signed-off-by: Eugene Mwangi <mwangi.em37@gmail.com>
Enhance module boundaries by moving functionality into service. [Changes] - Update broken documentation link in docstrings - Rm base model `DynamicQRCode` with duplicate service behavior - Move all tests requiring a `dynamic_qr_service` fixture to: tests/unit/services/test_dynamic_qr_service.py Signed-off-by: Eugene Mwangi <mwangi.em37@gmail.com> chore(!): Rebase into 0125945 Commit msg: refactor(qr): Rm service responsibility from `pydantic` models Signed-off-by: Eugene Mwangi <mwangi.em37@gmail.com> chore(!): Rebase into 80d9436 Signed-off-by: Eugene Mwangi <mwangi.em37@gmail.com> chore(!): Rebase into 0488068 Signed-off-by: Eugene Mwangi <mwangi.em37@gmail.com>
[Issue] The `DynamicQRGenerateResponse` represents the JSON response from Daraja The current model misses `RequestID` which is a documented field in the response body [Fix] - Add missing field to get exact parity with API response. - Update all instances failing validation to include field across repo [Note] - The models docstring lists `RequestID`, but absent in the model - Confirm if this is intentional from the maintainer Signed-off-by: Eugene Mwangi <mwangi.em37@gmail.com>
Install and add `dotenv` to uv lockfile. [Commands] ➜ uv add python-dotenv [Note] - Pkg is used in library but not present in project's lockfile. Signed-off-by: Eugene Mwangi <mwangi.em37@gmail.com>
[Issue] Docs document an env var: `MPESA_ENVIRONMENT` to set whether using the sandbox or prod APIs. But tests read `MPESA_ENV` [Fix] Rename all occurrences of `MPESA_ENV` to `MPESA_ENVIRONMENT` Signed-off-by: Eugene Mwangi <mwangi.em37@gmail.com>
Add dedicated directory to store and manage envs. [Note] - Tell git to ignore files that end in .env - Prefer to split env files to avoid common pitfalls. (ie, reading only `sandbox.env` for test or local targets). [Commands] ➜ cp .envs/sandbox.env.example .envs/sandbox.env ➜ cp .envs/production.env.example .envs/production.env Signed-off-by: Eugene Mwangi <mwangi.em37@gmail.com>
Add shared fixtures for integration tests and the service: DynamicQRCode Important: This pattern should be applied to all integration tests [Changes] - Fix wrong type hint assigned to request model - Make clients session fixtures to trigger a single network call for an auth token and reuse for all integration tests. Signed-off-by: Eugene Mwangi <mwangi.em37@gmail.com>
[Problem] QR services receive params only to reassemble internally. [Rationale] Multiple params can make APIs harder to reason and think about. Param object patterns in this context centralize data structure and validation, while making the contract and tests simpler and readable. [Issue] The `**kwargs` arg is a serious bug: It silently drops all args not in the model `DynamicQRGenerateRequest` This gives callers an illusion that we support other request args when internally we actually drop them without warning. [Fix] Drop the `**kwargs` arg and all traces of it across this module. Using a single param object is enough here. Signed-off-by: Eugene Mwangi <mwangi.em37@gmail.com>
Updates to enhance code semantics and design. [Rationale] - Parametrize test to use idiomatic patterns for variable inputs - Rm TrxCode method validation: pydantic can handle validating fields assigned an enum type - Update model configs to: Enforce runtime validation for updates to request DTO Enforce immutable read-only snapshot for response DTO - Run ruff formatter - Add fixtures for common, reused data and centralize fixtures in the `conftest.py` under the unit tests module Signed-off-by: Eugene Mwangi <mwangi.em37@gmail.com>
[Issue] The DynamicQRCode module refers usage of a ResponseCode with a value: 00 [Fix] Daraja 3.0 uses a string value `0` for the synchronous Gateway response Signed-off-by: Eugene Mwangi <mwangi.em37@gmail.com>
RafaelJohn9
requested changes
Sep 12, 2026
Member
There was a problem hiding this comment.
Awesome work!
Requested few changes.
Ran the full check suite locally against this branch (ruff, ruff format, mypy, bandit, pytest). Lint/format/types/security are all clean, and 525 unit tests pass — but 6 tests currently error out, which will fail CI once it runs.
Ensure all hatch env scripts pass successfully and fix related issues. [Command] ➜ hatch run check && hatch run ci [Changes] - Ignore `bandit` false positive with nosec pragma - Avoid using `pytest_plugins` in non top-level `conftest.py` scripts - Fix any issues raised from checks Signed-off-by: Eugene Mwangi <mwangi.em37@gmail.com>
eugengi
force-pushed
the
refactor-service-dynamic-qr
branch
from
September 13, 2026 09:07
2e87b4e to
6a836ac
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
I initially had one question:
I found myself deep-diving into internals and infra plumbing to answer the above question. I noticed auth logic was leaked and duplicated across services and retested across various modules... I chose to fix this first to improve developer experience and reduce friction for the community when contributing and adding new services.
This led me to review at least one module to understand how this library works. That was
Dynamic QR Code. I noted a few gaps and required fixes. This PR consists of patches to improve the module's design and other required updates.Note
This PR is prerequisite work tied to the following issue #164. The next change will be creating an abstraction for a reusable auth contract and finally implementing the
Mobile Number Validationservice.Type of Change
How Has This Been Tested?
Click to expand terminal trace
Warning
The script
hatch run ciwas run but didn't pass. This is for live integration tests with the Daraja sandbox API.Important
I tried to run live integration tests for the
Dynamic QR Codeservice, but I've been getting a503 Service Unavailableerror from the M-Pesa Gateway for a couple of days now.Checklist
Additional Context
ResponseCodeshould be in the following formatAG_20191219_000043fdf61864fe9ff5, though we currently use0. Is there a specific reason for this, or should we update it?Then use
pytest.markersto slice and drive tests:unit,integration,e2e,live, etc.Note
This was after I noted some
Dynamic QR Codetests were hidden intests/unit/test_async_mpesa_client.pyandtests/unit/test_mpesa_client.py. Centralizing all these tests under a single module would make things easier, imo.