Skip to content

SDK-2473: Python - IDV Support Brand ID in session config - python#458

Open
mehmet-yoti wants to merge 1 commit into
masterfrom
websdk-auto/SDK-2473-python-python---idv-support-brand-id-in-session-config
Open

SDK-2473: Python - IDV Support Brand ID in session config - python#458
mehmet-yoti wants to merge 1 commit into
masterfrom
websdk-auto/SDK-2473-python-python---idv-support-brand-id-in-session-config

Conversation

@mehmet-yoti
Copy link
Copy Markdown
Contributor

Summary

Adds support for setting a brand_id on the IDV SdkConfig, enabling consumers to theme the IDV iframe via a brand identifier. This adds a new builder method (with_brand_id), a property accessor, and serialization support that omits the field when unset.

Changes

  • yoti_python_sdk/doc_scan/session/create/sdk_config.py
    • Added optional brand_id parameter to SdkConfig.__init__ (defaults to None).
    • Added brand_id property exposing the value.
    • Included brand_id in to_json() (filtered out by remove_null_values when not set).
    • Added SdkConfigBuilder.with_brand_id(brand_id) fluent setter and threaded the value through build().
  • yoti_python_sdk/tests/doc_scan/session/create/test_sdk_config.py
    • Extended the existing happy-path build test to cover brand_id.
    • Added tests covering: builder default (None), setting via builder, JSON inclusion when set, JSON omission when not set, and JSON omission when explicitly set to None.

QA Test Steps

  1. Check out the branch and install dev dependencies (pip install -e . and any test requirements).
  2. Run the new/updated unit tests:
    pytest yoti_python_sdk/tests/doc_scan/session/create/test_sdk_config.py -v
    
    Verify the following pass:
    • test_should_build_correctly (includes brand_id assertion)
    • test_not_passing_brand_id
    • test_with_brand_id
    • test_brand_id_in_json_when_set
    • test_brand_id_absent_from_json_when_not_set
    • test_brand_id_absent_from_json_when_none
  3. Run the full doc_scan test suite to confirm no regressions:
    pytest yoti_python_sdk/tests/doc_scan -v
    
  4. Happy path — manual smoke test in a Python REPL:
    import json
    from yoti_python_sdk.doc_scan.session.create.sdk_config import SdkConfigBuilder
    from yoti_python_sdk.utils import YotiEncoder
    
    cfg = SdkConfigBuilder().with_brand_id("your-brand-id").build()
    assert cfg.brand_id == "your-brand-id"
    payload = json.loads(json.dumps(cfg, cls=YotiEncoder))
    assert payload["brand_id"] == "your-brand-id"
  5. Edge case — omitted brand_id should not appear in the serialised payload:
    cfg = SdkConfigBuilder().with_allows_camera().build()
    payload = json.loads(json.dumps(cfg, cls=YotiEncoder))
    assert "brand_id" not in payload
  6. Edge case — explicit None should also be omitted from the payload:
    cfg = SdkConfigBuilder().with_brand_id(None).build()
    payload = json.loads(json.dumps(cfg, cls=YotiEncoder))
    assert "brand_id" not in payload
  7. Regression — build an SdkConfig using the existing builder chain (without with_brand_id) and confirm all previously supported fields (allowed_capture_methods, primary_colour, success_url, error_url, privacy_policy_url, allow_handoff, etc.) serialize and read back unchanged.
  8. Integration (optional, if a sandbox is available) — create an IDV session via DocScanClient using an SdkConfig built with a valid brand id and confirm the API accepts the request and the iframe reflects the brand theming.

Notes

  • brand_id is optional and defaults to None; it is stripped from the JSON payload by remove_null_values when unset, preserving the existing wire format for callers that don't use the new field — fully backwards compatible.
  • No validation is performed on the brand_id string (consistent with how other identifier fields like preset_issuing_country are handled); validation is delegated to the IDV backend.
  • The branch also includes an unrelated update to .claude/settings.local.json (local Claude Code tooling settings) — this is not part of the SDK behavior change and can be ignored / dropped before merge if the team prefers not to ship local tool config in the repo.

Related Jira: SDK-2473
Auto-generated by n8n + Claude CLI

@sonarqubecloud
Copy link
Copy Markdown

@mehmet-yoti
Copy link
Copy Markdown
Contributor Author

🤖 Claude Code Review

Code Review Findings

Summary: The change threads a new optional brand_id field through SdkConfig and SdkConfigBuilder so consumers can theme the IDV iframe. Implementation cleanly follows the established builder pattern in sdk_config.py: optional kwarg on __init__, private attribute, read-only property, builder default of None, with_brand_id() setter, and inclusion in to_json() via the existing remove_null_values flow. New tests cover the build/property path and JSON inclusion/omission. Overall this is a small, well-bounded, backwards-compatible additive change. No correctness, security, or design bugs found — only a few minor/nit items below.

Critical

None.

Major

None.

Minor

  • File: .claude/settings.local.json (entire file, new)

    • Issue: A local Claude Code settings file has been added to the repository. This is normally a developer-machine artifact and is typically gitignored (the filename literally ends in .local.json).
    • Why it matters: Committing it bakes one developer's local tool config into the shared branch and will produce noisy diffs over time as other contributors regenerate it.
    • Recommendation: Remove the file from this branch and add .claude/settings.local.json to .gitignore (or remove the entire .claude/ directory from VCS, depending on team preference). Verify nothing else in .claude/ is intentionally tracked first.
  • File: yoti_python_sdk/doc_scan/session/create/sdk_config.py:177 (and to_json more broadly)

    • Issue: The JSON key emitted is "brand_id" (snake_case), which is internally consistent with neighbouring keys (primary_colour, privacy_policy_url, allow_handoff). Worth cross-checking against the IDV backend spec, since a casing mismatch would silently drop the theme rather than fail.
    • Why it matters: A snake_case/camelCase mismatch with the server would silently disable theming; there is no server validation failure path to alert the caller.
    • Recommendation: Confirm the IDV API spec for sdk_config.brand_id. Consider adding an integration/contract test that asserts the full JSON shape of a fully-built SdkConfig including brand_id.
  • File: yoti_python_sdk/doc_scan/session/create/sdk_config.py:14-27

    • Issue: __init__ continues to grow as a long positional parameter list (now 11 parameters, with three optional ones at the end). The builder itself calls it positionally on line 351 — one accidental reorder away from a silent bug. Not introduced by this PR but made slightly worse.
    • Why it matters: Maintainability and risk of subtle defect during future additions. The constructor is "public" (no leading underscore), so external callers can also hit this.
    • Recommendation: Follow-up: make the builder pass keyword arguments in build():
      return SdkConfig(
          allowed_capture_methods=self.__allowed_capture_methods,
          ...
          brand_id=self.__brand_id,
      )

Nit

  • File: yoti_python_sdk/doc_scan/session/create/sdk_config.py:45-50

    • Issue: Docstring order is slightly out-of-sync with the signature order: the signature places allow_handoff before privacy_policy_url (line 24-25), but the docstring lists privacy_policy_url before allow_handoff (line 45-48). The new brand_id entry is correctly placed last but inherits this inconsistency.
    • Recommendation: Reorder the docstring :param: lines to match signature order (allow_handoff then privacy_policy_url then brand_id).
  • File: yoti_python_sdk/tests/doc_scan/session/create/test_sdk_config.py:49

    • Issue: assert result.brand_id is self.SOME_BRAND_ID uses identity (is) rather than equality (==) on a string. Works today via CPython string interning for short literals, but not guaranteed by the language. Matches a pre-existing pattern rather than fixing it.
    • Recommendation: Use == for string comparisons: assert result.brand_id == self.SOME_BRAND_ID. The new test_with_brand_id (line 92) already does this correctly.
  • File: yoti_python_sdk/tests/doc_scan/session/create/test_sdk_config.py:84-110

    • Issue: test_not_passing_brand_id and test_brand_id_absent_from_json_when_not_set exercise nearly identical paths; test_brand_id_absent_from_json_when_none adds little over the previous case (passing None explicitly vs. not setting is the same code path: builder default is None).
    • Recommendation: Optional — consolidate the two "absent from JSON" tests, or rename for clarity.

What's Done Well

  • Fully additive change with an optional kwarg default of None, preserving backwards compatibility for builder and constructor callers.
  • Property/getter, builder method, and docstring are consistent with the existing style (with_allow_handoff, with_privacy_policy_url).
  • to_json() correctly leverages the existing remove_null_values pipeline, so omitted brand_id does not leak null to the API.
  • Test coverage matrix is right for an optional serialized field: default None, explicit set, present in JSON when set, absent when unset, absent when explicitly None.
  • No mutable default arguments introduced.

Overall

Approved with Comments — correct, narrow, backwards-compatible. Address .claude/settings.local.json before merge; remaining items are nits and an optional builder-kwargs follow-up.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds optional brand_id support to the Doc Scan IDV SdkConfig so consumers can theme the IDV iframe via a brand identifier, including builder support and JSON serialization that omits the field when unset.

Changes:

  • Added brand_id to SdkConfig (constructor arg + property) and included it in to_json() via remove_null_values.
  • Added SdkConfigBuilder.with_brand_id(...) and threaded the value through build().
  • Extended/added unit tests to cover builder defaults and JSON inclusion/omission for brand_id.

Reviewed changes

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

File Description
yoti_python_sdk/doc_scan/session/create/sdk_config.py Adds brand_id to the config model, builder, and JSON serialization.
yoti_python_sdk/tests/doc_scan/session/create/test_sdk_config.py Adds tests validating brand_id behavior and serialization.
.claude/settings.local.json Introduces local Claude tooling settings file into the repo.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 45 to +49
assert result.success_url is self.SOME_SUCCESS_URL
assert result.error_url is self.SOME_ERROR_URL
assert result.privacy_policy_url is self.SOME_PRIVACY_POLICY_URL
assert result.allow_handoff is True
assert result.brand_id is self.SOME_BRAND_ID
Comment on lines 46 to +50
:type privacy_policy_url: str
:param allow_handoff: boolean flag for allow_handoff
:type allow_handoff: bool
:param brand_id: the brand id used to theme the IDV iframe
:type brand_id: str
Comment on lines +3 to +8
"allow": [
"Bash(git checkout *)",
"Bash(git pull *)",
"Bash(git ls-tree *)",
"Bash(awk '{print $4}')"
]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants