-
Notifications
You must be signed in to change notification settings - Fork 54
Fix: Adding #username to subscription link option #105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Fix: Adding #username to subscription link option #105
Conversation
This reverts commit 02ea80d.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Overview
Summary
This pull request consolidates subscription URL generation logic across the codebase and implements a feature to optionally add usernames to subscription links. The changes move URL construction from being scattered across multiple response models and controllers to a centralized utility function `resolveSubscriptionUrl` that can conditionally append `#username` fragments based on the `addUsernameToBaseSubscription` setting.The main architectural changes include:
- New utility function: Created
resolveSubscriptionUrlinsrc/modules/subscription/utils/resolve-subscription-url.tsthat handles conditional username appending - CQRS query handlers: Added
GetSubscriptionUrlHandlerandGetUsersSubscriptionUrlHandlerto manage single and bulk subscription URL generation - Service layer updates: Modified
SubscriptionServiceto expose new public methods for URL generation and removed duplicate logic - Response model refactoring: Updated all user response models (
CreateUserResponseModel,GetUserResponseModel,GetFullUserResponseModel) to accept pre-built subscription URLs instead of constructing them internally - Controller integration: Modified
UsersControllerandUsersServiceto return structured responses containing both user data and subscription URLs - Template engine updates: Updated template formatting to use the centralized URL generation for consistency
The consolidation ensures that the new username feature works consistently across all subscription-related endpoints, including user creation, updates, bulk operations, and template generation. The changes leverage the existing database migration that added the add_username_to_base_subscription boolean field to the subscription_settings table.
Important Files Changed
Changed Files
| Filename | Score | Overview |
|---|---|---|
| src/modules/subscription/utils/resolve-subscription-url.ts | 5/5 | New utility function for centralized subscription URL generation with optional username fragments |
| src/modules/subscription/subscription.service.ts | 4/5 | Major refactoring to use centralized URL utility and expose new public methods for URL generation |
| src/modules/users/users.service.ts | 4/5 | Extensive changes to integrate subscription URL generation across all user operations |
| src/modules/users/controllers/users.controller.ts | 4/5 | Updated all response constructors to use structured data with pre-built subscription URLs |
| src/modules/subscription-template/generators/format-hosts.service.ts | 4/5 | Integrated centralized URL generation for template formatting with username support |
| src/modules/users/models/create-user.response.model.ts | 4/5 | Refactored constructor to accept pre-built subscription URL instead of domain parameter |
| src/modules/users/models/get-user.response.model.ts | 5/5 | Updated constructor signature to use pre-constructed subscription URLs |
| src/modules/users/models/get-full-user.response.model.ts | 5/5 | Modified to accept subscription URL parameter instead of building URLs inline |
| src/common/utils/templates/replace-templates-values.ts | 4/5 | Updated template engine to accept pre-constructed subscription links for flexibility |
| src/modules/subscription/queries/get-subscription-url/get-subscription-url.handler.ts | 4/5 | New CQRS query handler for single subscription URL generation |
| src/modules/subscription/queries/get-users-subscription-url/get-users-subscription-url.handler.ts | 4/5 | New CQRS query handler for bulk subscription URL generation |
| src/modules/subscription/queries/get-subscription-url/get-subscription-url.query.ts | 5/5 | Query class encapsulating parameters for single user subscription URL generation |
| src/modules/subscription/queries/get-users-subscription-url/get-users-subscription-url.query.ts | 5/5 | Query class for bulk user subscription URL generation |
| src/modules/subscription/subscription.module.ts | 5/5 | Added new query handlers to module providers |
| src/modules/subscription/queries/index.ts | 5/5 | Index file consolidating query handlers for dependency injection |
| src/modules/subscription/queries/get-subscription-url/index.ts | 5/5 | Standard index file for query module exports |
| src/modules/subscription/queries/get-users-subscription-url/index.ts | 5/5 | Standard index file for bulk query module exports |
| src/modules/hosts/index.ts | 5/5 | Minor cosmetic reordering of exports with no functional impact |
| src/modules/users/users.module.ts | 5/5 | Minor import reordering for code consistency |
| src/queue/bulk-user-operations/index.ts | 5/5 | Minor export reordering for code formatting consistency |
Confidence score: 4/5
- This PR is safe to merge with minimal risk as it primarily consolidates existing functionality into a centralized pattern
- Score reflects well-structured refactoring with proper error handling, though the extensive changes across multiple modules require careful testing
- Pay close attention to subscription service methods and template generation logic to ensure URL formatting works correctly across all use cases
Sequence Diagram
sequenceDiagram
participant User
participant Controller as "UsersController"
participant Service as "UsersService"
participant QueryBus as "QueryBus"
participant SubService as "SubscriptionService"
participant Util as "resolveSubscriptionUrl"
participant Settings as "SubscriptionSettings"
User->>Controller: "Request user operations (create/update/get)"
Controller->>Service: "createUser() / updateUser() / getUserByUniqueFields()"
Service->>Service: "Create/Update user entity"
Service->>QueryBus: "execute(GetSubscriptionUrlQuery)"
QueryBus->>SubService: "getSubscriptionUrl(shortUuid, username)"
SubService->>SubService: "getCachedSubscriptionSettings()"
SubService->>Settings: "Get addUsernameToBaseSubscription setting"
SubService->>Util: "resolveSubscriptionUrl(shortUuid, username, addUsernameToBaseSubscription, subPublicDomain)"
Util->>Util: "Check addUsernameToBaseSubscription flag"
alt addUsernameToBaseSubscription is true
Util-->>SubService: "Return https://${domain}/${shortUuid}#${username}"
else addUsernameToBaseSubscription is false
Util-->>SubService: "Return https://${domain}/${shortUuid}"
end
SubService-->>QueryBus: "Return subscription URL"
QueryBus-->>Service: "Return subscription URL"
Service->>Service: "Create response model with subscription URL"
Service-->>Controller: "Return user with subscription URL"
Controller-->>User: "Return user response with subscription URL"
Additional Comments (3)
-
src/modules/users/controllers/users.controller.ts, line 95 (link)style: The
subPublicDomainproperty is still initialized but no longer used in response construction - consider removing if not needed elsewhere -
src/modules/users/users.service.ts, line 1250 (link)style: These helper methods use arrow function syntax while other private methods in the class use regular function declarations - inconsistent with class style
-
src/modules/subscription-template/generators/format-hosts.service.ts, line 55 (link)logic: Early return when settings are unavailable now affects all users, not just inactive ones. This could impact active users if subscription settings fail to load.
20 files reviewed, 3 comments
Merge pull request remnawave#110 from remnawave/dev
Merge pull request remnawave#112 from remnawave/dev
# Conflicts: # src/common/utils/templates/replace-templates-values.ts # src/modules/subscription-template/generators/format-hosts.service.ts # src/modules/subscription/subscription.service.ts
Changes made:
Resolve remnawave/panel#165