Dev #13
Dev #13
Conversation
WalkthroughIntroduces a new .NET 6 SDK and test solution for JFW: solution/project scaffolding, HTTP connection layer, base abstractions, constants, helpers, models, API interfaces and clients (users, roles, features, events, issue categories), authentication API, DI registration, and xUnit tests. Adds repository metadata files (.gitignore, LICENSE, README, CHANGELOG). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Test/Caller
participant Api as IJfwApiClient
participant Users as UsersClient
participant Conn as ManagementConnection
participant HTTP as Remote API
Dev->>Api: UpdateAuthKey(key)
Dev->>Api: Users.GetByIdAsync(userId)
Api->>Users: Forward call
Users->>Conn: GET api/v1/users/{id}\n+ DefaultHeaders
Conn->>HTTP: HTTP GET (headers, query)
HTTP-->>Conn: ApiResponse<User>
alt Success = true
Conn-->>Users: User
Users-->>Api: User
Api-->>Dev: User
else Errors present
Conn--x Users: throws JfwException
Users--x Api: propagate
Api--x Dev: JfwException
end
sequenceDiagram
autonumber
actor Dev as App
participant DI as IServiceCollection
participant BJFW as BaseJFWClient
participant Auth as AuthApi
participant HTTP as Remote API
Dev->>DI: AddJfwSdk(options)
DI-->>Dev: Services configured
Dev->>Auth: AuthAsync(request)
Auth->>BJFW: PostAsync<AuthenticationResponse>(HttpRequest{uri,payload})
BJFW->>HTTP: POST /api/v1/users/auth\nBrand-Url, Auth-Key
HTTP-->>BJFW: 200 + body or error
BJFW-->>Auth: AuthenticationResponse
Auth-->>Dev: AuthenticationResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 42
🧹 Nitpick comments (76)
JFW.Sdk/Constants/UserTypes.cs (3)
13-14: Make the constants container static to prevent instantiation/inheritance.This is a pure constants holder; mark it static.
- public class UserTypes + public static class UserTypes
10-12: Tighten XML docs (short, consistent, non-redundant).Current phrasing is repetitive (“user user type”) and wordy.
- /// <summary> - /// Provides all the user types. - /// </summary> + /// <summary> + /// Compile-time constants for JFW user role names. + /// </summary> - /// <summary> - /// The super admin user type. This user type is used for the super administrators. - /// </summary> + /// <summary> + /// Super administrator role. + /// </summary> - /// <summary> - /// The admin user type. This user type is used for the administrators. - /// </summary> + /// <summary> + /// Administrator role. + /// </summary> - /// <summary> - /// The partner user type. That user type is used for the partners. - /// </summary> + /// <summary> + /// Partner role. + /// </summary> - /// <summary> - /// The user user type. This user type is used for the users. - /// </summary> + /// <summary> + /// Standard user role. + /// </summary>Also applies to: 15-17, 20-22, 25-27, 30-32
1-6: Drop author/history header; rely on Git.Keeps files clean and avoids drift with commit history.
-/* -* Description: This class provides all constants for the user types. -* Author: Steve Bang. -* History: -* - 2024-08-12: Created - Steve Bang. -*/JFW.Sdk/Models/Requests/README.md (1)
1-3: Add links and a minimal example for faster onboarding.
Consider listing key request models (e.g., AuthenticationRequest) and a 5–10 line example showing serialization/usage. This keeps the README actionable.CHANGELOG.md (1)
2-5: Adopt Keep a Changelog + set a real version tag.
Replace placeholders with an Unreleased section and an initial version (e.g., 0.1.0) with today’s date to match this PR’s first drop.Apply this diff:
-Running changelog of releases since `version` +All notable changes to this project will be documented in this file. +This project adheres to [Semantic Versioning](https://semver.org/) +and follows the [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) format. -## A.B.C.D +## [Unreleased] + +## [0.1.0] - 2025-09-11 ### AddedJFW.Sdk/GlobalUsing.cs (1)
1-3: Nit: rename file to GlobalUsings.cs and keep scope minimal.
Align with SDK template naming and revisit if any of these introduce ambiguity with similarly named types.JFW.Sdk/Models/Responses/BaseResponse.cs (3)
1-6: Fix incorrect header comment (copy/paste).
Header says “constants for the user types” but this is a response base type. Update to avoid confusion.Apply this diff:
-/* -* Description: This class provides all constants for the user types. +/* +* Description: Base type for SDK response models. * Author: Steve Bang. * History: -* - 2024-08-31: Created - Steve Bang. +* - 2024-08-31: Created - Steve Bang. +* - 2025-09-11: Updated description - PR#13. */
13-15: Optional: centralize common response metadata.
If many responses share fields (e.g., RequestId, TraceId, Errors), put them here to reduce duplication.Apply this diff:
public abstract class BaseResponse { + /// <summary>Server-generated correlation identifier (if available).</summary> + public string? RequestId { get; init; } + + /// <summary>UTC timestamp of the response (if provided by the API).</summary> + public DateTimeOffset? Timestamp { get; init; } + + /// <summary>Collection of non-fatal warnings or errors.</summary> + public IReadOnlyList<string>? Errors { get; init; } }
1-1: Remove UTF-8 BOM.
The file starts with a BOM; standardize on UTF-8 without BOM per typical .NET repos to reduce noisy diffs.JFW.Sdk/Constants/HostUrl.cs (1)
4-7: Prefer configuration over hard-coded host.
Move the base URL to JfwOptions (with env/override support) and set HttpClient.BaseAddress from options. Keep this constant only as a safe default.JFW.Sdk/JFW.Sdk.csproj (1)
3-8: Add NuGet package metadata.
Helps publishing and discoverability.Apply this diff:
<PropertyGroup> <TargetFramework>net6.0</TargetFramework> <ImplicitUsings>enable</ImplicitUsings> <Nullable>enable</Nullable> <GenerateDocumentationFile>true</GenerateDocumentationFile> + <PackageId>JFW.Sdk</PackageId> + <Title>Jframework .NET SDK</Title> + <Description>Official .NET SDK for Jframework APIs.</Description> + <Authors>1Byte Software</Authors> + <Company>1Byte Software</Company> + <RepositoryUrl>https://github.com/1Byte-Software/JFW-NET-Sdk</RepositoryUrl> + <RepositoryType>git</RepositoryType> + <PackageLicenseExpression>Apache-2.0</PackageLicenseExpression> + <PackageTags>JFW;SDK;Jframework;Client;HTTP</PackageTags> </PropertyGroup>LICENSE (1)
1-12: Minor license header polish.
Consider standard Apache-2.0 header formatting (e.g., “Copyright © 2020–Present 1Byte Software”).JFW.Sdk/Abstracts/BaseModel.cs (1)
8-13: Make the base type abstract (and confirm namespace).If this is intended only for inheritance, mark it abstract to avoid accidental instantiation. Also, confirm the namespace “JFW.Sdk.Clients.Abstracts” is intentional for a model base (path suggests Abstracts, not Clients).
-public class BaseModel +public abstract class BaseModel { /// <summary> /// The unique identifier of the model. /// </summary> public string? Id { get; set; } }JFW.Sdk/Constants/HeaderKeys.cs (1)
8-20: Make the holder static.Prevent instantiation by marking the class static.
-public class HeaderKeys +public static class HeaderKeys { /// <summary> /// Authentication key header. /// </summary> public const string AuthKey = "Auth-Key";JFW.Sdk/Models/Requests/AuthenticationRequest.cs (1)
13-24: Provide a minimal constructor for required fields.Avoid relying on null-forgiving for required properties.
public class AuthenticationRequest { + public AuthenticationRequest(string username, string password) + { + Username = username ?? throw new ArgumentNullException(nameof(username)); + Password = password ?? throw new ArgumentNullException(nameof(password)); + } + /// <summary> /// The username of the user. /// </summary> public string Username { get; set; } = null!;JFW.Sdk/Models/BasePagination.cs (1)
12-27: Confirm page numbering semantics and consider simple validation.Defaulting PageNumber to 0 assumes 0-based paging; many APIs are 1-based. Verify server expectation. Optionally add a Validate() to guard negative page numbers and non-positive sizes.
public class BasePagination { + public void Validate() + { + if (PageNumber < 0) throw new ArgumentOutOfRangeException(nameof(PageNumber), "PageNumber cannot be negative."); + if (PageSize <= 0) throw new ArgumentOutOfRangeException(nameof(PageSize), "PageSize must be greater than 0."); + }JFW.Sdk/Models/Users/UserBaseCreateRequest.cs (1)
8-33: Lightweight client-side validation (optional).If you already use DataAnnotations, add basic constraints to help callers catch issues earlier (lengths and confirm match).
+using System.ComponentModel.DataAnnotations; + public class UserBaseCreateRequest { /// <summary> /// The password for the user to create. /// </summary> - public string Password { get; set; } = null!; + [Required, MinLength(8)] + public string Password { get; set; } = null!; /// <summary> /// Confirmation of the password for the user to create. /// </summary> - public string ConfirmPassword { get; set; } = null!; + [Required, Compare(nameof(Password))] + public string ConfirmPassword { get; set; } = null!; /// <summary> /// The first name of the user to create. /// </summary> public string? FirstName { get; set; }JFW.Sdk/IManagementConnection.cs (3)
27-35: Prefer IReadOnlyDictionary for headers to prevent accidental mutation.Headers are inputs and shouldn’t be mutated by implementations.
Apply this minimal change (and mirror it on the other methods for consistency):
- IDictionary<string, string>? headers = null, + IReadOnlyDictionary<string, string>? headers = null,
26-27: Avoid silent deserialization fallbacks.“Returns default if empty or not parsable” risks masking real failures. Consider throwing a typed exception on deserialization errors, or introducing a TrySendAsync/Result alternative to surface parse failures explicitly.
4-4: Verify dependency on Newtonsoft in the public interface surface.Coupling the transport abstraction to Newtonsoft.Json (JsonConverter[]) reduces flexibility if you later switch to System.Text.Json or custom serializers. Consider hiding serializer details behind an internal adapter or a serializer-agnostic options object.
JFW.Sdk/Constants/JfwStatus.cs (2)
4-7: Tighten summary wording.Minor grammar/polish.
-/// <summary> -/// Provide status of the JFW common. -/// </summary> +/// <summary> +/// Provides status for JFW entities. +/// </summary>
7-18: Consider string-based enum serialization for API clarity.If the API emits/accepts “Active”/“Inactive”, annotate for string enum JSON to avoid numeric values on the wire.
+ [Newtonsoft.Json.JsonConverter(typeof(Newtonsoft.Json.Converters.StringEnumConverter))] public enum JfwStatus {Confirm your API actually uses string enum values; otherwise skip.
JFW.Sdk.sln (1)
8-9: Unify SDK-style project type GUIDs (nit).Optional consistency: use the SDK-style C# GUID for the test project as well.
-Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "JFW.Sdk.Tests", "JFW.Sdk.Tests\JFW.Sdk.Tests.csproj", "{FE6EB11F-DC22-4A6B-AC71-6C0CC3E97777}" +Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "JFW.Sdk.Tests", "JFW.Sdk.Tests\JFW.Sdk.Tests.csproj", "{FE6EB11F-DC22-4A6B-AC71-6C0CC3E97777}"JFW.Sdk/Clients/Interfaces/IRolesClient.cs (1)
9-18: Pass CancellationToken through client methods.Expose cancellation to callers for timeouts/shutdowns.
- /// <param name="id">The id of the role to get.</param> - /// <returns></returns> - Task<Role?> GetByIdAsync(string id); + /// <param name="id">The id of the role to get.</param> + /// <param name="cancellationToken">The cancellation token.</param> + /// <returns></returns> + Task<Role?> GetByIdAsync(string id, CancellationToken cancellationToken = default);If you adopt this, remember to thread the token through the implementation and call sites.
JFW.Sdk/Models/Users/User.cs (2)
96-100: Align Status type with shared enum (if API supports it).Other models use JfwStatus; using string here is inconsistent and can hide invalid values.
- public string Status { get; set; } = null!; + public JfwStatus? Status { get; set; }If the service returns string values, ensure StringEnumConverter is configured (or keep string).
123-126: Prefer DateTimeOffset for timestamps.Avoids timezone ambiguity on wire-level dates.
- public DateTime? ExpiryDate { get; set; } + public DateTimeOffset? ExpiryDate { get; set; }JFW.Sdk/Clients/Interfaces/IIssueCategoriesClient.cs (1)
9-19: Grammar + cancellation support.Small doc fix and expose CancellationToken.
- /// Gets a issue category by the given code. + /// Gets an issue category by the given code. @@ - /// <param name="code">The code of the issue category to get.</param> - /// <returns></returns> - Task<IssueCategory?> GetByCodeAsync(string code); + /// <param name="code">The code of the issue category to get.</param> + /// <param name="cancellationToken">The cancellation token.</param> + /// <returns></returns> + Task<IssueCategory?> GetByCodeAsync(string code, CancellationToken cancellationToken = default);Propagate the token in the implementation and call sites if adopted.
JFW.Sdk/Abstracts/PaginationList.cs (2)
10-19: Prefer an immutable, materialized collection and guard against invalid totalsIEnumerable is fine, but for DTOs it’s safer to materialize (avoid deferred execution) and make read-only. Also consider preventing negative totals.
- public IEnumerable<T> Items { get; set; } = Enumerable.Empty<T>(); + public IReadOnlyList<T> Items { get; init; } = Array.Empty<T>(); @@ - public int TotalItems { get; set; } + public int TotalItems { get; init; }If other code relies on settable properties, keep set but validate on construction/factory.
4-8: Fill in XML docsThe type and type parameter summaries are blank.
JFW.Sdk/Clients/Implements/FeaturesClient.cs (1)
25-37: Validate inputs and consider CancellationTokenGuard against empty inputs; optionally add a CT if the connection supports it.
- public Task<bool> CheckUserAccessAsync(string userId, string featureCode) + public Task<bool> CheckUserAccessAsync(string userId, string featureCode) { - return Connection.GetAsync<bool>( + if (string.IsNullOrWhiteSpace(userId)) throw new ArgumentException("userId is required.", nameof(userId)); + if (string.IsNullOrWhiteSpace(featureCode)) throw new ArgumentException("featureCode is required.", nameof(featureCode)); + + return Connection.GetAsync<bool>( UrlHelper.BuildUri( BaseUri, "check", // api/v1/features/check new Dictionary<string, string>() { { nameof(userId), userId }, { nameof(featureCode), featureCode }, }), // Build parameters DefaultHeaders ); }If IManagementConnection has CT-aware overloads, propagate it here and through the interface.
JFW.Sdk/Clients/Interfaces/IEventClient.cs (2)
20-30: Expose CancellationToken and return a non-null pagination containerReturning null for lists is awkward for consumers; prefer an empty page. Also surface CT.
- Task<Event?> GetByIdAsync(string id); + Task<Event?> GetByIdAsync(string id, CancellationToken cancellationToken = default); @@ - Task<PaginationList<Event>?> GetAllAsync(GetEventsRequest request); + Task<PaginationList<Event>> GetAllAsync(GetEventsRequest request, CancellationToken cancellationToken = default);Confirm implementors and callers are updated.
9-10: File/interface naming consistencyFile name is IEventClient.cs but interface is IEventsClient. Align to avoid search/maintenance friction.
JFW.Sdk/Extensions/ServiceCollectionExtensions.cs (1)
41-44: Consider registering the aggregate client and adding transient-fault handlingRegister IJfwApiClient (if intended) and add a retry/backoff policy (e.g., Polly) keyed off options.RetryCount/Delay.
JFW.Sdk/Models/Roles/Role.cs (1)
39-43: Expose permissions as read-only for DTO safetyIf mutation isn’t required post-deserialization, prefer read-only to avoid accidental modification.
- public List<string> Permissions { get; set; } = new List<string>(); + public IReadOnlyList<string> Permissions { get; init; } = Array.Empty<string>();JFW.Sdk.Tests/FeaturesTests.cs (1)
37-39: Remove redundant type assertionThe result is already bool.
- Assert.IsType<bool>(result); Assert.False(result);JFW.Sdk/Models/Users/UserCreateByEmailRequest.cs (2)
2-3: Namespace likely inconsistent with base type locationBase class appears under JFW.Sdk.Models.Users. Align namespaces for clarity and simpler imports.
-namespace JFW.Sdk.Models; +namespace JFW.Sdk.Models.Users;
12-12: Consider adding basic validation annotationsIf these DTOs are validated via DataAnnotations, mark EmailAddress accordingly.
+ [EmailAddress] public string EmailAddress { get; set; }Remember to add:
using System.ComponentModel.DataAnnotations;JFW.Sdk/Abstracts/ApiResponse.cs (3)
15-18: Prefer HttpStatusCode over int for status codesUsing the enum improves readability and avoids magic numbers.
- public int StatusCode { get; set; } + public System.Net.HttpStatusCode StatusCode { get; set; }
25-29: Use IReadOnlyList for error collectionArrays are mutable and harder to extend; IReadOnlyList better expresses intent and is friendlier for serializers.
- public ApiErrorResponse[]? Errors { get; set; } + public IReadOnlyList<ApiErrorResponse>? Errors { get; init; }
39-49: Tighten nullability/immutability on error fieldsThese look required. Consider init-only and optionally add a minimal constructor.
- public string Message { get; set; } = null!; + public string Message { get; init; } = null!; - public string Code { get; set; } = null!; + public string Code { get; init; } = null!; - public string CorrelationId { get; set; } = null!; + public string CorrelationId { get; init; } = null!;JFW.Sdk/Clients/Implements/RolesClient.cs (2)
16-19: Fix copy/paste in XML doc: UsersClient → RolesClientMinor doc correctness.
- /// Initializes a new instance of the <see cref="UsersClient"/> class. + /// Initializes a new instance of the <see cref="RolesClient"/> class.
24-31: Validate id input to fail fastAvoid round-trips on empty/whitespace ids.
- public Task<Role?> GetByIdAsync(string id) + public Task<Role?> GetByIdAsync(string id) { + if (string.IsNullOrWhiteSpace(id)) + throw new ArgumentException("id cannot be null or empty.", nameof(id)); return Connection.GetAsync<Role>( UrlHelper.BuildUriRelative(BaseUri, id), // api/v1/roles/{id} DefaultHeaders ); }JFW.Sdk/Clients/Implements/IssueCategoriesClient.cs (1)
26-26: Unify generic type nullability with other clientsUse IssueCategory (not IssueCategory?) as the generic argument; the Task<T?> already communicates nullability.
JFW.Sdk/Models/Users/UserCreateByPhoneRequest.cs (2)
9-13: Fix XML doc: phone number, not email addressMinor doc accuracy.
- /// The email address of the user to create. + /// The phone number of the user to create.
18-32: Optional: validate phone format at constructionConsider basic E.164 validation to catch mistakes early; alternatively, add a TODO to centralize validation.
JFW.Sdk/Models/IssueCategories/IssueCategory.cs (1)
44-48: Rename SuggestionURL → SuggestionUrl for .NET naming; map JSON with attributeKeeps public API idiomatic while preserving wire format via [JsonProperty("suggestionURL")].
- public string? SuggestionURL { get; set; } + // [JsonProperty("suggestionURL")] // add Newtonsoft.Json import if needed + public string? SuggestionUrl { get; set; }Ensure serializer settings or attributes match the backend payload before renaming.
JFW.Sdk/Abstracts/HttpRequest.cs (1)
11-21: Consider making Uri and Payload nullable or init-onlyIf Payload is optional, mark it nullable. If Uri must always be provided, consider init-only to ensure construction-time assignment.
- public string Uri { get; set; } + public string Uri { get; init; } = null!; - public object Payload { get; set; } + public object? Payload { get; set; }Also applies to: 23-40
JFW.Sdk.Tests/RolesTests.cs (1)
38-41: Assert.NotNull before Assert.IsType for clearer failuresMinor ordering for diagnostics.
- Assert.IsType<Role>(roleResult); - Assert.NotNull(roleResult); + Assert.NotNull(roleResult); + Assert.IsType<Role>(roleResult);JFW.Sdk/IJfwApiClient.cs (1)
44-48: Reduce API surface: consider removing UpdateAuthKey in favor of AddOrUpdateDefaultHeader.
It’s a specialized duplicate of AddOrUpdateDefaultHeader(HeaderKeys.AuthKey, …). If you keep it, mark it as a convenience wrapper in docs.JFW.Sdk/Clients/Implements/EventClient.cs (1)
25-40: Offer CancellationToken overloads.
SDK methods should allow cancellation/timeouts control in consumer apps.If IManagementConnection supports it, add optional CancellationToken parameters and forward them.
JFW.Sdk/Clients/Interfaces/IUsersClient.cs (2)
60-66: Complete XML docs for GetAllAsync.
The summary/param/returns are empty; fill in endpoint and semantics.- /// <summary> - /// - /// </summary> - /// <param name="request"></param> - /// <returns></returns> + /// <summary> + /// Gets a paged list of users filtered by the provided criteria. + /// API Endpoint: GET api/v1/users + /// </summary> + /// <param name="request">Filter and pagination criteria.</param> + /// <returns>Returns a paginated list of users, or null if no data.</returns> Task<PaginationList<User>?> GetAllAsync(GetUsersRequest request);
11-35: Consider CancellationToken support across the interface.
Add optional CancellationToken parameters to all methods for robustness in high-latency environments.Also applies to: 37-59, 60-66
JFW.Sdk.Tests/EventsTests.cs (2)
66-87: Stabilize GetAll test with env config and minimal assertions.
Avoid real calls without config; reuse initialized auth key; ensure only runs when JFW_EVENT_CODE is set.- string eventCode = "Event_Code"; - - string authKey = "YOUR_AUTH_KEY"; - - _apiClients.UpdateAuthKey(authKey); + var eventCode = Environment.GetEnvironmentVariable("JFW_EVENT_CODE"); + if (string.IsNullOrWhiteSpace(eventCode)) + throw new Xunit.Sdk.SkipException("Set JFW_EVENT_CODE for this test."); ... - Code = eventCode + Code = eventCode
5-7: Namespace naming nit: place tests under a matching namespace.
Consider JFW.Sdk.Tests.Events to reflect file name.-namespace JFW.Sdk.Tests.Features; +namespace JFW.Sdk.Tests.Events;JFW.Sdk/Abstracts/BaseClient.cs (2)
67-76: Guard string baseUri overload; trim slashes to keep stable URL composition.public BaseClient( IManagementConnection managementConnection, IDictionary<string, string> defaultHeaders, string baseUri ) : this( managementConnection, defaultHeaders, - new Uri(baseUri, UriKind.Relative) + new Uri( + string.IsNullOrWhiteSpace(baseUri) + ? throw new ArgumentException("baseUri cannot be null or whitespace.", nameof(baseUri)) + : baseUri.TrimEnd('/'), + UriKind.Relative) ) { }
4-7: Fill in XML docs for BaseClient and members.
Improves SDK IntelliSense.JFW.Sdk/Models/Users/GetUsersRequest.cs (2)
47-63: Harden input normalization (trim/ignore blanks).Avoid sending empty/whitespace query values and normalize Ids before join.
- if (Ids != null && Ids.Any()) - dictionary.Add("ids", string.Join(",", Ids)); + if (Ids != null) + { + var cleaned = Ids.Where(s => !string.IsNullOrWhiteSpace(s)) + .Select(s => s.Trim()) + .ToArray(); + if (cleaned.Length > 0) + dictionary.Add("ids", string.Join(",", cleaned)); + } - if (!string.IsNullOrEmpty(RoleId)) - dictionary.Add("roleId", RoleId); + if (!string.IsNullOrWhiteSpace(RoleId)) + dictionary.Add("roleId", RoleId!.Trim()); - if (!string.IsNullOrEmpty(Code)) - dictionary.Add("code", Code); + if (!string.IsNullOrWhiteSpace(Code)) + dictionary.Add("code", Code!.Trim()); - if (!string.IsNullOrEmpty(FirstName)) - dictionary.Add("firstName", FirstName); + if (!string.IsNullOrWhiteSpace(FirstName)) + dictionary.Add("firstName", FirstName!.Trim()); - if (!string.IsNullOrEmpty(LastName)) - dictionary.Add("lastName", LastName); + if (!string.IsNullOrWhiteSpace(LastName)) + dictionary.Add("lastName", LastName!.Trim()); - if (!string.IsNullOrEmpty(NickName)) - dictionary.Add("nickName", NickName); + if (!string.IsNullOrWhiteSpace(NickName)) + dictionary.Add("nickName", NickName!.Trim());
39-43: Polish the XML summary.- /// Build to dictionary + /// Builds a dictionary of query parameters.JFW.Sdk/Abstracts/JfwException.cs (2)
18-28: Ensure the parameterless ctor is safe and add inner-exception overload.Keep a default message and code for the empty ctor; add an overload to preserve exception chaining.
-public JfwException() { } +public JfwException() : base("Unknown error message") { } public JfwException(string code, string message) : base(message) { Code = code; } + +public JfwException(string code, string message, Exception? innerException) : base(message, innerException) +{ + Code = code; +}
1-52: Consider capturing CorrelationId.ApiErrorResponse exposes CorrelationId; surfacing it on JfwException improves diagnostics.
If helpful, I can add a CorrelationId property and wire it through constructors/factory.
JFW.Sdk/Abstracts/BaseJfwClient.cs (3)
37-41: Throw ArgumentException for invalid brandUrl.NullReferenceException is the wrong choice for parameter validation.
- if (string.IsNullOrEmpty(brandUrl)) throw new NullReferenceException(nameof(brandUrl)); + if (string.IsNullOrWhiteSpace(brandUrl)) + throw new ArgumentException("brandUrl cannot be null or whitespace.", nameof(brandUrl));
31-33: Prefer configuring BaseAddress via DI, not inside the client.Hardcoding BaseAddress reduces flexibility (multi-host, testing). Consider moving this to the HttpClient factory setup.
56-56: Rename the generic type parameter for clarity.BaseResponse as a type parameter reads like a concrete type. Prefer TResponse.
Also applies to: 74-74, 92-92
JFW.Sdk/Models/Events/Event.cs (2)
14-18: Align doc with property name.-/// The group name of the event. +/// The group code name of the event.
39-43: Modeling of Tags.If the API returns tags as an array, prefer a collection (e.g., List) over a comma-delimited string for type safety.
JFW.Sdk.Tests/UsersTest.cs (1)
24-49: Consider isolating external calls.Prefer mocking
IManagementConnection/HttpMessageHandlerto validate URLs/headers without hitting the network; keep live tests behind a separate Integration test category.Also applies to: 68-78
JFW.Sdk/Helpers/UrlHelper.cs (2)
18-24: Handle base URIs that already include query strings.Current implementation always appends
?{queryString}. Use&when a query already exists.Apply:
- public static string BuildUriQuery(Uri baseUri, string? queryString) + public static string BuildUriQuery(Uri baseUri, string? queryString) { if (string.IsNullOrEmpty(queryString)) return baseUri.ToString(); - else - return $"{baseUri}?{queryString}"; + var separator = baseUri.ToString().Contains('?') ? "&" : "?"; + return $"{baseUri}{separator}{queryString}"; }Also applies to: 32-37, 60-65
72-89: Minor: avoid nullability mismatch and extra ToString.
queryStringsis non-null in callers. The null-conditional?and trailing.ToString()on the Aggregate may hide errors. Keep it simple.Apply:
- public static string? AddQueryString(IDictionary<string, string> queryStrings) + public static string? AddQueryString(IDictionary<string, string> queryStrings) { - // Add the query strings - var queryString = queryStrings?.Aggregate(new StringBuilder(), (sb, kvp) => + var queryString = queryStrings.Aggregate(new StringBuilder(), (sb, kvp) => { if (kvp.Value != null) { if (sb.Length > 0) sb = sb.Append('&'); sb.Append($"{Uri.EscapeDataString(kvp.Key)}={Uri.EscapeDataString(kvp.Value)}"); } return sb; - }) - .ToString(); + }).ToString(); return queryString; }JFW.Sdk/Apis/Implements/AuthApi.cs (1)
24-49: Propagate CancellationToken support.Consider accepting a
CancellationTokenin each method and passing it to_client.PostAsync/GetAsyncto avoid hangs on network issues.Also applies to: 51-75, 77-101, 103-127, 129-153, 155-179
JFW.Sdk/ManagementConnection.cs (2)
53-58: Preserve serializer settings when custom converters are passed.Currently, passing converters drops the default settings.
Apply:
- return JsonConvert.DeserializeObject<ApiResponse<T>>(content, - converters == null ? jsonSerializerSettings : new JsonSerializerSettings() { Converters = converters }); + var settings = converters == null + ? jsonSerializerSettings + : new JsonSerializerSettings + { + NullValueHandling = NullValueHandling.Ignore, + DateParseHandling = DateParseHandling.DateTime, + ContractResolver = new CamelCasePropertyNamesContractResolver(), + Converters = converters + }; + return JsonConvert.DeserializeObject<ApiResponse<T>>(content, settings);
81-134: Add timeouts/retry policy at call-site or via HttpClient configuration.Long-lived hangs hurt resilience. Consider setting
HttpClient.Timeoutand/or plugging a Polly policy externally.JFW.Sdk/Exceptions/JfwException.cs (5)
69-81: Prefer HttpStatusCode over int for stronger typing.Use the framework enum and keep ErrorCode numeric via cast.
Apply:
+using System.Net; @@ - public int StatusCode { get; } + public HttpStatusCode StatusCode { get; } @@ - public JfwApiException(string message, int statusCode) - : base(message, $"API_ERROR_{statusCode}") + public JfwApiException(string message, HttpStatusCode statusCode) + : base(message, $"API_ERROR_{(int)statusCode}") { StatusCode = statusCode; } @@ - public JfwApiException(string message, int statusCode, Exception innerException) - : base(message, $"API_ERROR_{statusCode}", innerException) + public JfwApiException(string message, HttpStatusCode statusCode, Exception innerException) + : base(message, $"API_ERROR_{(int)statusCode}", innerException) { StatusCode = statusCode; }
49-59: Seal leaf exceptions.These are not meant for further inheritance; sealing clarifies intent and enables minor runtime optimizations.
Apply:
- public class JfwAuthenticationException : JfwException + public sealed class JfwAuthenticationException : JfwException @@ - public class JfwApiException : JfwException + public sealed class JfwApiException : JfwException @@ - public class JfwConfigurationException : JfwException + public sealed class JfwConfigurationException : JfwExceptionAlso applies to: 64-82, 87-97
18-43: Consider adding standard/serialization constructors if public SDK stability across boundaries matters.If you target broad reuse, add parameterless and protected (SerializationInfo, StreamingContext) constructors and [Serializable] on types; also persist ErrorCode/StatusCode in GetObjectData. Otherwise, skip—.NET 6 apps rarely require binary serialization.
Do you need binary serialization or cross-AppDomain compatibility? If yes, I can generate the exact patches.
46-58: Centralize error code strings.Avoid hardcoding "AUTH_ERROR"/"CONFIG_ERROR" in multiple places; define shared constants to prevent drift.
I can add a static JfwErrorCodes class and update call sites on request.
Also applies to: 84-96
8-9: Unify duplicate JfwException — rename Exceptions/JfwException.cs to JfwSdkExceptionTwo definitions exist: JFW.Sdk/Abstracts/JfwException.cs and JFW.Sdk/Exceptions/JfwException.cs. Rename the Exceptions variant (and its file) to JfwSdkException (consider making it abstract) and update its derived classes (JfwAuthenticationException, JfwApiException, JfwConfigurationException) and all usages — or consolidate into a single canonical base if that was the intent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
.gitignore(1 hunks)CHANGELOG.md(1 hunks)JFW.Sdk.Tests/EventsTests.cs(1 hunks)JFW.Sdk.Tests/FeaturesTests.cs(1 hunks)JFW.Sdk.Tests/JFW.Sdk.Tests.csproj(1 hunks)JFW.Sdk.Tests/RolesTests.cs(1 hunks)JFW.Sdk.Tests/UsersTest.cs(1 hunks)JFW.Sdk.Tests/Usings.cs(1 hunks)JFW.Sdk.sln(1 hunks)JFW.Sdk/Abstracts/ApiResponse.cs(1 hunks)JFW.Sdk/Abstracts/BaseClient.cs(1 hunks)JFW.Sdk/Abstracts/BaseJfwClient.cs(1 hunks)JFW.Sdk/Abstracts/BaseModel.cs(1 hunks)JFW.Sdk/Abstracts/HttpRequest.cs(1 hunks)JFW.Sdk/Abstracts/JfwException.cs(1 hunks)JFW.Sdk/Abstracts/PaginationList.cs(1 hunks)JFW.Sdk/Apis/Implements/AuthApi.cs(1 hunks)JFW.Sdk/Apis/Interfaces/IAuthApi.cs(1 hunks)JFW.Sdk/Clients/Implements/EventClient.cs(1 hunks)JFW.Sdk/Clients/Implements/FeaturesClient.cs(1 hunks)JFW.Sdk/Clients/Implements/IssueCategoriesClient.cs(1 hunks)JFW.Sdk/Clients/Implements/RolesClient.cs(1 hunks)JFW.Sdk/Clients/Implements/UsersClient.cs(1 hunks)JFW.Sdk/Clients/Interfaces/IEventClient.cs(1 hunks)JFW.Sdk/Clients/Interfaces/IFeaturesClient.cs(1 hunks)JFW.Sdk/Clients/Interfaces/IIssueCategoriesClient.cs(1 hunks)JFW.Sdk/Clients/Interfaces/IRolesClient.cs(1 hunks)JFW.Sdk/Clients/Interfaces/IUsersClient.cs(1 hunks)JFW.Sdk/Constants/HeaderKeys.cs(1 hunks)JFW.Sdk/Constants/HostUrl.cs(1 hunks)JFW.Sdk/Constants/JfwStatus.cs(1 hunks)JFW.Sdk/Constants/UserTypes.cs(1 hunks)JFW.Sdk/Exceptions/JfwException.cs(1 hunks)JFW.Sdk/Extensions/ServiceCollectionExtensions.cs(1 hunks)JFW.Sdk/GlobalUsing.cs(1 hunks)JFW.Sdk/Helpers/UrlHelper.cs(1 hunks)JFW.Sdk/IJfwApiClient.cs(1 hunks)JFW.Sdk/IManagementConnection.cs(1 hunks)JFW.Sdk/JFW.Sdk.csproj(1 hunks)JFW.Sdk/JfwApiClient.cs(1 hunks)JFW.Sdk/ManagementConnection.cs(1 hunks)JFW.Sdk/Models/BasePagination.cs(1 hunks)JFW.Sdk/Models/Events/Event.cs(1 hunks)JFW.Sdk/Models/Events/GetEventsRequest.cs(1 hunks)JFW.Sdk/Models/IssueCategories/IssueCategory.cs(1 hunks)JFW.Sdk/Models/Options/JfwOptions.cs(1 hunks)JFW.Sdk/Models/Requests/AuthenticationRequest.cs(1 hunks)JFW.Sdk/Models/Requests/README.md(1 hunks)JFW.Sdk/Models/Responses/AuthenticationResponse.cs(1 hunks)JFW.Sdk/Models/Responses/BaseResponse.cs(1 hunks)JFW.Sdk/Models/Roles/Role.cs(1 hunks)JFW.Sdk/Models/Users/GetUsersRequest.cs(1 hunks)JFW.Sdk/Models/Users/User.cs(1 hunks)JFW.Sdk/Models/Users/UserBaseCreateRequest.cs(1 hunks)JFW.Sdk/Models/Users/UserCreateByEmailRequest.cs(1 hunks)JFW.Sdk/Models/Users/UserCreateByPhoneRequest.cs(1 hunks)LICENSE(1 hunks)README.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (33)
JFW.Sdk/Clients/Interfaces/IRolesClient.cs (1)
JFW.Sdk/Models/Roles/Role.cs (1)
Role(7-43)
JFW.Sdk/Models/Events/Event.cs (1)
JFW.Sdk/Abstracts/BaseModel.cs (1)
BaseModel(8-14)
JFW.Sdk/Clients/Interfaces/IUsersClient.cs (5)
JFW.Sdk/Models/Users/User.cs (1)
User(7-141)JFW.Sdk/Models/Users/UserCreateByEmailRequest.cs (2)
UserCreateByEmailRequest(7-33)UserCreateByEmailRequest(17-31)JFW.Sdk/Models/Users/UserCreateByPhoneRequest.cs (2)
UserCreateByPhoneRequest(7-34)UserCreateByPhoneRequest(18-32)JFW.Sdk/Abstracts/PaginationList.cs (1)
PaginationList(8-19)JFW.Sdk/Models/Users/GetUsersRequest.cs (1)
GetUsersRequest(7-67)
JFW.Sdk/Models/Roles/Role.cs (1)
JFW.Sdk/Abstracts/BaseModel.cs (1)
BaseModel(8-14)
JFW.Sdk/Apis/Interfaces/IAuthApi.cs (3)
JFW.Sdk/Models/Responses/AuthenticationResponse.cs (1)
AuthenticationResponse(14-40)JFW.Sdk/Models/Requests/AuthenticationRequest.cs (1)
AuthenticationRequest(13-24)JFW.Sdk/Models/Responses/BaseResponse.cs (1)
BaseResponse(13-15)
JFW.Sdk/Clients/Interfaces/IIssueCategoriesClient.cs (1)
JFW.Sdk/Models/IssueCategories/IssueCategory.cs (1)
IssueCategory(7-53)
JFW.Sdk/Abstracts/HttpRequest.cs (1)
JFW.Sdk/JfwApiClient.cs (1)
Dictionary(75-85)
JFW.Sdk/Models/Responses/AuthenticationResponse.cs (1)
JFW.Sdk/Models/Responses/BaseResponse.cs (1)
BaseResponse(13-15)
JFW.Sdk/Models/Users/GetUsersRequest.cs (1)
JFW.Sdk/Models/BasePagination.cs (1)
BasePagination(7-28)
JFW.Sdk/Apis/Implements/AuthApi.cs (6)
JFW.Sdk/Apis/Interfaces/IAuthApi.cs (6)
Task(11-11)Task(16-16)Task(21-21)Task(26-26)Task(31-31)Task(36-36)JFW.Sdk/Models/Responses/AuthenticationResponse.cs (1)
AuthenticationResponse(14-40)JFW.Sdk/Models/Requests/AuthenticationRequest.cs (1)
AuthenticationRequest(13-24)JFW.Sdk/Abstracts/HttpRequest.cs (1)
HttpRequest(8-73)JFW.Sdk/Exceptions/JfwException.cs (3)
JfwAuthenticationException(49-59)JfwAuthenticationException(51-53)JfwAuthenticationException(55-58)JFW.Sdk/Models/Responses/BaseResponse.cs (1)
BaseResponse(13-15)
JFW.Sdk/Clients/Interfaces/IEventClient.cs (3)
JFW.Sdk/Models/Events/Event.cs (1)
Event(7-58)JFW.Sdk/Abstracts/PaginationList.cs (1)
PaginationList(8-19)JFW.Sdk/Models/Events/GetEventsRequest.cs (1)
GetEventsRequest(7-99)
JFW.Sdk/Models/Users/UserCreateByPhoneRequest.cs (1)
JFW.Sdk/Models/Users/UserBaseCreateRequest.cs (1)
UserBaseCreateRequest(7-34)
JFW.Sdk/Clients/Implements/IssueCategoriesClient.cs (4)
JFW.Sdk/Abstracts/BaseClient.cs (4)
BaseClient(7-79)BaseClient(35-44)BaseClient(51-59)BaseClient(67-77)JFW.Sdk/Clients/Interfaces/IIssueCategoriesClient.cs (1)
Task(19-19)JFW.Sdk/Models/IssueCategories/IssueCategory.cs (1)
IssueCategory(7-53)JFW.Sdk/Helpers/UrlHelper.cs (2)
UrlHelper(9-90)BuildUriRelative(45-51)
JFW.Sdk/Clients/Implements/FeaturesClient.cs (3)
JFW.Sdk/Abstracts/BaseClient.cs (4)
BaseClient(7-79)BaseClient(35-44)BaseClient(51-59)BaseClient(67-77)JFW.Sdk/Clients/Interfaces/IFeaturesClient.cs (1)
Task(18-18)JFW.Sdk/Helpers/UrlHelper.cs (2)
UrlHelper(9-90)BuildUri(60-65)
JFW.Sdk/ManagementConnection.cs (3)
JFW.Sdk/Abstracts/ApiResponse.cs (1)
ApiResponse(8-29)JFW.Sdk/IManagementConnection.cs (2)
Task(27-35)Task(46-46)JFW.Sdk/Abstracts/JfwException.cs (5)
JfwException(8-52)JfwException(18-18)JfwException(25-28)JfwException(34-37)JfwException(43-51)
JFW.Sdk/Abstracts/ApiResponse.cs (1)
JFW.Sdk/ManagementConnection.cs (1)
ApiResponse(53-58)
JFW.Sdk.Tests/FeaturesTests.cs (4)
JFW.Sdk/JfwApiClient.cs (4)
JfwApiClient(12-104)JfwApiClient(44-56)JfwApiClient(65-67)UpdateAuthKey(97-103)JFW.Sdk/Clients/Implements/FeaturesClient.cs (1)
Task(26-38)JFW.Sdk/Clients/Interfaces/IFeaturesClient.cs (1)
Task(18-18)JFW.Sdk/IJfwApiClient.cs (1)
UpdateAuthKey(48-48)
JFW.Sdk/Models/Users/User.cs (1)
JFW.Sdk/Abstracts/BaseModel.cs (1)
BaseModel(8-14)
JFW.Sdk/Models/Events/GetEventsRequest.cs (1)
JFW.Sdk/Models/BasePagination.cs (1)
BasePagination(7-28)
JFW.Sdk/Models/Users/UserCreateByEmailRequest.cs (1)
JFW.Sdk/Models/Users/UserBaseCreateRequest.cs (1)
UserBaseCreateRequest(7-34)
JFW.Sdk/IJfwApiClient.cs (1)
JFW.Sdk/JfwApiClient.cs (2)
AddOrUpdateDefaultHeader(88-94)UpdateAuthKey(97-103)
JFW.Sdk/Extensions/ServiceCollectionExtensions.cs (3)
JFW.Sdk/Models/Options/JfwOptions.cs (1)
JfwOptions(7-59)JFW.Sdk/Abstracts/BaseJfwClient.cs (4)
BaseJFWClient(17-122)BaseJFWClient(29-33)BaseJFWClient(35-41)BaseJFWClient(43-47)JFW.Sdk/Apis/Implements/AuthApi.cs (2)
AuthApi(13-180)AuthApi(18-22)
JFW.Sdk/Clients/Implements/RolesClient.cs (4)
JFW.Sdk/Abstracts/BaseClient.cs (4)
BaseClient(7-79)BaseClient(35-44)BaseClient(51-59)BaseClient(67-77)JFW.Sdk/Clients/Interfaces/IRolesClient.cs (1)
Task(17-17)JFW.Sdk/Models/Roles/Role.cs (1)
Role(7-43)JFW.Sdk/Helpers/UrlHelper.cs (2)
UrlHelper(9-90)BuildUriRelative(45-51)
JFW.Sdk/Clients/Implements/EventClient.cs (5)
JFW.Sdk/Models/Events/GetEventsRequest.cs (2)
IDictionary(63-98)GetEventsRequest(7-99)JFW.Sdk/Clients/Interfaces/IEventClient.cs (2)
Task(20-20)Task(30-30)JFW.Sdk/Abstracts/PaginationList.cs (1)
PaginationList(8-19)JFW.Sdk/Models/Events/Event.cs (1)
Event(7-58)JFW.Sdk/Helpers/UrlHelper.cs (3)
UrlHelper(9-90)AddQueryString(72-89)BuildUriRelative(45-51)
JFW.Sdk/JfwApiClient.cs (6)
JFW.Sdk/Clients/Implements/IssueCategoriesClient.cs (2)
IssueCategoriesClient(10-28)IssueCategoriesClient(18-21)JFW.Sdk/Clients/Implements/FeaturesClient.cs (2)
FeaturesClient(11-39)FeaturesClient(20-23)JFW.Sdk/Clients/Implements/RolesClient.cs (2)
RolesClient(10-32)RolesClient(19-22)JFW.Sdk/Clients/Implements/UsersClient.cs (2)
UsersClient(11-65)UsersClient(19-22)JFW.Sdk/Constants/HeaderKeys.cs (1)
HeaderKeys(8-20)JFW.Sdk/IJfwApiClient.cs (2)
AddOrUpdateDefaultHeader(42-42)UpdateAuthKey(48-48)
JFW.Sdk.Tests/UsersTest.cs (6)
JFW.Sdk/ManagementConnection.cs (2)
ManagementConnection(12-176)ManagementConnection(40-44)JFW.Sdk/JfwApiClient.cs (5)
JfwApiClient(12-104)JfwApiClient(44-56)JfwApiClient(65-67)AddOrUpdateDefaultHeader(88-94)UpdateAuthKey(97-103)JFW.Sdk/Clients/Implements/UsersClient.cs (5)
Task(26-29)Task(32-35)Task(38-44)Task(47-53)Task(56-63)JFW.Sdk/Models/Users/UserCreateByEmailRequest.cs (2)
UserCreateByEmailRequest(7-33)UserCreateByEmailRequest(17-31)JFW.Sdk/Models/Users/User.cs (1)
User(7-141)JFW.Sdk/Models/Users/UserCreateByPhoneRequest.cs (2)
UserCreateByPhoneRequest(7-34)UserCreateByPhoneRequest(18-32)
JFW.Sdk/Abstracts/JfwException.cs (1)
JFW.Sdk/Abstracts/ApiResponse.cs (1)
ApiErrorResponse(34-50)
JFW.Sdk/Abstracts/BaseJfwClient.cs (3)
JFW.Sdk/Constants/HostUrl.cs (1)
HostUrl(4-7)JFW.Sdk/Constants/HeaderKeys.cs (1)
HeaderKeys(8-20)JFW.Sdk/Abstracts/HttpRequest.cs (2)
HttpRequest(8-73)BuildUrlRequest(45-71)
JFW.Sdk.Tests/EventsTests.cs (7)
JFW.Sdk/JfwApiClient.cs (4)
JfwApiClient(12-104)JfwApiClient(44-56)JfwApiClient(65-67)UpdateAuthKey(97-103)JFW.Sdk/Clients/Interfaces/IEventClient.cs (2)
Task(20-20)Task(30-30)JFW.Sdk/Clients/Implements/EventClient.cs (2)
Task(26-33)Task(36-40)JFW.Sdk/IJfwApiClient.cs (1)
UpdateAuthKey(48-48)JFW.Sdk/Models/Events/Event.cs (1)
Event(7-58)JFW.Sdk/Models/Events/GetEventsRequest.cs (1)
GetEventsRequest(7-99)JFW.Sdk/Abstracts/PaginationList.cs (1)
PaginationList(8-19)
JFW.Sdk.Tests/RolesTests.cs (5)
JFW.Sdk/JfwApiClient.cs (4)
JfwApiClient(12-104)JfwApiClient(44-56)JfwApiClient(65-67)UpdateAuthKey(97-103)JFW.Sdk/Clients/Implements/RolesClient.cs (1)
Task(25-31)JFW.Sdk/Clients/Interfaces/IRolesClient.cs (1)
Task(17-17)JFW.Sdk/IJfwApiClient.cs (1)
UpdateAuthKey(48-48)JFW.Sdk/Models/Roles/Role.cs (1)
Role(7-43)
JFW.Sdk/Helpers/UrlHelper.cs (1)
JFW.Sdk/Models/Users/GetUsersRequest.cs (1)
IDictionary(43-66)
JFW.Sdk/Models/IssueCategories/IssueCategory.cs (1)
JFW.Sdk/Abstracts/BaseModel.cs (1)
BaseModel(8-14)
JFW.Sdk/Clients/Implements/UsersClient.cs (5)
JFW.Sdk/Abstracts/BaseClient.cs (4)
BaseClient(7-79)BaseClient(35-44)BaseClient(51-59)BaseClient(67-77)JFW.Sdk/Models/Users/GetUsersRequest.cs (2)
IDictionary(43-66)GetUsersRequest(7-67)JFW.Sdk/Clients/Interfaces/IUsersClient.cs (5)
Task(22-22)Task(35-35)Task(47-47)Task(58-58)Task(65-65)JFW.Sdk/IManagementConnection.cs (3)
Task(27-35)Task(46-46)Task(59-59)JFW.Sdk/Helpers/UrlHelper.cs (5)
UrlHelper(9-90)BuildUriRelative(45-51)AddQueryString(72-89)BuildUriQuery(18-24)BuildUriQuery(32-37)
🔇 Additional comments (16)
JFW.Sdk/Constants/UserTypes.cs (1)
18-18: Stabilize externally significant role strings — use fixed literals, not nameofIf these constants are part of persisted/API contracts, renaming fields will change emitted strings; replace nameof(...) with fixed literals.
File: JFW.Sdk/Constants/UserTypes.cs (also applies to lines 23, 28, 33)
- public const string SuperAdmin = nameof(SuperAdmin); + public const string SuperAdmin = "SuperAdmin"; - public const string Admin = nameof(Admin); + public const string Admin = "Admin"; - public const string Partner = nameof(Partner); + public const string Partner = "Partner"; - public const string User = nameof(User); + public const string User = "User";Verify usages and any hard-coded role strings:
rg -n 'UserTypes.(SuperAdmin|Admin|Partner|User)' -C2
rg -n '"(SuperAdmin|Admin|Partner|User)"' -g '!JFW.Sdk/Constants/UserTypes.cs' -C2If rg reports "No files were searched", run rg --debug or fallback to git grep -nE '(SuperAdmin|Admin|Partner|User)'.
JFW.Sdk/GlobalUsing.cs (1)
1-3: LGTM: sensible globals to cut boilerplate.
No functional concerns.JFW.Sdk/JFW.Sdk.csproj (1)
16-22: Consolidate JSON serializers or justify keeping bothNewtonsoft.Json is used in JFW.Sdk/IManagementConnection.cs and JFW.Sdk/ManagementConnection.cs (JsonConvert.SerializeObject/DeserializeObject); System.Text.Json is used in JFW.Sdk/Abstracts/BaseJfwClient.cs (JsonSerializer.Deserialize). Carrying both increases package size and maintenance surface — remove or migrate to a single serializer unless you need features from both (specific converters, attribute behaviors, or compatibility).
.gitignore (1)
1-401: Solid baseline .gitignore.Comprehensive and appropriate for a .NET SDK. No issues.
JFW.Sdk/IManagementConnection.cs (1)
28-29: Confirm HttpMethod availability via global usings.This file doesn’t import System.Net.Http. If no global using exists, add it to avoid build breaks.
JFW.Sdk.Tests/Usings.cs (1)
1-1: LGTM.Global using for xUnit is concise and keeps test files clean.
JFW.Sdk.Tests/JFW.Sdk.Tests.csproj (1)
11-22: Bump test tooling to latest stable versions (as of 2025-09-11)Update packages in JFW.Sdk.Tests/JFW.Sdk.Tests.csproj to:
- Microsoft.NET.Test.Sdk → 17.14.1
- xunit → 2.9.3 (v3 is still pre-release)
- xunit.runner.visualstudio → 3.1.4
- coverlet.collector → 6.0.4
Run the full test suite and verify CI agents; consider enabling deterministic builds and Source Link.
JFW.Sdk/Abstracts/ApiResponse.cs (1)
8-29: Verify JSON contract alignment (PascalCase vs camelCase)If the API returns camelCase fields, either ensure the JsonSerializerSettings use a CamelCaseContractResolver or add [JsonProperty] attributes to avoid deserialization misses.
Also applies to: 31-50
JFW.Sdk/Clients/Implements/RolesClient.cs (1)
10-15: LGTM on base URI compositionBaseUriClient + UrlHelper yields the expected api/v1/roles/{id} path.
JFW.Sdk/IJfwApiClient.cs (1)
12-36: Good aggregator surface; properties are cohesive and clear.
The client grouping (Events, IssueCategories, Features, Roles, Users) looks clean and discoverable.JFW.Sdk/Abstracts/BaseClient.cs (1)
2-9: No action required — global using present.
JFW.Sdk/GlobalUsing.cs containsglobal using JFW.Sdk.Clients.Abstracts;, so IManagementConnection is available to BaseClient and no explicit using is needed.JFW.Sdk/Models/Events/Event.cs (1)
12-18: Confirm dual identifiers (Id and Guid) are both required.This model inherits Id (string?) and adds Guid (Guid). If both map to API fields, fine; otherwise consider consolidating to one to avoid confusion.
JFW.Sdk/JfwApiClient.cs (1)
49-56: Confirm clients don't clone or mutate the shared DefaultHeaders instance.
Passing the same headers object to all clients is acceptable only if each client stores the reference and does not copy or modify it. Verify constructors/field assignments in EventsClient, IssueCategoriesClient, FeaturesClient, RolesClient, and UsersClient (see JFW.Sdk/JfwApiClient.cs lines 49–56).JFW.Sdk/Clients/Implements/UsersClient.cs (1)
56-63: Consider passing pagination to query.If
GetUsersRequestincludes paging, ensureToDictionary()adds pageNumber/pageSize (see related comment).JFW.Sdk/Models/Events/GetEventsRequest.cs (1)
85-87: Enum serialization format.Confirm the API expects status as string; if numeric is required, use
( (int)Status.Value ).ToString().JFW.Sdk/Exceptions/JfwException.cs (1)
3-7: Overall: solid, readable hierarchy and XML docs.Clear responsibilities and helpful properties for consumers.
| [Fact] | ||
| public async Task GetByIdAsync_Should_Result_DataCorrect() | ||
| { | ||
|
|
||
| string eventId = "EVENT_ID"; | ||
|
|
||
| string authKey = "YOUR_AUTH_KEY"; | ||
|
|
||
| _apiClients.UpdateAuthKey(authKey); | ||
|
|
||
| // Act | ||
| var result = await _apiClients.Events.GetByIdAsync(eventId); | ||
|
|
||
| // Assert | ||
| Assert.IsType<Event>(result); | ||
| Assert.NotNull(result); | ||
| } |
There was a problem hiding this comment.
Flaky: live endpoint + placeholder credentials will fail CI. Gate as integration via environment and skip if unset.
Also assert order can be simplified (NotNull before IsType).
Apply:
namespace JFW.Sdk.Tests.Features;
public class EventsTest
{
- private const string BrandUrl = "your.domain.com"; // Replace with your actual brand URL for testing
+ private readonly string? _brandUrl = Environment.GetEnvironmentVariable("JFW_BRAND_URL");
+ private readonly string? _baseUrl = Environment.GetEnvironmentVariable("JFW_BASE_URL"); // e.g. https://{subdomain}.jframework.io
+ private readonly string? _authKey = Environment.GetEnvironmentVariable("JFW_AUTH_KEY");
private readonly IJfwApiClient _apiClients;
public EventsTest()
{
- var baseUrl = "https://[subdomain].jframework.io";
+ if (string.IsNullOrWhiteSpace(_baseUrl) || string.IsNullOrWhiteSpace(_brandUrl) || string.IsNullOrWhiteSpace(_authKey))
+ throw new Xunit.Sdk.SkipException("Integration credentials not configured (JFW_BASE_URL, JFW_BRAND_URL, JFW_AUTH_KEY).");
- var httpClient = new HttpClient();
- httpClient.BaseAddress = new Uri(baseUrl);
+ var httpClient = new HttpClient { BaseAddress = new Uri(_baseUrl!, UriKind.Absolute) };
var connection = new ManagementConnection(httpClient);
- _apiClients = new JfwApiClient(BrandUrl, connection);
+ _apiClients = new JfwApiClient(_brandUrl!, connection);
+ _apiClients.UpdateAuthKey(_authKey!);
}
[Fact]
public async Task GetByIdAsync_Should_Result_DataCorrect()
{
- string eventId = "EVENT_ID";
-
- string authKey = "YOUR_AUTH_KEY";
-
- _apiClients.UpdateAuthKey(authKey);
+ // Provide a valid ID via env var to make this deterministic
+ var validId = Environment.GetEnvironmentVariable("JFW_VALID_EVENT_ID");
+ if (string.IsNullOrWhiteSpace(validId))
+ throw new Xunit.Sdk.SkipException("Set JFW_VALID_EVENT_ID for this test.");
// Act
- var result = await _apiClients.Events.GetByIdAsync(eventId);
+ var result = await _apiClients.Events.GetByIdAsync(validId!);
// Assert
- Assert.IsType<Event>(result);
Assert.NotNull(result);
+ Assert.IsType<Event>(result);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [Fact] | |
| public async Task GetByIdAsync_Should_Result_DataCorrect() | |
| { | |
| string eventId = "EVENT_ID"; | |
| string authKey = "YOUR_AUTH_KEY"; | |
| _apiClients.UpdateAuthKey(authKey); | |
| // Act | |
| var result = await _apiClients.Events.GetByIdAsync(eventId); | |
| // Assert | |
| Assert.IsType<Event>(result); | |
| Assert.NotNull(result); | |
| } | |
| namespace JFW.Sdk.Tests.Features; | |
| public class EventsTest | |
| { | |
| private readonly string? _brandUrl = Environment.GetEnvironmentVariable("JFW_BRAND_URL"); | |
| private readonly string? _baseUrl = Environment.GetEnvironmentVariable("JFW_BASE_URL"); // e.g. https://{subdomain}.jframework.io | |
| private readonly string? _authKey = Environment.GetEnvironmentVariable("JFW_AUTH_KEY"); | |
| private readonly IJfwApiClient _apiClients; | |
| public EventsTest() | |
| { | |
| if (string.IsNullOrWhiteSpace(_baseUrl) || string.IsNullOrWhiteSpace(_brandUrl) || string.IsNullOrWhiteSpace(_authKey)) | |
| throw new Xunit.Sdk.SkipException("Integration credentials not configured (JFW_BASE_URL, JFW_BRAND_URL, JFW_AUTH_KEY)."); | |
| var httpClient = new HttpClient { BaseAddress = new Uri(_baseUrl!, UriKind.Absolute) }; | |
| var connection = new ManagementConnection(httpClient); | |
| _apiClients = new JfwApiClient(_brandUrl!, connection); | |
| _apiClients.UpdateAuthKey(_authKey!); | |
| } | |
| [Fact] | |
| public async Task GetByIdAsync_Should_Result_DataCorrect() | |
| { | |
| // Provide a valid ID via env var to make this deterministic | |
| var validId = Environment.GetEnvironmentVariable("JFW_VALID_EVENT_ID"); | |
| if (string.IsNullOrWhiteSpace(validId)) | |
| throw new Xunit.Sdk.SkipException("Set JFW_VALID_EVENT_ID for this test."); | |
| // Act | |
| var result = await _apiClients.Events.GetByIdAsync(validId!); | |
| // Assert | |
| Assert.NotNull(result); | |
| Assert.IsType<Event>(result); | |
| } | |
| } |
🤖 Prompt for AI Agents
In JFW.Sdk.Tests/EventsTests.cs around lines 25 to 41 the test calls a live
endpoint using placeholder auth which makes the test flaky and will fail CI;
gate this as an integration test by checking for a specific environment variable
(e.g., INTEGRATION_TESTS or JFW_API_KEY) at the start of the test and call
Skip/Test.Skip/Assert.Skip (or use xUnit's Skip via conditional Fact) when the
env var is not set so the test only runs in integration environments, and
reorder the assertions to Assert.NotNull(result) before
Assert.IsType<Event>(result).
| [Fact] | ||
| public async Task GetByIdAsync_Should_Throw_NotFound() | ||
| { | ||
|
|
||
| string eventId = "EVENT_ID"; | ||
|
|
||
| string authKey = "YOUR_AUTH_KEY"; | ||
|
|
||
| _apiClients.UpdateAuthKey(authKey); | ||
|
|
||
| // Act | ||
| // throw BaseException if the id was not found | ||
| // Act & Assert | ||
| var exception = await Assert.ThrowsAsync<JfwException>(async () => | ||
| { | ||
| await _apiClients.Events.GetByIdAsync(eventId); | ||
| }); | ||
|
|
||
| // Assert | ||
| // Verify exception details if needed | ||
| Assert.Equal("EventNotFound", exception.Code); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
NotFound test uses the same ID as the success test; make it deterministically invalid.
Use a random GUID or env var for an invalid id.
- string eventId = "EVENT_ID";
-
- string authKey = "YOUR_AUTH_KEY";
-
- _apiClients.UpdateAuthKey(authKey);
+ var invalidId = Guid.NewGuid().ToString("N");
...
- await _apiClients.Events.GetByIdAsync(eventId);
+ await _apiClients.Events.GetByIdAsync(invalidId);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In JFW.Sdk.Tests/EventsTests.cs around lines 43 to 64, the NotFound test reuses
the same eventId as the success test which can make the failure
non-deterministic; replace the hardcoded "EVENT_ID" with a guaranteed-invalid
value such as Guid.NewGuid().ToString() or use an env var (e.g.,
Environment.GetEnvironmentVariable("INVALID_EVENT_ID") ??
Guid.NewGuid().ToString()) so the id is unique and deterministic for the
negative test, update the local variable assignment accordingly, and keep the
existing assertion checking exception.Code == "EventNotFound".
| var baseUrl = "https://[subdomain].jframework.io"; | ||
|
|
||
| var httpClient = new HttpClient(); | ||
| httpClient.BaseAddress = new Uri(baseUrl); | ||
|
|
||
| var connection = new ManagementConnection(httpClient); | ||
|
|
||
| _apiClients = new JfwApiClient(BrandUrl, connection); | ||
| } |
There was a problem hiding this comment.
Test uses an invalid BaseAddress and performs real HTTP calls
"https://[subdomain].jframework.io" is not a valid host; unit tests should not hit the network. Stub HttpMessageHandler to return a controlled response.
- var baseUrl = "https://[subdomain].jframework.io";
-
- var httpClient = new HttpClient();
- httpClient.BaseAddress = new Uri(baseUrl);
+ var httpClient = new HttpClient(new FakeHandler(_ =>
+ new HttpResponseMessage(HttpStatusCode.OK)
+ {
+ Content = new StringContent("false", Encoding.UTF8, "application/json")
+ }))
+ {
+ BaseAddress = new Uri("https://unit.test")
+ };Add this helper in the test file (or a shared test utilities file):
private sealed class FakeHandler : HttpMessageHandler
{
private readonly Func<HttpRequestMessage, HttpResponseMessage> _responder;
public FakeHandler(Func<HttpRequestMessage, HttpResponseMessage> responder) => _responder = responder;
protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
=> Task.FromResult(_responder(request));
}🤖 Prompt for AI Agents
In JFW.Sdk.Tests/FeaturesTests.cs around lines 12-20, the test uses an invalid
BaseAddress ("https://[subdomain].jframework.io") and makes real HTTP calls;
replace real network calls by adding a FakeHandler HttpMessageHandler (as
suggested) to return controlled HttpResponseMessage instances, then construct
the HttpClient with that FakeHandler and set its BaseAddress to a valid URI or
the expected test URL, pass that HttpClient into ManagementConnection, and
update tests to assert against the fake responses rather than performing live
network calls.
| private const string BrandUrl = "your.domain.com"; // Replace with your actual brand URL for testing | ||
|
|
||
| private readonly IJfwApiClient _apiClients; | ||
|
|
||
| public RolesTests() | ||
| { | ||
| var baseUrl = "https://[subdomain].jframework.io"; | ||
|
|
||
| var httpClient = new HttpClient(); | ||
| httpClient.BaseAddress = new Uri(baseUrl); | ||
|
|
||
| var connection = new ManagementConnection(httpClient); | ||
|
|
||
| _apiClients = new JfwApiClient(BrandUrl, connection); | ||
| } |
There was a problem hiding this comment.
Tests rely on real HTTP endpoints and placeholders—will fail in CI
Gate these as integration tests or mock IManagementConnection to make them unit tests.
- public RolesTests()
+ public RolesTests()
{
- var baseUrl = "https://[subdomain].jframework.io";
+ var baseUrl = "https://[subdomain].jframework.io"; // integration target
@@
- _apiClients = new JfwApiClient(BrandUrl, connection);
+ _apiClients = new JfwApiClient(BrandUrl, connection);
}Follow-up options:
- Quick: mark tests skipped until real values are provided:
-[Fact]
+[Fact(Skip = "Integration test: requires valid BaseUrl/AuthKey/RoleId")]
public async Task GetByIdAsync_Should_Result_RoleData()and similarly for the not-found test.
- Better: mock IManagementConnection and assert call/URL composition.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private const string BrandUrl = "your.domain.com"; // Replace with your actual brand URL for testing | |
| private readonly IJfwApiClient _apiClients; | |
| public RolesTests() | |
| { | |
| var baseUrl = "https://[subdomain].jframework.io"; | |
| var httpClient = new HttpClient(); | |
| httpClient.BaseAddress = new Uri(baseUrl); | |
| var connection = new ManagementConnection(httpClient); | |
| _apiClients = new JfwApiClient(BrandUrl, connection); | |
| } | |
| private const string BrandUrl = "your.domain.com"; // Replace with your actual brand URL for testing | |
| private readonly IJfwApiClient _apiClients; | |
| public RolesTests() | |
| { | |
| var baseUrl = "https://[subdomain].jframework.io"; // integration target | |
| var httpClient = new HttpClient(); | |
| httpClient.BaseAddress = new Uri(baseUrl); | |
| var connection = new ManagementConnection(httpClient); | |
| _apiClients = new JfwApiClient(BrandUrl, connection); | |
| } |
🤖 Prompt for AI Agents
JFW.Sdk.Tests/RolesTests.cs lines 9-23: tests construct a real
HttpClient/ManagementConnection against placeholder domains which makes them
integration tests and will fail in CI; replace this setup by mocking
IManagementConnection (or injecting a fake HttpClientFactory) so the
JfwApiClient is constructed with a mocked connection that returns canned
responses and allows asserting the request URL and behavior, or as a quick
alternative mark these tests with a skip/Ignore attribute until real brand/base
URLs are provided; ensure the mock verifies expected endpoint composition for
both success and not-found scenarios.
| @@ -0,0 +1,98 @@ | |||
|
|
|||
| using JFW.Sdk.Models; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Missing usings likely to break compilation.
IJfwApiClient, JfwApiClient, ManagementConnection, and Fact/Assert come from other namespaces. If you don’t already have global usings, add these.
Apply:
using JFW.Sdk.Models;
+using JFW.Sdk;
+using JFW.Sdk.Clients.Interfaces;
+using Xunit;Run to confirm globals exist before applying:
🏁 Script executed:
#!/bin/bash
rg -nP '^\s*global\s+using\s+JFW\.Sdk\b' -g 'JFW.Sdk.Tests/**'
rg -nP '^\s*global\s+using\s+Xunit\b' -g 'JFW.Sdk.Tests/**'Length of output: 45
Add missing usings: JFW.Sdk & JFW.Sdk.Clients.Interfaces (Xunit already global)
JFW.Sdk.Tests/Usings.cs contains global using Xunit; — add using JFW.Sdk; and using JFW.Sdk.Clients.Interfaces; at the top of JFW.Sdk.Tests/UsersTest.cs so IJfwApiClient, JfwApiClient and ManagementConnection resolve.
🤖 Prompt for AI Agents
In JFW.Sdk.Tests/UsersTest.cs around line 2, the file only has "using
JFW.Sdk.Models;" causing IJfwApiClient, JfwApiClient and ManagementConnection to
be unresolved; add "using JFW.Sdk;" and "using JFW.Sdk.Clients.Interfaces;" at
the top of this file (Xunit is already provided globally) so those types
resolve.
| /// <summary> | ||
| /// The unique identifier of the authenticated user | ||
| /// </summary> | ||
| public string Id { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// The username of the authenticated user | ||
| /// </summary> | ||
| public string Username { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// The email address of the authenticated user | ||
| /// </summary> | ||
| public string EmailAddress { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// The authentication key for subsequent requests | ||
| /// </summary> | ||
| public string AuthKey { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// The expiration time of the authentication token | ||
| /// </summary> | ||
| public DateTime ExpiresAt { get; set; } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix nullability for non-null properties; consider DateTimeOffset
Non-nullable strings lack defaults and will trigger warnings at minimum. Use null-forgiving or ‘required’. Also prefer DateTimeOffset for expirations.
- public string Id { get; set; }
+ public string Id { get; set; } = null!;
@@
- public string Username { get; set; }
+ public string Username { get; set; } = null!;
@@
- public string EmailAddress { get; set; }
+ public string EmailAddress { get; set; } = null!;
@@
- public string AuthKey { get; set; }
+ public string AuthKey { get; set; } = null!;
@@
- public DateTime ExpiresAt { get; set; }
+ public DateTimeOffset ExpiresAt { get; set; }If the API returns a plain ISO-8601 without offset, keep DateTime.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// <summary> | |
| /// The unique identifier of the authenticated user | |
| /// </summary> | |
| public string Id { get; set; } | |
| /// <summary> | |
| /// The username of the authenticated user | |
| /// </summary> | |
| public string Username { get; set; } | |
| /// <summary> | |
| /// The email address of the authenticated user | |
| /// </summary> | |
| public string EmailAddress { get; set; } | |
| /// <summary> | |
| /// The authentication key for subsequent requests | |
| /// </summary> | |
| public string AuthKey { get; set; } | |
| /// <summary> | |
| /// The expiration time of the authentication token | |
| /// </summary> | |
| public DateTime ExpiresAt { get; set; } | |
| } | |
| /// <summary> | |
| /// The unique identifier of the authenticated user | |
| /// </summary> | |
| public string Id { get; set; } = null!; | |
| /// <summary> | |
| /// The username of the authenticated user | |
| /// </summary> | |
| public string Username { get; set; } = null!; | |
| /// <summary> | |
| /// The email address of the authenticated user | |
| /// </summary> | |
| public string EmailAddress { get; set; } = null!; | |
| /// <summary> | |
| /// The authentication key for subsequent requests | |
| /// </summary> | |
| public string AuthKey { get; set; } = null!; | |
| /// <summary> | |
| /// The expiration time of the authentication token | |
| /// </summary> | |
| public DateTimeOffset ExpiresAt { get; set; } | |
| } |
| /// <summary> | ||
| /// Build to dictionary | ||
| /// </summary> | ||
| /// <returns></returns> | ||
| public IDictionary<string, string> ToDictionary() | ||
| { | ||
| var dictionary = new Dictionary<string, string>(); | ||
|
|
||
| if (Ids != null && Ids.Any()) | ||
| dictionary.Add("ids", string.Join(",", Ids)); | ||
|
|
||
| if (!string.IsNullOrEmpty(RoleId)) | ||
| dictionary.Add("roleId", RoleId); | ||
|
|
||
| if (!string.IsNullOrEmpty(Code)) | ||
| dictionary.Add("code", Code); | ||
|
|
||
| if (!string.IsNullOrEmpty(FirstName)) | ||
| dictionary.Add("firstName", FirstName); | ||
|
|
||
| if (!string.IsNullOrEmpty(LastName)) | ||
| dictionary.Add("lastName", LastName); | ||
|
|
||
| if (!string.IsNullOrEmpty(NickName)) | ||
| dictionary.Add("nickName", NickName); | ||
|
|
||
| return dictionary; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include pagination parameters in the query dictionary.
GetUsersRequest inherits BasePagination but ToDictionary() does not emit pageNumber/pageSize. This will ignore caller-specified pagination and rely on server defaults.
Apply near the return:
if (!string.IsNullOrEmpty(NickName))
dictionary.Add("nickName", NickName);
+ // Pagination (only when non-defaults)
+ if (PageNumber != BasePagination.PageNumberDefault)
+ dictionary.Add("pageNumber", PageNumber.ToString(System.Globalization.CultureInfo.InvariantCulture));
+ if (PageSize != BasePagination.PageSizeDefault)
+ dictionary.Add("pageSize", PageSize.ToString(System.Globalization.CultureInfo.InvariantCulture));
+
return dictionary;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// <summary> | |
| /// Build to dictionary | |
| /// </summary> | |
| /// <returns></returns> | |
| public IDictionary<string, string> ToDictionary() | |
| { | |
| var dictionary = new Dictionary<string, string>(); | |
| if (Ids != null && Ids.Any()) | |
| dictionary.Add("ids", string.Join(",", Ids)); | |
| if (!string.IsNullOrEmpty(RoleId)) | |
| dictionary.Add("roleId", RoleId); | |
| if (!string.IsNullOrEmpty(Code)) | |
| dictionary.Add("code", Code); | |
| if (!string.IsNullOrEmpty(FirstName)) | |
| dictionary.Add("firstName", FirstName); | |
| if (!string.IsNullOrEmpty(LastName)) | |
| dictionary.Add("lastName", LastName); | |
| if (!string.IsNullOrEmpty(NickName)) | |
| dictionary.Add("nickName", NickName); | |
| return dictionary; | |
| } | |
| /// <summary> | |
| /// Build to dictionary | |
| /// </summary> | |
| /// <returns></returns> | |
| public IDictionary<string, string> ToDictionary() | |
| { | |
| var dictionary = new Dictionary<string, string>(); | |
| if (Ids != null && Ids.Any()) | |
| dictionary.Add("ids", string.Join(",", Ids)); | |
| if (!string.IsNullOrEmpty(RoleId)) | |
| dictionary.Add("roleId", RoleId); | |
| if (!string.IsNullOrEmpty(Code)) | |
| dictionary.Add("code", Code); | |
| if (!string.IsNullOrEmpty(FirstName)) | |
| dictionary.Add("firstName", FirstName); | |
| if (!string.IsNullOrEmpty(LastName)) | |
| dictionary.Add("lastName", LastName); | |
| if (!string.IsNullOrEmpty(NickName)) | |
| dictionary.Add("nickName", NickName); | |
| // Pagination (only when non-defaults) | |
| if (PageNumber != BasePagination.PageNumberDefault) | |
| dictionary.Add("pageNumber", PageNumber.ToString(System.Globalization.CultureInfo.InvariantCulture)); | |
| if (PageSize != BasePagination.PageSizeDefault) | |
| dictionary.Add("pageSize", PageSize.ToString(System.Globalization.CultureInfo.InvariantCulture)); | |
| return dictionary; | |
| } |
🤖 Prompt for AI Agents
In JFW.Sdk/Models/Users/GetUsersRequest.cs around lines 39 to 66, ToDictionary()
currently omits pagination parameters inherited from BasePagination; add entries
for pageNumber and pageSize before the return so caller-specified pagination is
sent. Check the pagination properties (e.g., PageNumber and PageSize or their
nullable equivalents) and if they have values (or are > 0) add
dictionary.Add("pageNumber", PageNumber.ToString()) and
dictionary.Add("pageSize", PageSize.ToString()) using the same casing expected
by the API.
| The Okta software accompanied by this notice has build dependencies on certain | ||
| third party software licensed under separate terms ("Third Party Components") | ||
| located in THIRD_PARTY_NOTICES. | ||
|
|
There was a problem hiding this comment.
Remove unrelated Okta reference block.
This appears to be a leftover and can confuse consumers.
Apply this diff:
-The Okta software accompanied by this notice has build dependencies on certain
-third party software licensed under separate terms ("Third Party Components")
-located in THIRD_PARTY_NOTICES.
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The Okta software accompanied by this notice has build dependencies on certain | |
| third party software licensed under separate terms ("Third Party Components") | |
| located in THIRD_PARTY_NOTICES. |
🤖 Prompt for AI Agents
In LICENSE around lines 14 to 17, remove the leftover Okta reference block that
mentions Okta and Third Party Components; it is unrelated and confusing to
consumers. Edit the LICENSE file to delete those lines (the entire Okta/Third
Party Components paragraph) and ensure surrounding text remains grammatically
correct and that line breaks are adjusted so the license reads continuously
without the removed block.
| The SDK is compatible with: | ||
| - .NET Standard 2.0 and 2.1 | ||
| - .NET Framework 6.0 or higher | ||
| - .NET Core 3.0 or higher | ||
| - .NET 6.0 |
There was a problem hiding this comment.
Correct framework support to match the project (targets net6.0).
The list mentions “.NET Framework 6.0” (doesn’t exist) and broader TFMs not present in the csproj. Align docs with reality.
Apply this diff:
- The SDK is compatible with:
-- .NET Standard 2.0 and 2.1
-- .NET Framework 6.0 or higher
-- .NET Core 3.0 or higher
-- .NET 6.0
+ The SDK currently targets .NET 6.0 (net6.0).
+ Multi-targeting (e.g., net7.0/net8.0/netstandard2.1) may be added in future releases.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The SDK is compatible with: | |
| - .NET Standard 2.0 and 2.1 | |
| - .NET Framework 6.0 or higher | |
| - .NET Core 3.0 or higher | |
| - .NET 6.0 | |
| The SDK currently targets .NET 6.0 (net6.0). | |
| Multi-targeting (e.g., net7.0/net8.0/netstandard2.1) may be added in future releases. |
🤖 Prompt for AI Agents
In README.md around lines 5 to 9 the supported frameworks list is incorrect (it
mentions .NET Standard, .NET Framework 6.0 which does not exist, and other TFMs
not present in the project); replace that list so it matches the project TFM(s)
in the .csproj (e.g., state only ".NET 6.0 (net6.0)" or the exact TFMs found in
the csproj), removing ".NET Framework 6.0", ".NET Standard 2.0 and 2.1", and
".NET Core 3.0 or higher".
| 1. Right-click on your project in the Solution Explorer and choose Manage Nuget Packages... | ||
| 2. Search for Okta. Install the `Jfw.Sdk` package. |
There was a problem hiding this comment.
Fix package manager instructions (wrong vendor) and casing.
Replace “Okta” with “Jfw.Sdk” and use “NuGet”.
Apply this diff:
-### Install using Nuget Package Manager
+### Install using NuGet Package Manager
1. Right-click on your project in the Solution Explorer and choose Manage Nuget Packages...
-2. Search for Okta. Install the `Jfw.Sdk` package.
+2. Search for "Jfw.Sdk" and install the `Jfw.Sdk` package.
+
+Or via CLI:
+
+```bash
+dotnet add package Jfw.Sdk
+```🤖 Prompt for AI Agents
In README.md around lines 12-13, replace the vendor name "Okta" with "Jfw.Sdk"
and correct the casing of "Nuget" to "NuGet"; then add a bash code block
immediately after the two numbered steps containing the command dotnet add
package Jfw.Sdk so users can install the package via CLI.
| /// <summary> | ||
| /// The error code associated with the exception | ||
| /// </summary> | ||
| public string ErrorCode { get; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix potential NRT warning: ErrorCode may be uninitialized.
Under enable, this non-nullable auto-property isn’t assigned in all constructors.
Apply:
- public string ErrorCode { get; }
+ public string ErrorCode { get; } = string.Empty;Optionally, set a default in the message-only constructors (e.g., "UNKNOWN_ERROR") if having a non-empty code helps downstream logic.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public string ErrorCode { get; } | |
| public string ErrorCode { get; } = string.Empty; |
🤖 Prompt for AI Agents
In JFW.Sdk/Exceptions/JfwException.cs around line 13, the non-nullable
auto-property ErrorCode may be uninitialized under Nullable enable; update the
class so ErrorCode is always initialized: either make it nullable (string?) or
assign a default value on declaration, and ensure every constructor sets
ErrorCode (for message-only constructors set a sensible default like
"UNKNOWN_ERROR" if callers expect a non-empty code). Ensure all overloads assign
the property to eliminate the NRT warning.
| // Add headers | ||
| if (headers != null) | ||
| { | ||
| foreach (var header in headers) | ||
| { | ||
| if (!request.Headers.TryAddWithoutValidation(header.Key, header.Value)) | ||
| { | ||
| request.Content?.Headers.TryAddWithoutValidation(header.Key, header.Value); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Special-case restricted headers (e.g., User-Agent).
Falling back to request.Content.Headers drops general headers. Handle User-Agent explicitly; otherwise throw on unknown content headers.
Apply:
if (headers != null)
{
foreach (var header in headers)
{
- if (!request.Headers.TryAddWithoutValidation(header.Key, header.Value))
- {
- request.Content?.Headers.TryAddWithoutValidation(header.Key, header.Value);
- }
+ if (header.Key.Equals("User-Agent", StringComparison.OrdinalIgnoreCase))
+ {
+ request.Headers.TryAddWithoutValidation("User-Agent", header.Value);
+ continue;
+ }
+ if (!request.Headers.TryAddWithoutValidation(header.Key, header.Value))
+ request.Content?.Headers.TryAddWithoutValidation(header.Key, header.Value);
}
}🤖 Prompt for AI Agents
In JFW.Sdk/ManagementConnection.cs around lines 103-114, the current header
fallback adds restricted headers (like User-Agent) to request.Content.Headers
which drops general headers and can silently fail; update the logic to
special-case restricted headers by detecting "User-Agent" (case-insensitive) and
set it on request.Headers using the appropriate UserAgent property or TryAdd on
request.Headers, handle other known restricted headers similarly, and if a
header cannot be applied to either request.Headers or request.Content.Headers
throw a clear exception instead of silently ignoring it.
Summary by CodeRabbit