Skip to content

opensrs adapter fixes#45

Merged
loks0n merged 20 commits intomasterfrom
adapter-fixes-opensrs
Oct 16, 2025
Merged

opensrs adapter fixes#45
loks0n merged 20 commits intomasterfrom
adapter-fixes-opensrs

Conversation

@ChiragAgg5k
Copy link
Member

@ChiragAgg5k ChiragAgg5k commented Oct 15, 2025

Summary by CodeRabbit

  • New Features

    • Registrar: added registration-type constants, public name and price queries, and expanded suggestion inputs/options (query types, limit, filtering, price bounds).
    • Mock registrar: new simulated registrar supporting availability, purchase/transfer, suggestions, pricing, renewals, and test-state utilities.
    • New domain-specific exceptions for clearer error signaling.
  • Bug Fixes

    • Improved error handling and clearer exception mapping in purchase and pricing flows; tightened suggestion price-filter validation.
  • Tests

    • Expanded tests covering mock behaviors, purchase/transfer, pricing, suggestions, renewals, and error cases.

@coderabbitai
Copy link

coderabbitai bot commented Oct 15, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds registrar name and pricing APIs, registration-type constants, and domain-specific exceptions; expands suggest() to accept string|array queries plus optional limit/filterType/priceMax/priceMin and renames premiumPrice* → price*; adds Adapter abstract methods getName() and getPrice(); updates OpenSRS to use new response-code constants and to throw DomainTaken, InvalidContact, PriceNotFound, or base Utopia\Domains\Exception as appropriate; introduces an in-memory Mock registrar with comprehensive behavior and tests (including OpenSRS test updates and a new MockTest).

Changes

Cohort / File(s) Summary
Registrar public surface
src/Domains/Registrar.php
Adds public constants REG_TYPE_NEW/TRANSFER/RENEWAL/TRADE; adds getName(): string; expands suggest() to accept `array
Registrar Adapter
src/Domains/Registrar/Adapter.php
Adds REG_TYPE_* constants; introduces abstract getName(): string and getPrice(...): array; updates suggest() signature to `array
OpenSRS adapter
src/Domains/Registrar/OpenSRS.php
Implements getName() and response-code constants; updates available(), purchase(), transfer(), suggest() and getPrice() to use new constants/signatures; wraps flows in try/catch and throws DomainTaken, InvalidContact, PriceNotFound, or base Utopia\Domains\Exception as appropriate; renames premium price params to priceMin/priceMax and adjusts request/response parsing.
Mock registrar implementation
src/Domains/Registrar/Mock.php
New in-memory Mock adapter implementing Adapter API: availability, purchase, transfer, suggest, getPrice, renew, domain retrieval, state management helpers, premium domain handling, and configurable defaults.
Exceptions
src/Domains/Exception.php
src/Domains/Registrar/Exception/DomainTaken.php
src/Domains/Registrar/Exception/PriceNotFound.php
src/Domains/Registrar/Exception/InvalidContact.php
Adds base Utopia\Domains\Exception and specific exceptions DomainTaken, PriceNotFound, and InvalidContact extending it.
Tests
tests/OpenSRSTest.php
tests/MockTest.php
Updates OpenSRS tests to import new exceptions and Registrar constant, change testPurchase() return type and assertions, add testPurchaseWithInvalidContact(), and expect PriceNotFound; adds comprehensive MockTest covering availability, purchase, transfer, suggest, pricing, renewals, error cases, and state helpers.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Client
  participant Registrar
  participant Adapter as Registrar Adapter
  participant API as External Registrar API

  rect rgb(240,248,255)
  note over Client,Adapter: Purchase flow (error normalization)
  Client->>Registrar: purchase(domain, ...)
  Registrar->>Adapter: purchase(...)
  Adapter->>API: API purchase request
  alt success
    API-->>Adapter: success
    Adapter-->>Registrar: result
    Registrar-->>Client: result
  else domain taken
    API-->>Adapter: error(code: domain taken)
    Adapter-->>Registrar: throw DomainTaken
    Registrar-->>Client: DomainTaken
  else invalid contact
    API-->>Adapter: error(code: invalid contact)
    Adapter-->>Registrar: throw InvalidContact
    Registrar-->>Client: InvalidContact
  else other error
    API-->>Adapter: error(message)
    Adapter-->>Registrar: throw Utopia\Domains\Exception
    Registrar-->>Client: Exception
  end
  end

  rect rgb(245,255,245)
  note over Client,Adapter: Get price (specific error)
  Client->>Registrar: getPrice(domain, period, regType)
  Registrar->>Adapter: getPrice(...)
  Adapter->>API: price request
  alt price found
    API-->>Adapter: price data
    Adapter-->>Registrar: price array
    Registrar-->>Client: price array
  else price not found
    API-->>Adapter: error("get_price_domain ... not supported")
    Adapter-->>Registrar: throw PriceNotFound
    Registrar-->>Client: PriceNotFound
  else other error
    API-->>Adapter: error(message)
    Adapter-->>Registrar: throw Utopia\Domains\Exception
    Registrar-->>Client: Exception
  end
  end

  rect rgb(255,250,240)
  note over Client,Adapter: Suggest with price filters
  Client->>Registrar: suggest(query, tlds, limit, filterType, priceMax, priceMin)
  Registrar->>Adapter: suggest(...same params...)
  Adapter->>API: suggest request (price_min/price_max)
  API-->>Adapter: suggestions
  Adapter-->>Registrar: suggestions[]
  Registrar-->>Client: suggestions[]
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • loks0n

Poem

I hop through code and fetch a name,
I sniff for cost and play the game,
If someone took it, I thump — "Nope!"
If price is missing, I beam with hope,
New mocks and errors tidy the lane. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The title “opensrs adapter fixes” focuses solely on the OpenSRS adapter, but this PR also adds new constants and methods to the core Registrar and Adapter classes, introduces several exception types, updates tests, and implements a Mock adapter. Therefore it does not clearly summarize the overall scope of the changes. Please update the title to reflect the broader scope of the changes, for example: “Enhance registrar interface with constants, exception handling, and adapter updates” or similar.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch adapter-fixes-opensrs

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 981d9bd and a585687.

📒 Files selected for processing (1)
  • src/Domains/Registrar/Mock.php (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Domains/Registrar/Mock.php (6)
src/Domains/Registrar.php (1)
  • Registrar (7-68)
src/Domains/Contact.php (2)
  • Contact (5-44)
  • toArray (24-43)
src/Domains/Exception.php (1)
  • Exception (5-7)
src/Domains/Registrar/Exception/DomainTaken.php (1)
  • DomainTaken (7-9)
src/Domains/Registrar/Exception/InvalidContact.php (1)
  • InvalidContact (7-9)
src/Domains/Registrar/Exception/PriceNotFound.php (1)
  • PriceNotFound (7-9)
🔇 Additional comments (1)
src/Domains/Registrar/Mock.php (1)

303-319: LGTM! Renewal logic is correct.

The renewal correctly fetches the current expiration from the domain info and extends it by the specified number of years.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@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 (4)
src/Domains/Registrar/Adapter.php (1)

45-49: Consider adding optional parameters for registration period and type.

The abstract signature only accepts $domain, but the OpenSRS implementation (line 418 in src/Domains/Registrar/OpenSRS.php) includes additional parameters: int $period = 1, string $regType = 'new'. These are meaningful parameters that other registrars might also support.

Consider updating the abstract signature to:

-    abstract public function getPrice(string $domain): array;
+    abstract public function getPrice(string $domain, int $period = 1, string $regType = 'new'): array;

This would:

  1. Allow callers to specify these commonly needed parameters
  2. Maintain backward compatibility (via defaults)
  3. Provide a consistent interface across implementations
src/Domains/Registrar/OpenSRS.php (2)

241-243: Consider improving null-safe error messaging.

The price range validation correctly enforces priceMin < priceMax, but when either value is null, the interpolation will display empty strings in the error message (e.g., "priceMin () must be less than priceMax ()").

While this scenario should not occur due to the condition check, you could make the error message more robust:

-        if ($priceMin !== null && $priceMax !== null && $priceMin >= $priceMax) {
-            throw new Exception("Invalid price range: priceMin ($priceMin) must be less than priceMax ($priceMax).");
-        }
+        if ($priceMin !== null && $priceMax !== null && $priceMin >= $priceMax) {
+            throw new Exception("Invalid price range: priceMin must be less than priceMax (got $priceMin >= $priceMax).");
+        }

249-251: Validate the logic for price filtering with suggestions.

The validation prevents setting price parameters when filterType is 'suggestion', which is correct since suggestion domains don't have associated prices. However, the error message interpolation could be improved (same null-safety concern as above).

Consider this adjustment for clearer messaging:

         if ($filterType !== null && $filterType === 'suggestion' && ($priceMin !== null || $priceMax !== null)) {
-            throw new Exception("Invalid price range: priceMin ($priceMin) and priceMax ($priceMax) cannot be set when filterType is 'suggestion'.");
+            throw new Exception("Price parameters (priceMin/priceMax) cannot be set when filterType is 'suggestion'.");
         }
src/Domains/Registrar.php (1)

41-44: Align getPrice signature across interface and implementation

  • In src/Domains/Registrar/Adapter.php, change
    abstract public function getPrice(string $domain): array
    to
    abstract public function getPrice(string $domain, int $period = 1, string $regType = 'new'): array.
  • In src/Domains/Registrar.php, update
    public function getPrice(string $domain): array
    to
    public function getPrice(string $domain, int $period = 1, string $regType = 'new'): array
    and forward both parameters to the adapter.
  • Adjust docblocks for both methods to document $period and $regType.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between caa294d and daef485.

📒 Files selected for processing (3)
  • src/Domains/Registrar.php (2 hunks)
  • src/Domains/Registrar/Adapter.php (2 hunks)
  • src/Domains/Registrar/OpenSRS.php (2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ChiragAgg5k
PR: utopia-php/domains#44
File: src/Domains/Registrar/OpenSRS.php:292-438
Timestamp: 2025-10-05T13:46:00.588Z
Learning: In the OpenSRS `suggest()` method, non-premium (suggestion) domains should be prioritized and shown first. When a limit is applied, the limit should be filled with suggestion domains first, and premium domains should only be included if there's remaining capacity after processing suggestions.
📚 Learning: 2025-10-05T13:46:00.588Z
Learnt from: ChiragAgg5k
PR: utopia-php/domains#44
File: src/Domains/Registrar/OpenSRS.php:292-438
Timestamp: 2025-10-05T13:46:00.588Z
Learning: In the OpenSRS `suggest()` method, non-premium (suggestion) domains should be prioritized and shown first. When a limit is applied, the limit should be filled with suggestion domains first, and premium domains should only be included if there's remaining capacity after processing suggestions.

Applied to files:

  • src/Domains/Registrar/OpenSRS.php
🧬 Code graph analysis (3)
src/Domains/Registrar/Adapter.php (2)
src/Domains/Registrar.php (3)
  • suggest (26-29)
  • tlds (31-34)
  • getPrice (41-44)
src/Domains/Registrar/OpenSRS.php (3)
  • suggest (239-408)
  • tlds (452-456)
  • getPrice (418-450)
src/Domains/Registrar.php (2)
src/Domains/Registrar/Adapter.php (3)
  • suggest (32-32)
  • tlds (37-37)
  • getPrice (49-49)
src/Domains/Registrar/OpenSRS.php (3)
  • suggest (239-408)
  • tlds (452-456)
  • getPrice (418-450)
src/Domains/Registrar/OpenSRS.php (2)
src/Domains/Registrar.php (2)
  • suggest (26-29)
  • tlds (31-34)
src/Domains/Registrar/Adapter.php (2)
  • suggest (32-32)
  • tlds (37-37)
🔇 Additional comments (5)
src/Domains/Registrar.php (1)

26-29: LGTM!

The updated suggest method signature correctly accepts flexible query types and optional filtering/pricing parameters, properly delegating all arguments to the adapter.

src/Domains/Registrar/Adapter.php (1)

28-32: LGTM!

The parameter renaming from premiumPriceMax/premiumPriceMin to priceMax/priceMin aligns with the implementation changes and provides a cleaner, more general API for price filtering.

src/Domains/Registrar/OpenSRS.php (3)

235-239: LGTM!

The method signature and docblock updates correctly reflect the parameter renaming from premiumPriceMax/premiumPriceMin to the more general priceMax/priceMin, aligning with the abstract interface.


283-288: LGTM!

The parameter assignments correctly map priceMin and priceMax to the OpenSRS API's price_min and price_max fields within the premium service override.


418-449: Signature extends abstract interface with optional parameters.

The getPrice implementation includes additional optional parameters ($period and $regType) beyond what's defined in the abstract Adapter::getPrice(string $domain): array. While PHP permits this (the extra parameters have defaults), it means callers using the Registrar or Adapter interface cannot access this functionality.

This is acceptable if these parameters are OpenSRS-specific. However, if period and regType are common across registrars, consider promoting them to the abstract interface (as noted in the Adapter.php review).

The implementation itself correctly:

  • Constructs the OpenSRS GET_PRICE request
  • Parses the XML response for price, premium status, and premium group
  • Returns a well-structured array

Based on learnings

Copy link

@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

♻️ Duplicate comments (2)
src/Domains/Registrar.php (1)

46-49: Delegation is correct but signature needs updating.

The delegation correctly calls adapter->getPrice($domain), but as noted in the Adapter.php review, the interface signature should be updated to include the period and regType parameters that the OpenSRS implementation supports.

src/Domains/Registrar/OpenSRS.php (1)

426-426: Signature mismatch with Adapter interface.

The getPrice() method signature includes optional parameters int $period = 1 and string $regType = 'new' that are not defined in the Adapter interface (src/Domains/Registrar/Adapter.php line 54). Update this signature after the interface is corrected as suggested in the earlier review comment.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between daef485 and a8aec91.

📒 Files selected for processing (3)
  • src/Domains/Registrar.php (3 hunks)
  • src/Domains/Registrar/Adapter.php (3 hunks)
  • src/Domains/Registrar/OpenSRS.php (3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-05T13:46:00.588Z
Learnt from: ChiragAgg5k
PR: utopia-php/domains#44
File: src/Domains/Registrar/OpenSRS.php:292-438
Timestamp: 2025-10-05T13:46:00.588Z
Learning: In the OpenSRS `suggest()` method, non-premium (suggestion) domains should be prioritized and shown first. When a limit is applied, the limit should be filled with suggestion domains first, and premium domains should only be included if there's remaining capacity after processing suggestions.

Applied to files:

  • src/Domains/Registrar/OpenSRS.php
🧬 Code graph analysis (3)
src/Domains/Registrar.php (2)
src/Domains/Registrar/OpenSRS.php (4)
  • getName (17-20)
  • suggest (247-416)
  • tlds (460-464)
  • getPrice (426-458)
src/Domains/Registrar/Adapter.php (4)
  • getName (12-12)
  • suggest (37-37)
  • tlds (42-42)
  • getPrice (54-54)
src/Domains/Registrar/OpenSRS.php (2)
src/Domains/Registrar.php (3)
  • getName (16-19)
  • suggest (31-34)
  • tlds (36-39)
src/Domains/Registrar/Adapter.php (3)
  • getName (12-12)
  • suggest (37-37)
  • tlds (42-42)
src/Domains/Registrar/Adapter.php (2)
src/Domains/Registrar/OpenSRS.php (4)
  • getName (17-20)
  • suggest (247-416)
  • tlds (460-464)
  • getPrice (426-458)
src/Domains/Registrar.php (4)
  • getName (16-19)
  • suggest (31-34)
  • tlds (36-39)
  • getPrice (46-49)
🔇 Additional comments (6)
src/Domains/Registrar/Adapter.php (2)

9-12: LGTM!

The addition of getName() provides a clean way to identify which registrar adapter is in use.


33-37: LGTM!

The parameter renaming from premiumPriceMax/premiumPriceMin to priceMax/priceMin is applied consistently across the signature and docblock, and aligns with the broader naming changes in OpenSRS and Registrar implementations.

src/Domains/Registrar.php (2)

16-19: LGTM!

The delegation to adapter->getName() is correct and follows the established pattern in this facade class.


31-33: LGTM!

The updated suggest() signature correctly accepts the new parameters (array|string $query, limit, filterType, priceMax, priceMin) and properly delegates them to the adapter.

src/Domains/Registrar/OpenSRS.php (2)

14-20: LGTM!

The getName() implementation correctly returns the adapter identifier 'opensrs'.


243-296: LGTM!

The parameter renaming from premiumPriceMin/premiumPriceMax to priceMin/priceMax is consistently applied throughout:

  • Docblock (lines 243-244)
  • Method signature (line 247)
  • Validation logic (lines 249-250, 257-258)
  • Service override configuration (lines 291-296)

The validation logic correctly enforces that priceMin < priceMax and that price filters are only used with premium domains, not suggestion domains.

Based on learnings.

Copy link

@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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/Domains/Registrar/OpenSRS.php (1)

419-427: Docblock vs return value mismatch for price

Code can return 'price' => null, but doc says float. Either:

  • Guarantee non-null price (throw DomainException if missing), or
  • Update doc to float|null.
🧹 Nitpick comments (4)
tests/OpenSRSTest.php (1)

192-195: Make exception message assertion resilient

Upstream error text can vary. Prefer regex or assert only class to avoid flaky tests.

Apply this diff:

-        $this->expectException(DomainException::class);
-        $this->expectExceptionMessage("Failed to get price for domain: get_price_domain API is not supported for 'invalid domain'");
+        $this->expectException(DomainException::class);
+        $this->expectExceptionMessageMatches('/Failed to get price for domain: .*get_price_domain API is not supported/i');
src/Domains/Registrar/OpenSRS.php (3)

244-249: Updated suggest() signature — document price units

Params are int|null but prices from APIs can be decimal and currency-specific. Clarify units/currency in the docblock (e.g., “account currency, whole units vs cents”), or consider float for bounds.


354-416: Avoid overriding suggestion entries with premium duplicates

If a domain appears in both groups, keep the suggestion entry to preserve type and ordering.

Apply this diff:

-                if ($domain) {
+                if ($domain) {
+                    // Preserve suggestion-first entries if already added
+                    if (isset($items[$domain])) {
+                        continue;
+                    }
                     $items[$domain] = [
                         'available' => $available,
                         'price' => $price,
                         'type' => 'premium'
                     ];

430-463: Wrap all failures, not only Exceptions

sanitizeResponse or XML parsing can raise Error/TypeError. Catch Throwable to consistently surface DomainException.

Apply this diff:

-        } catch (Exception $e) {
+        } catch (\Throwable $e) {
             throw new DomainException('Failed to get price for domain: ' . $e->getMessage(), $e->getCode(), $e);
         }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a8aec91 and 8e8f081.

📒 Files selected for processing (3)
  • src/Domains/Exception/DomainException.php (1 hunks)
  • src/Domains/Registrar/OpenSRS.php (4 hunks)
  • tests/OpenSRSTest.php (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-05T13:46:00.588Z
Learnt from: ChiragAgg5k
PR: utopia-php/domains#44
File: src/Domains/Registrar/OpenSRS.php:292-438
Timestamp: 2025-10-05T13:46:00.588Z
Learning: In the OpenSRS `suggest()` method, non-premium (suggestion) domains should be prioritized and shown first. When a limit is applied, the limit should be filled with suggestion domains first, and premium domains should only be included if there's remaining capacity after processing suggestions.

Applied to files:

  • src/Domains/Registrar/OpenSRS.php
🧬 Code graph analysis (2)
tests/OpenSRSTest.php (3)
src/Domains/Exception/DomainException.php (1)
  • DomainException (7-9)
src/Domains/Registrar/OpenSRS.php (1)
  • getPrice (428-464)
src/Domains/Registrar/Adapter.php (1)
  • getPrice (54-54)
src/Domains/Registrar/OpenSRS.php (3)
src/Domains/Exception/DomainException.php (1)
  • DomainException (7-9)
src/Domains/Registrar/Adapter.php (5)
  • Adapter (7-70)
  • getName (12-12)
  • suggest (37-37)
  • tlds (42-42)
  • getPrice (54-54)
src/Domains/Registrar.php (4)
  • getName (16-19)
  • suggest (31-34)
  • tlds (36-39)
  • getPrice (46-49)
🔇 Additional comments (7)
src/Domains/Exception/DomainException.php (1)

7-9: DomainException addition — LGTM

Clean, namespaced, and ready for use across adapters.

tests/OpenSRSTest.php (1)

7-7: Correct exception import — LGTM

src/Domains/Registrar/OpenSRS.php (5)

7-7: Importing DomainException — LGTM


15-22: getName() added — LGTM

Matches Adapter contract and Registrar facade.


250-260: Input validation improvements — LGTM

Clear range checks and filter constraints.


292-297: Premium price filters wired correctly — LGTM

Only applied to premium service override; suggestion path guarded by validation.


320-352: Suggestions prioritized before premium — good alignment with product intent

Limit is filled by suggestions first; premium considered only if space remains. Based on learnings.

Copy link

@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 (1)
src/Domains/Registrar/OpenSRS.php (1)

438-439: Document the new exception in the docblock.

getPrice() now throws PriceNotFound, so the docblock should list it alongside DomainsException for accurate documentation.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8e8f081 and b06ceb5.

📒 Files selected for processing (5)
  • src/Domains/Exception.php (1 hunks)
  • src/Domains/Registrar/Exception/DomainTaken.php (1 hunks)
  • src/Domains/Registrar/Exception/PriceNotFound.php (1 hunks)
  • src/Domains/Registrar/OpenSRS.php (5 hunks)
  • tests/OpenSRSTest.php (4 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/Domains/Exception.php
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/OpenSRSTest.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-05T13:46:00.588Z
Learnt from: ChiragAgg5k
PR: utopia-php/domains#44
File: src/Domains/Registrar/OpenSRS.php:292-438
Timestamp: 2025-10-05T13:46:00.588Z
Learning: In the OpenSRS `suggest()` method, non-premium (suggestion) domains should be prioritized and shown first. When a limit is applied, the limit should be filled with suggestion domains first, and premium domains should only be included if there's remaining capacity after processing suggestions.

Applied to files:

  • src/Domains/Registrar/OpenSRS.php
🧬 Code graph analysis (3)
src/Domains/Registrar/Exception/PriceNotFound.php (2)
src/Domains/Registrar.php (1)
  • Registrar (7-60)
src/Domains/Exception.php (1)
  • Exception (5-7)
src/Domains/Registrar/Exception/DomainTaken.php (1)
src/Domains/Exception.php (1)
  • Exception (5-7)
src/Domains/Registrar/OpenSRS.php (5)
src/Domains/Exception.php (1)
  • Exception (5-7)
src/Domains/Registrar.php (5)
  • Registrar (7-60)
  • getName (16-19)
  • suggest (31-34)
  • tlds (36-39)
  • getPrice (46-49)
src/Domains/Registrar/Exception/DomainTaken.php (1)
  • DomainTaken (7-9)
src/Domains/Registrar/Exception/PriceNotFound.php (1)
  • PriceNotFound (7-9)
src/Domains/Registrar/Adapter.php (5)
  • Adapter (7-70)
  • getName (12-12)
  • suggest (37-37)
  • tlds (42-42)
  • getPrice (54-54)

Copy link

@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

♻️ Duplicate comments (1)
src/Domains/Registrar/Adapter.php (1)

50-56: Registrar delegation still needs updating to forward optional parameters.

While the interface now correctly declares the optional $period and $regType parameters, the Registrar class delegation at src/Domains/Registrar.php (lines 45-48) still only accepts $domain and doesn't forward these parameters:

public function getPrice(string $domain): array
{
    return $this->adapter->getPrice($domain);
}

This means consumers using the Registrar class cannot access the enhanced functionality.

Update the delegation to match:

-public function getPrice(string $domain): array
+public function getPrice(string $domain, int $period = 1, string $regType = 'new'): array
 {
-    return $this->adapter->getPrice($domain);
+    return $this->adapter->getPrice($domain, $period, $regType);
 }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b06ceb5 and 26e9126.

📒 Files selected for processing (2)
  • src/Domains/Registrar/Adapter.php (3 hunks)
  • src/Domains/Registrar/OpenSRS.php (5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-05T13:46:00.588Z
Learnt from: ChiragAgg5k
PR: utopia-php/domains#44
File: src/Domains/Registrar/OpenSRS.php:292-438
Timestamp: 2025-10-05T13:46:00.588Z
Learning: In the OpenSRS `suggest()` method, non-premium (suggestion) domains should be prioritized and shown first. When a limit is applied, the limit should be filled with suggestion domains first, and premium domains should only be included if there's remaining capacity after processing suggestions.

Applied to files:

  • src/Domains/Registrar/OpenSRS.php
🧬 Code graph analysis (2)
src/Domains/Registrar/OpenSRS.php (5)
src/Domains/Exception.php (1)
  • Exception (5-7)
src/Domains/Registrar.php (5)
  • Registrar (7-60)
  • getName (16-19)
  • suggest (31-34)
  • tlds (36-39)
  • getPrice (46-49)
src/Domains/Registrar/Exception/DomainTaken.php (1)
  • DomainTaken (7-9)
src/Domains/Registrar/Exception/PriceNotFound.php (1)
  • PriceNotFound (7-9)
src/Domains/Registrar/Adapter.php (5)
  • Adapter (7-72)
  • getName (12-12)
  • suggest (37-37)
  • tlds (42-42)
  • getPrice (56-56)
src/Domains/Registrar/Adapter.php (2)
src/Domains/Registrar/OpenSRS.php (4)
  • getName (20-23)
  • suggest (260-429)
  • tlds (486-490)
  • getPrice (440-484)
src/Domains/Registrar.php (4)
  • getName (16-19)
  • suggest (31-34)
  • tlds (36-39)
  • getPrice (46-49)
🔇 Additional comments (8)
src/Domains/Registrar/Adapter.php (2)

9-12: LGTM!

The addition of getName() provides a clear way for registrar implementations to identify themselves.


33-37: LGTM!

The parameter rename from premiumPriceMax/premiumPriceMin to priceMax/priceMin is applied consistently across the interface, implementation, and delegation layers.

src/Domains/Registrar/OpenSRS.php (6)

7-9: LGTM!

The exception imports are necessary for the enhanced error handling in purchase() and getPrice().


17-23: LGTM!

The implementation correctly returns the registrar identifier.


176-201: LGTM!

The error handling correctly distinguishes between domain-taken errors and general exceptions while preserving the exception chain.


256-272: LGTM!

The parameter rename from premiumPriceMax/premiumPriceMin to priceMax/priceMin is applied consistently in the signature, docblock, and validation logic.


304-309: LGTM!

The service override configuration correctly uses the renamed priceMin and priceMax parameters.


442-472: LGTM!

The price retrieval implementation correctly constructs the API request, parses the XML response using XPath, and returns a well-structured result with price details and premium status.

Copy link

@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: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 26e9126 and 0e9b43f.

📒 Files selected for processing (1)
  • src/Domains/Registrar/OpenSRS.php (5 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-05T13:46:00.588Z
Learnt from: ChiragAgg5k
PR: utopia-php/domains#44
File: src/Domains/Registrar/OpenSRS.php:292-438
Timestamp: 2025-10-05T13:46:00.588Z
Learning: In the OpenSRS `suggest()` method, non-premium (suggestion) domains should be prioritized and shown first. When a limit is applied, the limit should be filled with suggestion domains first, and premium domains should only be included if there's remaining capacity after processing suggestions.

Applied to files:

  • src/Domains/Registrar/OpenSRS.php
🧬 Code graph analysis (1)
src/Domains/Registrar/OpenSRS.php (5)
src/Domains/Exception.php (1)
  • Exception (5-7)
src/Domains/Registrar.php (5)
  • Registrar (7-60)
  • getName (16-19)
  • suggest (31-34)
  • tlds (36-39)
  • getPrice (46-49)
src/Domains/Registrar/Exception/DomainTaken.php (1)
  • DomainTaken (7-9)
src/Domains/Registrar/Exception/PriceNotFound.php (1)
  • PriceNotFound (7-9)
src/Domains/Registrar/Adapter.php (5)
  • Adapter (7-72)
  • getName (12-12)
  • suggest (37-37)
  • tlds (42-42)
  • getPrice (56-56)
🪛 GitHub Actions: Tests
src/Domains/Registrar/OpenSRS.php

[error] 106-106: Failed to get price for domain: get_price_domain API is not supported for 'invalid domain' (OpenSRS price retrieval).


[error] 454-454: Error context in OpenSRS price retrieval flow (line 454) during price lookup for domain.

🔇 Additional comments (5)
src/Domains/Registrar/OpenSRS.php (5)

7-9: LGTM!

The exception imports are appropriate for the new error handling in purchase() and getPrice().


17-23: LGTM!

The getName() method correctly returns the registrar identifier and aligns with the Adapter interface requirement.


256-272: LGTM!

The parameter renaming aligns with the Adapter interface, and both validations are correct:

  • Ensuring priceMin < priceMax prevents invalid ranges
  • Rejecting price parameters for filterType='suggestion' is appropriate since suggestion domains don't have pricing

304-309: LGTM!

The service override correctly uses the renamed priceMin/priceMax parameters, maintaining consistency with the signature change.


176-201: Prefer matching on OpenSRS response code for domain-taken errors
Detect the “domain already registered” condition by matching $e->getCode() against the OpenSRS response_code instead of stripos on the message. Confirm the exact numeric code from the OpenSRS API docs and update the catch block accordingly.

Copy link

@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

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5cea8cf and 924da1a.

📒 Files selected for processing (3)
  • src/Domains/Registrar.php (3 hunks)
  • src/Domains/Registrar/OpenSRS.php (7 hunks)
  • tests/OpenSRSTest.php (4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/Domains/Registrar/OpenSRS.php (5)
src/Domains/Exception.php (1)
  • Exception (5-7)
src/Domains/Registrar.php (6)
  • Registrar (7-60)
  • getName (16-19)
  • __construct (11-14)
  • suggest (31-34)
  • tlds (36-39)
  • getPrice (46-49)
src/Domains/Registrar/Exception/DomainTaken.php (1)
  • DomainTaken (7-9)
src/Domains/Registrar/Exception/PriceNotFound.php (1)
  • PriceNotFound (7-9)
src/Domains/Registrar/Adapter.php (5)
  • Adapter (7-72)
  • getName (12-12)
  • suggest (37-37)
  • tlds (42-42)
  • getPrice (56-56)
src/Domains/Registrar.php (2)
src/Domains/Registrar/OpenSRS.php (4)
  • getName (30-33)
  • suggest (275-444)
  • tlds (507-511)
  • getPrice (456-505)
src/Domains/Registrar/Adapter.php (4)
  • getName (12-12)
  • suggest (37-37)
  • tlds (42-42)
  • getPrice (56-56)
tests/OpenSRSTest.php (4)
src/Domains/Registrar/Exception/DomainTaken.php (1)
  • DomainTaken (7-9)
src/Domains/Registrar/Exception/PriceNotFound.php (1)
  • PriceNotFound (7-9)
src/Domains/Registrar/OpenSRS.php (3)
  • purchase (189-217)
  • getPrice (456-505)
  • OpenSRS (11-927)
src/Domains/Registrar/Adapter.php (2)
  • purchase (26-26)
  • getPrice (56-56)
🔇 Additional comments (14)
src/Domains/Registrar.php (3)

16-19: LGTM!

The getName() delegation is correctly implemented and matches the adapter interface.


31-34: LGTM!

The updated suggest() signature correctly passes through all parameters including the renamed priceMax and priceMin to the adapter.


46-49: LGTM!

The getPrice() delegation is correctly implemented with proper parameter forwarding.

src/Domains/Registrar/OpenSRS.php (7)

7-9: LGTM!

The new exception imports align with the updated error handling strategy in purchase() and getPrice().


17-33: LGTM!

The new domainPriceMarkup and domainPriceCap properties are well-documented, and the getName() implementation is straightforward.


48-70: LGTM!

The constructor correctly initializes the new markup and cap properties with sensible defaults (0.0 = no markup/cap).


275-287: LGTM!

The parameter validation logic is correct:

  • Price range validation ensures priceMin < priceMax
  • Prevents price filters with 'suggestion' filterType (since suggestions don't have pricing)
  • Error messages clearly identify the constraint violations

319-324: LGTM!

The parameter renaming from premiumPriceMax/premiumPriceMin to priceMax/priceMin is correctly applied to the service_override fields.


456-505: LGTM! Exception handling improved per past reviews.

The getPrice() implementation correctly:

  • Calculates markup with cap enforcement: min($markup, $domainPriceCap) when cap > 0
  • Returns comprehensive pricing data including base_price and markup
  • Throws PriceNotFound for "is not supported" errors (more specific than previous broad checks)
  • Preserves exception context when rethrowing

The exception detection is now properly scoped per past review feedback.


191-216: Confirm and handle all "domain taken" API error messages. stripos($e->getMessage(), 'Domain taken') may miss variants like "Domain is taken" or "Domain already taken". Verify the exact OpenSRS API error wording and consider using a case-insensitive regex (e.g. /domain (?:is |already )?taken/i) in src/Domains/Registrar/OpenSRS.php around line 211.

tests/OpenSRSTest.php (4)

7-8: LGTM!

The exception imports enable testing of the new exception handling behavior in purchase() and getPrice().


45-55: LGTM!

The updated test correctly:

  • Changed return type from string to void (more appropriate for a test)
  • Verifies successful purchase for a random domain
  • Tests DomainTaken exception when attempting to purchase an already-taken domain (google.com)

188-207: LGTM!

The updated testGetPrice() correctly:

  • Verifies the new base_price and markup fields in the response
  • Confirms no markup is applied by default (markup = 0, price = base_price)
  • Tests PriceNotFound exception for invalid domains

209-315: LGTM!

The new markup test methods provide comprehensive coverage:

  • testGetPriceWithMarkup(): Verifies markup with cap enforcement
  • testGetPriceMarkupCalculation(): Tests uncapped markup calculation
  • testGetPriceMarkupCap(): Confirms cap is enforced when markup exceeds it

The test logic correctly validates the markup calculation: min(basePrice * rate, cap).

Copy link

@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 (5)
tests/OpenSRSTest.php (5)

197-202: Consider using assertEqualsWithDelta for float comparisons.

While the current assertEquals calls should work for zero markup (where markup = 0.0 and price = base_price), using assertEqualsWithDelta is more robust for floating-point comparisons and protects against precision issues.

Apply this diff:

-        $this->assertEquals(0.0, $markup);
-        $this->assertEquals($basePrice, $price);
+        $this->assertEqualsWithDelta(0.0, $markup, 0.01);
+        $this->assertEqualsWithDelta($basePrice, $price, 0.01);

204-206: Exception message assertion is tightly coupled to API error text.

The expected exception message includes API-specific error text ("get_price_domain API is not supported for 'invalid domain'"). While this correctly tests the current behavior, consider whether asserting only the domain-specific error prefix (e.g., "Failed to get price for domain:") would make the test more resilient to upstream API message changes.


209-245: Consider using assertEqualsWithDelta for calculated float comparisons.

The test correctly validates markup calculation with cap enforcement. However, lines 241 and 243 use assertEquals for calculated float values. While these values are rounded and should match exactly, using assertEqualsWithDelta is more robust for floating-point comparisons.

Apply this diff:

-        $this->assertEquals($expectedMarkup, $markup);
+        $this->assertEqualsWithDelta($expectedMarkup, $markup, 0.01);
         $expectedPrice = round($basePrice + $expectedMarkup, 2);
-        $this->assertEquals($expectedPrice, $price);
+        $this->assertEqualsWithDelta($expectedPrice, $price, 0.01);

247-280: Good use of assertEqualsWithDelta, but consider consistency.

Line 277 correctly uses assertEqualsWithDelta for float comparison. For consistency and robustness, consider using the same approach at line 279.

Apply this diff:

         $this->assertEqualsWithDelta($basePrice * 0.20, $markup, 0.01);
         $expectedPrice = round($basePrice + $markup, 2);
-        $this->assertEquals($expectedPrice, $result['price']);
+        $this->assertEqualsWithDelta($expectedPrice, $result['price'], 0.01);

282-315: Comprehensive cap enforcement test with minor refinement opportunity.

The test thoroughly validates that the markup cap is enforced. The conditional logic correctly handles both scenarios (capped vs. uncapped).

For consistency with the rest of the test suite, consider using assertEqualsWithDelta at line 308:

         if ($basePrice * 0.50 > 2.0) {
-            $this->assertEquals(2.0, $markup, "Markup should be capped at 2.0");
+            $this->assertEqualsWithDelta(2.0, $markup, 0.01, "Markup should be capped at 2.0");
         } else {
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 924da1a and 1852a40.

📒 Files selected for processing (1)
  • tests/OpenSRSTest.php (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/OpenSRSTest.php (4)
src/Domains/Registrar/Exception/DomainTaken.php (1)
  • DomainTaken (7-9)
src/Domains/Registrar/Exception/PriceNotFound.php (1)
  • PriceNotFound (7-9)
src/Domains/Registrar/Adapter.php (2)
  • purchase (26-26)
  • getPrice (56-56)
src/Domains/Registrar/OpenSRS.php (3)
  • purchase (189-217)
  • getPrice (456-505)
  • OpenSRS (11-927)
🔇 Additional comments (2)
tests/OpenSRSTest.php (2)

7-8: LGTM!

The new exception imports are correctly added to support testing exception handling in the purchase and price retrieval flows.


45-55: LGTM!

The enhanced test correctly validates both successful purchase and DomainTaken exception handling. Using 'google.com' as a taken domain is a pragmatic approach for testing the exception path.

Copy link

@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

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1852a40 and 0312020.

📒 Files selected for processing (2)
  • src/Domains/Registrar/OpenSRS.php (6 hunks)
  • tests/OpenSRSTest.php (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/OpenSRSTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Domains/Registrar/OpenSRS.php (5)
src/Domains/Exception.php (1)
  • Exception (5-7)
src/Domains/Registrar.php (5)
  • Registrar (7-60)
  • getName (16-19)
  • suggest (31-34)
  • tlds (36-39)
  • getPrice (46-49)
src/Domains/Registrar/Exception/DomainTaken.php (1)
  • DomainTaken (7-9)
src/Domains/Registrar/Exception/PriceNotFound.php (1)
  • PriceNotFound (7-9)
src/Domains/Registrar/Adapter.php (5)
  • Adapter (7-72)
  • getName (12-12)
  • suggest (37-37)
  • tlds (42-42)
  • getPrice (56-56)
🔇 Additional comments (7)
src/Domains/Registrar/OpenSRS.php (7)

23-29: LGTM!

The getName() method implementation is correct and matches the abstract interface requirement from Adapter.


99-99: LGTM!

Good refactor replacing the magic number with the named constant RESPONSE_CODE_DOMAIN_AVAILABLE.


309-314: LGTM!

The service override mappings are correctly updated to use price_min and price_max, maintaining consistency with the parameter name changes.


443-444: LGTM!

The docblock correctly documents all exceptions thrown by the method, addressing the past review comment about the missing @throws PriceNotFound annotation.


448-486: Improved exception handling pattern.

The exception normalization is well-implemented:

  • Uses error codes instead of substring matching (addressing past review concerns about overly broad detection)
  • Preserves context by including the original message and exception chain
  • Follows consistent pattern with purchase() method

Note: The verification of error code 400 (RESPONSE_CODE_DOMAIN_PRICE_NOT_FOUND) was already requested in an earlier comment on lines 13-18.


182-206: Approve exception handling in purchase()
Error code 485 is confirmed for “domain taken,” and exception chaining with DomainTaken and DomainsException is correctly implemented.


13-18: Verify price-not-found error code

Unable to locate a dedicated “price not found” response code in the OpenSRS API docs. Confirm that 400 is correct for price-not-found scenarios or update RESPONSE_CODE_DOMAIN_PRICE_NOT_FOUND to the actual code.

@ChiragAgg5k ChiragAgg5k requested a review from Copilot October 15, 2025 09:57
Copy link

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

This PR implements comprehensive fixes and improvements for the OpenSRS adapter, focusing on better error handling, API design consistency, and enhanced functionality for domain operations.

  • Adds specific exception handling for domain purchase failures and price lookup errors
  • Enhances the suggest method with improved parameter naming and validation
  • Implements the getName() method and getPrice() functionality across the adapter architecture

Reviewed Changes

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

Show a summary per file
File Description
src/Domains/Registrar/OpenSRS.php Core adapter implementation with enhanced error handling, API constants, and improved suggest method parameters
src/Domains/Registrar/Adapter.php Abstract base class updates with new getName() and getPrice() method signatures and parameter renaming
src/Domains/Registrar.php Wrapper class with new getName() and getPrice() methods plus enhanced suggest method signature
src/Domains/Exception.php New base exception class for the domains package
src/Domains/Registrar/Exception/DomainTaken.php Specific exception for domain purchase failures when domain is already taken
src/Domains/Registrar/Exception/PriceNotFound.php Specific exception for price lookup failures
tests/OpenSRSTest.php Updated test cases to verify new exception handling and method behavior

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

@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: 2

🧹 Nitpick comments (6)
src/Domains/Registrar/Mock.php (4)

1-2: Enable strict types

Add strict_types for safer type checks.

 <?php
+
+declare(strict_types=1);

321-326: Docblock type mismatch and PHPMD suppression for unused $contacts (transfer)

Mirror the purchase docblock fix here.

     /**
      * Transfer a domain
      *
      * @param string $domain
-     * @param array<\Utopia\Domains\Contact> $contacts
+     * @param array<\Utopia\Domains\Contact>|Contact $contacts
      * @param array $nameservers
      * @return array
      * @throws DomainTaken
+     * @SuppressWarnings(PHPMD.UnusedFormalParameter)
      */
     public function transfer(string $domain, array|Contact $contacts, array $nameservers = []): array

181-202: Premium suggestions ignore TLD filter

When caller passes specific TLDs, premium suggestions should respect them too.

-        if (($filterType === null || $filterType === 'premium') && $count < $limit) {
-            foreach ($this->premiumDomains as $domain => $price) {
+        if (($filterType === null || $filterType === 'premium') && $count < $limit) {
+            $normalizedTlds = array_map(fn($t) => ltrim($t, '.'), $tlds);
+            foreach ($this->premiumDomains as $domain => $price) {
+                $tld = substr(strrchr($domain, '.'), 1) ?: '';
+                if (!in_array($tld, $normalizedTlds, true)) {
+                    continue;
+                }
                 if ($count >= $limit) {
                     break;
                 }

306-308: Renew baseline date may be surprising

Renewal adds years on “now + 1 year”, not from now. Consider adding years from now to better match expectations for a mock.

-        $currentExpiry = strtotime('+1 year');
-        $newExpiry = strtotime("+{$years} years", $currentExpiry);
+        $currentTime = time();
+        $newExpiry = strtotime("+{$years} years", $currentTime);
tests/MockTest.php (2)

76-81: Silence PHPMD: unused foreach key

Rename unused loop key to $_domain.

-        foreach ($results as $domain => $data) {
+        foreach ($results as $_domain => $data) {

Apply to all four foreach loops.

Also applies to: 89-93, 99-104, 110-116


162-166: Float comparison: use delta-aware assertion

Avoid brittle float equality.

-        $this->assertGreaterThan(12.99, $result['price']);
-        $this->assertEquals(12.99 * 3, $result['price']);
+        $this->assertGreaterThan(12.99, $result['price']);
+        $this->assertEqualsWithDelta(12.99 * 3, $result['price'], 0.0001);

Based on learnings (PHPUnit 9).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e8522aa and 6b06f3e.

📒 Files selected for processing (2)
  • src/Domains/Registrar/Mock.php (1 hunks)
  • tests/MockTest.php (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Domains/Registrar/Mock.php (5)
src/Domains/Registrar.php (1)
  • Registrar (7-68)
src/Domains/Contact.php (1)
  • Contact (5-44)
src/Domains/Exception.php (1)
  • Exception (5-7)
src/Domains/Registrar/Exception/DomainTaken.php (1)
  • DomainTaken (7-9)
src/Domains/Registrar/Exception/PriceNotFound.php (1)
  • PriceNotFound (7-9)
tests/MockTest.php (6)
src/Domains/Contact.php (1)
  • Contact (5-44)
src/Domains/Registrar.php (1)
  • Registrar (7-68)
src/Domains/Exception.php (1)
  • Exception (5-7)
src/Domains/Registrar/Exception/DomainTaken.php (1)
  • DomainTaken (7-9)
src/Domains/Registrar/Exception/PriceNotFound.php (1)
  • PriceNotFound (7-9)
src/Domains/Registrar/Mock.php (15)
  • Mock (10-402)
  • reset (372-376)
  • getName (60-63)
  • available (94-107)
  • purchase (118-135)
  • getPurchasedDomains (352-355)
  • suggest (148-205)
  • tlds (212-215)
  • getPrice (252-290)
  • getDomain (224-241)
  • renew (300-315)
  • transfer (326-345)
  • getTransferredDomains (362-365)
  • addTakenDomain (384-389)
  • addPremiumDomain (398-401)
🪛 PHPMD (2.15.0)
src/Domains/Registrar/Mock.php

118-118: Avoid unused parameters such as '$contacts'. (undefined)

(UnusedFormalParameter)


326-326: Avoid unused parameters such as '$contacts'. (undefined)

(UnusedFormalParameter)

tests/MockTest.php

76-76: Avoid unused local variables such as '$domain'. (undefined)

(UnusedLocalVariable)


89-89: Avoid unused local variables such as '$domain'. (undefined)

(UnusedLocalVariable)


99-99: Avoid unused local variables such as '$domain'. (undefined)

(UnusedLocalVariable)


110-110: Avoid unused local variables such as '$domain'. (undefined)

(UnusedLocalVariable)

Copy link

@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

♻️ Duplicate comments (2)
src/Domains/Registrar/Mock.php (2)

323-330: Update docblock to match signature.

Similar to the purchase() method, the docblock at line 324 should reflect the actual signature that accepts array|Contact.

Apply this diff:

     /**
      * Transfer a domain
      *
      * @param string $domain
-     * @param array<\Utopia\Domains\Contact> $contacts
+     * @param array<\Utopia\Domains\Contact>|Contact $contacts
      * @param array $nameservers
      * @return array

269-272: Exception message mismatch with test expectations.

The exception message includes the domain variable ("Invalid domain format: {$domain}"), but the test at tests/MockTest.php line 172 expects the exact message "Invalid domain format". This will cause test failures.

Apply this diff:

         $parts = explode('.', $domain);
         if (count($parts) < 2) {
-            throw new PriceNotFound("Invalid domain format: {$domain}", self::RESPONSE_CODE_BAD_REQUEST);
+            throw new PriceNotFound("Invalid domain format", self::RESPONSE_CODE_BAD_REQUEST);
         }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 23092ae and 2085f8b.

📒 Files selected for processing (2)
  • src/Domains/Registrar/Mock.php (1 hunks)
  • tests/MockTest.php (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Domains/Registrar/Mock.php (6)
src/Domains/Registrar.php (1)
  • Registrar (7-68)
src/Domains/Contact.php (2)
  • Contact (5-44)
  • toArray (24-43)
src/Domains/Exception.php (1)
  • Exception (5-7)
src/Domains/Registrar/Exception/DomainTaken.php (1)
  • DomainTaken (7-9)
src/Domains/Registrar/Exception/InvalidContact.php (1)
  • InvalidContact (7-9)
src/Domains/Registrar/Exception/PriceNotFound.php (1)
  • PriceNotFound (7-9)
tests/MockTest.php (6)
src/Domains/Contact.php (1)
  • Contact (5-44)
src/Domains/Exception.php (1)
  • Exception (5-7)
src/Domains/Registrar/Exception/DomainTaken.php (1)
  • DomainTaken (7-9)
src/Domains/Registrar/Exception/InvalidContact.php (1)
  • InvalidContact (7-9)
src/Domains/Registrar/Exception/PriceNotFound.php (1)
  • PriceNotFound (7-9)
src/Domains/Registrar/Mock.php (15)
  • Mock (11-447)
  • reset (376-380)
  • getName (70-73)
  • available (104-115)
  • purchase (127-145)
  • getPurchasedDomains (356-359)
  • suggest (158-212)
  • tlds (219-222)
  • getPrice (259-293)
  • getDomain (231-248)
  • renew (303-318)
  • transfer (330-349)
  • getTransferredDomains (366-369)
  • addTakenDomain (388-393)
  • addPremiumDomain (402-405)
🪛 PHPMD (2.15.0)
tests/MockTest.php

77-77: Avoid unused local variables such as '$domain'. (undefined)

(UnusedLocalVariable)


90-90: Avoid unused local variables such as '$domain'. (undefined)

(UnusedLocalVariable)


100-100: Avoid unused local variables such as '$domain'. (undefined)

(UnusedLocalVariable)


111-111: Avoid unused local variables such as '$domain'. (undefined)

(UnusedLocalVariable)

🔇 Additional comments (4)
tests/MockTest.php (1)

1-405: LGTM! Comprehensive test coverage for Mock adapter.

The test suite thoroughly covers the Mock registrar adapter's functionality, including:

  • Basic operations (availability, purchase, transfer, renewal)
  • Pricing scenarios (standard, premium, multi-year)
  • Error handling (DomainTaken, InvalidContact, PriceNotFound)
  • Edge cases (invalid domains, unsupported TLDs, missing fields)
  • State management (reset, dynamic domain additions)

The static analysis warnings about unused variables in foreach loops (lines 77, 90, 100, 111) are false positives—the loop variables are used in assertions within the loop body.

src/Domains/Registrar/Mock.php (3)

158-212: LGTM! Suggestion logic handles filtering correctly.

The method properly:

  • Converts array queries to hyphenated strings
  • Defaults to supported TLDs when none provided
  • Enforces limit across suggestion and premium types
  • Applies price filtering for premium domains
  • Checks availability for each suggested domain

259-293: Pricing logic correctly implements premium and standard pricing.

The method appropriately:

  • Returns premium pricing with proper flags when domain is in premium list
  • Validates domain format and TLD support
  • Applies registration type multipliers (transfer 1.0, renewal 1.1, trade 1.2)
  • Calculates multi-period pricing correctly

Note: The exception message issue at line 271 is addressed in a separate comment.


414-446: LGTM! Thorough contact validation.

The validation correctly:

  • Normalizes input to array for consistent processing
  • Verifies Contact instance types
  • Checks all required fields are present and non-empty
  • Validates email format using PHP's built-in filter

Copy link

@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

♻️ Duplicate comments (2)
src/Domains/Registrar/Mock.php (2)

117-127: Update docblock to match signature.

The docblock at line 121 still indicates array<\Utopia\Domains\Contact> but the method signature at line 127 accepts array|Contact.

Apply this diff:

     /**
      * Purchase a domain
      *
      * @param string $domain
-     * @param array<\Utopia\Domains\Contact> $contacts
+     * @param array<\Utopia\Domains\Contact>|Contact $contacts
      * @param array $nameservers
      * @return array

269-272: Test will fail: exception message mismatch.

Tests expect 'Invalid domain format' (exact). Code throws with appended domain.

-        if (count($parts) < 2) {
-            throw new PriceNotFound("Invalid domain format: {$domain}", self::RESPONSE_CODE_BAD_REQUEST);
-        }
+        if (count($parts) < 2) {
+            throw new PriceNotFound('Invalid domain format', self::RESPONSE_CODE_BAD_REQUEST);
+        }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2085f8b and 981d9bd.

📒 Files selected for processing (1)
  • src/Domains/Registrar/Mock.php (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Domains/Registrar/Mock.php (5)
src/Domains/Contact.php (2)
  • Contact (5-44)
  • toArray (24-43)
src/Domains/Exception.php (1)
  • Exception (5-7)
src/Domains/Registrar/Exception/DomainTaken.php (1)
  • DomainTaken (7-9)
src/Domains/Registrar/Exception/InvalidContact.php (1)
  • InvalidContact (7-9)
src/Domains/Registrar/Exception/PriceNotFound.php (1)
  • PriceNotFound (7-9)
🔇 Additional comments (9)
src/Domains/Registrar/Mock.php (9)

1-21: LGTM!

The namespace, imports, and response code constants are well-structured and appropriate for a mock registrar implementation.


25-96: LGTM!

The in-memory state management and constructor logic are appropriate for a mock implementation. The choice to merge takenDomains while replacing supportedTlds provides flexibility for testing scenarios.


104-115: LGTM!

The availability check correctly verifies both pre-configured taken domains and domains purchased during the mock session.


158-212: LGTM!

The suggestion logic correctly handles both regular and premium domain suggestions, with appropriate filtering by price range and type. The count tracking ensures the limit is respected across both suggestion types.


231-248: LGTM!

The getDomain() method correctly validates domain ownership and returns appropriately structured mock registry data.


259-293: LGTM!

The pricing logic correctly handles premium domains, validates TLD support, and applies appropriate multipliers based on registration type using a clean match expression.


330-349: LGTM!

The transfer logic correctly prevents transferring already-owned domains and maintains state in both transferredDomains and purchasedDomains for comprehensive tracking.


356-405: LGTM!

The test helper methods appropriately expose internal state and provide mutation capabilities for comprehensive testing scenarios.


414-446: LGTM!

The contact validation logic is thorough and correctly handles both single Contact instances and arrays. The use of FILTER_VALIDATE_EMAIL and comprehensive required field checking ensures data integrity.

@loks0n loks0n merged commit b6d4c3a into master Oct 16, 2025
7 checks passed
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.

4 participants