Staging #6726
Merged
Staging #6726
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* chore: syncloanspolaris improved * chore: validator added on loans and savings * refactor: refactor code * refactor: refactor code * refactor: refactor code
* chore: syncloanspolaris improved * chore: validator added on loans and savings * refactor: refactor code * refactor: refactor code * refactor: refactor code * feat: savings transaction first commit * feat: savings transaction sync commit2 * feat: income deposit added * refactor: refactor code * refactor: refactor code * refactor: refactor code * refactor: refactor code * chore: create loan with polaris date * feat: loan give transactin added * refactor: refactor code * chore: peyment transaction add * feat: saving & deposit income add * feat: deposit and loans some imrpvement
Reviewer's GuideImplements client portal password history enforcement, introduces user-specific loan schedule querying and year-based schedule filtering in the loans UI/API, simplifies loan research detail fetching, and adds a contract resolver to schedules, along with minor integration and style fixes. Sequence diagram for client portal password change with history enforcementsequenceDiagram
actor User
participant API as ClientPortalAPI
participant Model as ClientPortalUserModel
participant DB as ClientPortalUsersCollection
User->>API: changePassword(_id, currentPassword, newPassword)
API->>Model: changePassword(params)
Model->>DB: findOne({_id})
DB-->>Model: user
Model-->>API: throw Error(User not found) if user null
Model->>Model: checkPassword(user, newPassword)
activate Model
Model->>Model: validate strength (regex)
Model->>Model: for each passwordHistory entry comparePassword(newPassword, oldHash)
Model-->>Model: throw Error(Password already used before) if match
Model->>Model: generatePassword(newPassword) -> hashedPassword
Model->>Model: trim passwordHistory to PASSWORD_HISTORY_LIMIT
Model-->>Model: return {hashedPassword, passwordHistory}
deactivate Model
Model->>Model: comparePassword(currentPassword, user.password)
Model-->>API: throw Error(Incorrect current password) if invalid
Model->>DB: findByIdAndUpdate(user._id, {password: hashedPassword, passwordHistory})
DB-->>Model: updatedUser
Model-->>API: updatedUser
API-->>User: success with updated user
Updated class diagram for client portal user password historyclassDiagram
class IUser {
+String _id
+String email
+String phone
+String password
+String[] passwordHistory
+String resetPasswordToken
+Date resetPasswordExpires
}
class IUserDocument {
<<interface>>
+String _id
+String email
+String phone
+String password
+String[] passwordHistory
}
class IUserModel {
<<interface>>
+checkPassword(user, password) Promise
+generatePassword(password) Promise~String~
+comparePassword(plainPassword, hashedPassword) Promise~Boolean~
+clientPortalResetPassword(params) Promise
+changePassword(params) Promise
+verifyPhoneOrEmail(params) Promise
}
class ClientPortalUserModel {
<<static methods>>
+getSecret() String
+generateToken() Promise
+generatePassword(password) Promise~String~
+comparePassword(plainPassword, hashedPassword) Promise~Boolean~
+checkPassword(user, password) Promise~CheckPasswordResult~
+clientPortalResetPassword(params) Promise
+changePassword(params) Promise
+verifyPhoneOrEmail(params) Promise
}
class CheckPasswordResult {
+String hashedPassword
+String[] passwordHistory
}
class clientPortalUserSchema {
+String password
+String[] passwordHistory
+String resetPasswordToken
+Date resetPasswordExpires
}
class PASSWORD_HISTORY_LIMIT {
+Number value
}
class handleContacts {
+handleContacts(args) Promise
}
IUserDocument --|> IUser
ClientPortalUserModel ..|> IUserModel
ClientPortalUserModel ..> CheckPasswordResult
clientPortalUserSchema ..> IUser
ClientPortalUserModel ..> clientPortalUserSchema
ClientPortalUserModel ..> PASSWORD_HISTORY_LIMIT
handleContacts ..> ClientPortalUserModel
note for ClientPortalUserModel "checkPassword now validates strength, checks passwordHistory, enforces PASSWORD_HISTORY_LIMIT, returns hashedPassword and updated passwordHistory. All password-set flows (reset, change, verify, handleContacts) use this method and persist passwordHistory on the user document."
Updated class diagram for loan schedules GraphQL with contract resolver and userSchedules queryclassDiagram
class LoanSchedule {
+String _id
+String contractId
+LoanContract contract
+String status
+Date payDate
+Number interest
+Number didInterest
}
class LoanContract {
+String _id
+String customerId
}
class ScheduleResolvers {
+interest(schedule) Number
+didInterest(schedule) Number
+contract(schedule, args, context) Promise~LoanContract~
}
class ScheduleQueries {
+scheduleYears(contractId, context) [ScheduleYear]
+schedules(contractId, isFirst, year, context) [LoanSchedule]
+userSchedules(userId, contractIds, context) [LoanSchedule]
}
class ScheduleYear {
+Number year
}
class LoansModels {
+Schedules
+Contracts
}
class sendClientPortalMessage {
+sendClientPortalMessage(args) Promise
}
LoanSchedule --> LoanContract : contract
ScheduleResolvers ..> LoanSchedule
ScheduleResolvers ..> LoansModels
ScheduleQueries ..> LoansModels
ScheduleQueries ..> sendClientPortalMessage
note for ScheduleResolvers "New field LoanSchedule.contract is resolved by looking up the related LoanContract via schedule.contractId."
note for ScheduleQueries "New userSchedules query resolves the calling user via client portal service, fetches that user's Contracts, then returns Schedules for those contractIds."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
useHasDetailthe new query flow changes behavior:loanResearchDetailnow always runs with{ dealId, customerId }and theskip: !dealId && !customerIdcondition means it will fire even beforecustomerIdis known (when onlydealIdis present); consider restoring the original two-step behavior (deal-first, then customer fallback) or tightening the skip logic so the backend is not called with partially populated variables. userSchedulescurrently returns schedules without any sort; since other schedule queries sort bypayDate(and sometimescreatedAt), you may want to add a similar sort to keep ordering consistent for consumers.- In the loans UI,
SchedulesListstill accepts acurrentYearprop that is no longer used now thattoggleCurrentYearcontrols filtering and is passed down toScheduleRow; consider removing this unused prop to avoid confusion and keep the interface minimal.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `useHasDetail` the new query flow changes behavior: `loanResearchDetail` now always runs with `{ dealId, customerId }` and the `skip: !dealId && !customerId` condition means it will fire even before `customerId` is known (when only `dealId` is present); consider restoring the original two-step behavior (deal-first, then customer fallback) or tightening the skip logic so the backend is not called with partially populated variables.
- `userSchedules` currently returns schedules without any sort; since other schedule queries sort by `payDate` (and sometimes `createdAt`), you may want to add a similar sort to keep ordering consistent for consumers.
- In the loans UI, `SchedulesList` still accepts a `currentYear` prop that is no longer used now that `toggleCurrentYear` controls filtering and is passed down to `ScheduleRow`; consider removing this unused prop to avoid confusion and keep the interface minimal.
## Individual Comments
### Comment 1
<location> `packages/plugin-loansresearch-ui/src/utils.tsx:35-11` </location>
<code_context>
const customerId = customerData?.customers?.[0]?._id;
- // Third query: If no deal details, fetch customer loan research
- const { data: customerLoanData, loading: customerLoanLoading } =
+ const { data: dealData, loading: dealLoading } =
useQuery<DetailQueryResponse>(gql(queries.loanResearchDetail), {
- variables: { customerId },
- skip: !!loansResearch || !customerId, // Skip if loansResearch exists or no customerId
</code_context>
<issue_to_address>
**issue (bug_risk):** The refactor drops the fallback to customer-based loan research, which changes the hook’s behavior.
The new implementation always queries `loanResearchDetail` by deal and never falls back to customer-level data, so in cases where only customer-based research exists, `loansResearch` will now be `undefined`. If preserving prior behavior is required, consider reintroducing a customer-based query or updating the resolver to accept both `dealId` and `customerId` while maintaining the previous fallback semantics.
</issue_to_address>
### Comment 2
<location> `packages/plugin-loansresearch-ui/src/utils.tsx:11-20` </location>
<code_context>
- // Third query: If no deal details, fetch customer loan research
- const { data: customerLoanData, loading: customerLoanLoading } =
+ const { data: dealData, loading: dealLoading } =
useQuery<DetailQueryResponse>(gql(queries.loanResearchDetail), {
- variables: { customerId },
- skip: !!loansResearch || !customerId, // Skip if loansResearch exists or no customerId
+ variables: { dealId, customerId },
+ skip: !dealId && !customerId, // Ensures hook order remains consistent
});
</code_context>
<issue_to_address>
**question (bug_risk):** The `loanResearchDetail` query is now called with both `dealId` and `customerId`, which may not match the server-side expectations.
Previously, `loanResearchDetail` was called either with `{ dealId }` or, when needed, with `{ customerId }`. The new call always passes both, which changes the contract and assumes the resolver supports and correctly combines both arguments. If the backend expects exactly one identifier, this could change behavior or return incorrect results. Please either keep the one-identifier-at-a-time pattern or confirm and update the resolver to handle both safely.
</issue_to_address>
### Comment 3
<location> `packages/plugin-loans-ui/src/contracts/components/schedules/SchedulesList.tsx:21-23` </location>
<code_context>
const SchedulesList = (props: IProps) => {
- const { schedules, loading, leaseType, scheduleYears, onClickYear } = props;
+ const { schedules, loading, leaseType, scheduleYears, currentYear } = props;
+
+ const [toggleCurrentYear, setToggleCurrentYear] = useState<number | null>(
+ null
+ );
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `currentYear` prop is no longer used to control the selected year, which may break existing expectations.
The selection state is now fully internal and starts at `null`, so `currentYear` is effectively ignored. This removes the ability for callers to control or react to the active year. If this prop is intended to be controlled (e.g. in `ContractSection`), either initialize and sync `toggleCurrentYear` from `currentYear`, or remove the prop to avoid misleading API surface.
Suggested implementation:
```typescript
const [toggleCurrentYear, setToggleCurrentYear] = useState<number | null>(
currentYear ?? null
);
useEffect(() => {
setToggleCurrentYear(currentYear ?? null);
}, [currentYear]);
const renderYear = () => {
```
1. Ensure `useEffect` is imported from React at the top of the file. For example, if you currently have:
`import React, { useState } from "react";`
change it to:
`import React, { useState, useEffect } from "react";`
2. If `currentYear` can never be `null`/`undefined`, you can simplify the initialization and effect to use `currentYear` directly instead of `currentYear ?? null`.
</issue_to_address>
### Comment 4
<location> `packages/plugin-loans-ui/src/contracts/components/schedules/ScheduleRow.tsx:14-17` </location>
<code_context>
- const renderIcon = status => {
- if (status === 'done') {
- return <Icon icon={'medal'} color={'orange'} />;
+function ScheduleRow({ schedule, leaseType, currentYear }: Props) {
+ const { payDate } = schedule || {};
+
+ if (currentYear && payDate && dayjs(payDate).year() !== currentYear) {
+ return null;
+ }
</code_context>
<issue_to_address>
**suggestion:** Returning `null` based on `currentYear` overloads row responsibility with filtering logic.
Since `SchedulesList` already owns the year selection state, it would be better to apply the year filter there and keep `ScheduleRow` as a purely presentational component responsible only for rendering a single row. This improves separation of concerns and makes its render behavior easier to understand.
Suggested implementation:
```typescript
type Props = {
schedule: ISchedule;
leaseType?: string;
};
```
```typescript
function ScheduleRow({ schedule, leaseType }: Props) {
const { payDate } = schedule || {};
```
To fully apply the separation of concerns you described, you’ll also need to:
1. Update `SchedulesList` (or whatever parent component renders `ScheduleRow`) to:
- Stop passing `currentYear` to `ScheduleRow`.
- Apply the `currentYear` filter when computing the list of schedules, e.g.:
`schedules.filter(s => !currentYear || (s.payDate && dayjs(s.payDate).year() === currentYear))`.
2. Update any other usages of `ScheduleRow` that still pass `currentYear` so they only pass `schedule` and `leaseType`.
</issue_to_address>
### Comment 5
<location> `packages/plugin-clientportal-api/src/models/utils.ts:65-69` </location>
<code_context>
- throw new Error('Password is required');
- }
-
- if (password) {
- this.checkPassword(password);
+ throw new Error("Password is required");
</code_context>
<issue_to_address>
**issue:** `checkPassword` is called with a possibly undefined user, which conflicts with its declared type.
In the `!user` branch, `user` is definitely `undefined`, but `checkPassword` is typed as `checkPassword(user: IUserDocument, password: string)`. The implementation tolerates a falsy user via `user || {}`, but the signature doesn’t. Please update the type to accept `IUserDocument | null | undefined` (or similar) and adjust call sites to match the actual usage.
</issue_to_address>
### Comment 6
<location> `packages/plugin-loans-api/src/graphql/resolvers/queries/schedules.ts:43-52` </location>
<code_context>
+ userSchedules: async (
</code_context>
<issue_to_address>
**suggestion (bug_risk):** `userSchedules` does not enforce any ordering on the returned schedules.
Other schedule queries (`schedules`, `scheduleYears`) explicitly sort by `payDate` (and sometimes `createdAt`), but `userSchedules` calls `models.Schedules.find(filter)` with no sort, leaving order up to the datastore. For consistent client behavior (UI, exports), consider adding an explicit sort, e.g. `.sort({ payDate: 1, createdAt: 1 })`.
Suggested implementation:
```typescript
return models.Schedules.find(filter).sort({ payDate: 1, createdAt: 1 });
```
I only see part of the `userSchedules` implementation. If the `find` call in `userSchedules` uses a different variable name than `filter` (e.g. `query`, `selector`, etc.), you should update the search/replace to match that exact line and append `.sort({ payDate: 1, createdAt: 1 })` there. The key is: in every code path where `userSchedules` returns `models.Schedules.find(...)`, ensure that call is followed by `.sort({ payDate: 1, createdAt: 1 })`.
</issue_to_address>
### Comment 7
<location> `packages/plugin-clientportal-api/src/models/ClientPortalUser.ts:508` </location>
<code_context>
public static async changePassword({
</code_context>
<issue_to_address>
**🚨 suggestion (security):** The new password is validated against history before verifying the current password, which may not be the intended flow.
In `changePassword`, `checkPassword(user, newPassword)` runs before verifying `currentPassword`. This allows a “Password already used before” error to be shown even when the current password is wrong, and can leak password-history information for invalid sessions. If the intended behavior is to first validate the current password, consider swapping the order so `currentPassword` is checked before calling `checkPassword` on the new password.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| export const useHasDetail = (dealId: string) => { | ||
| // First query: Check if deal has details | ||
| const { data: dealData, loading: dealLoading } = | ||
| useQuery<DetailQueryResponse>(gql(queries.loanResearchDetail), { |
There was a problem hiding this comment.
issue (bug_risk): The refactor drops the fallback to customer-based loan research, which changes the hook’s behavior.
The new implementation always queries loanResearchDetail by deal and never falls back to customer-level data, so in cases where only customer-based research exists, loansResearch will now be undefined. If preserving prior behavior is required, consider reintroducing a customer-based query or updating the resolver to accept both dealId and customerId while maintaining the previous fallback semantics.
Comment on lines
+14
to
+17
| function ScheduleRow({ schedule, leaseType, currentYear }: Props) { | ||
| const { payDate } = schedule || {}; | ||
|
|
||
| if (currentYear && payDate && dayjs(payDate).year() !== currentYear) { |
There was a problem hiding this comment.
suggestion: Returning null based on currentYear overloads row responsibility with filtering logic.
Since SchedulesList already owns the year selection state, it would be better to apply the year filter there and keep ScheduleRow as a purely presentational component responsible only for rendering a single row. This improves separation of concerns and makes its render behavior easier to understand.
Suggested implementation:
type Props = {
schedule: ISchedule;
leaseType?: string;
};function ScheduleRow({ schedule, leaseType }: Props) {
const { payDate } = schedule || {};To fully apply the separation of concerns you described, you’ll also need to:
- Update
SchedulesList(or whatever parent component rendersScheduleRow) to:- Stop passing
currentYeartoScheduleRow. - Apply the
currentYearfilter when computing the list of schedules, e.g.:
schedules.filter(s => !currentYear || (s.payDate && dayjs(s.payDate).year() === currentYear)).
- Stop passing
- Update any other usages of
ScheduleRowthat still passcurrentYearso they only passscheduleandleaseType.
Comment on lines
+43
to
+52
| userSchedules: async ( | ||
| _root, | ||
| params: { userId: string; contractIds?: string[] }, | ||
| { models, subdomain }: IContext | ||
| ) => { | ||
| const cpUser = await sendClientPortalMessage({ | ||
| subdomain, | ||
| action: "clientPortalUsers.findOne", | ||
| data: { | ||
| $or: [{ _id: params.userId }, { erxesCustomerId: params.userId }], |
There was a problem hiding this comment.
suggestion (bug_risk): userSchedules does not enforce any ordering on the returned schedules.
Other schedule queries (schedules, scheduleYears) explicitly sort by payDate (and sometimes createdAt), but userSchedules calls models.Schedules.find(filter) with no sort, leaving order up to the datastore. For consistent client behavior (UI, exports), consider adding an explicit sort, e.g. .sort({ payDate: 1, createdAt: 1 }).
Suggested implementation:
return models.Schedules.find(filter).sort({ payDate: 1, createdAt: 1 });I only see part of the userSchedules implementation. If the find call in userSchedules uses a different variable name than filter (e.g. query, selector, etc.), you should update the search/replace to match that exact line and append .sort({ payDate: 1, createdAt: 1 }) there. The key is: in every code path where userSchedules returns models.Schedules.find(...), ensure that call is followed by .sort({ payDate: 1, createdAt: 1 }).
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 26e5165 in 2 minutes and 35 seconds. Click for details.
- Reviewed
1300lines of code in13files - Skipped
0files when reviewing. - Skipped posting
14draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/plugin-loansresearch-ui/src/utils.tsx:27
- Draft comment:
Review skip condition: The second query uses 'skip: !dealId && !customerId'. This may run even if dealId exists but customerId is undefined. Consider using an OR condition if that’s the intended behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% The comment is speculative - it says "This may run even ifdealIdexists butcustomerIdis undefined. Consider using an OR condition if that's the intended behavior." The key phrase is "if that's the intended behavior" - the reviewer is not certain there's a bug. Looking at the code, the query accepts both dealId and customerId, suggesting it can work with either one. The current logic!dealId && !customerIdmeans "skip only if we have neither dealId nor customerId", which seems reasonable - we want to run the query if we have at least one identifier. The suggested change!dealId || !customerIdwould mean "skip if we're missing either one", which would be overly restrictive. The comment appears to be incorrect or at least speculative without strong evidence. I might be missing context about what the loanResearchDetail query actually requires. Perhaps it needs both parameters, not just one. The comment could be correct if the query fails when only one parameter is provided. However, the fact that the query accepts both parameters as variables and the code explicitly passes both suggests it's designed to handle either/or scenarios. Without seeing the actual query definition, I cannot be certain, but the comment is clearly speculative ("may run", "Consider...if that's the intended behavior") rather than definitively identifying a bug. This violates the rule against speculative comments. The comment is speculative and doesn't provide strong evidence of a bug. It uses phrases like "may run" and "if that's the intended behavior" which indicate uncertainty. The current logic appears intentional - allowing the query to run if at least one identifier is available. This should be deleted.
2. packages/plugin-sales-api/src/graphql/resolvers/mutations/utils.ts:69
- Draft comment:
Performance suggestion: In the loop iterating over sub-resolvers (lines 69-76), consider running asynchronous calls in parallel (e.g. using Promise.all) to improve performance. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. packages/plugin-loans-api/src/messageBroker.ts:315
- Draft comment:
Minor improvement: When catching errors in sendSms, rethrowing with 'new Error(e.message)' loses stack trace. Consider preserving the original error (or use proper error wrapping). - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. packages/plugin-clientportal-api/src/constants.ts:3
- Draft comment:
Typo/Inconsistency: In the CLIENTPORTALUSER_BASIC_INFO object, some labels (e.g. for 'lastName') use inconsistent capitalization compared to others. Consider standardizing labels. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
5. packages/plugin-loans-ui/src/contracts/components/schedules/ScheduleRow.tsx:39
- Draft comment:
Minor: The helper function 'renderCell' does not type its parameters ('name' and 'didName'). Consider adding explicit types for clarity and maintainability. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
6. packages/plugin-clientportal-api/src/models/ClientPortalUser.ts:18
- Draft comment:
Typographical error: The interface name "IInvitiation" may be a typo. Consider renaming it to "IInvitation". - Reason this comment was not posted:
Comment was on unchanged code.
7. packages/plugin-clientportal-api/src/models/ClientPortalUser.ts:19
- Draft comment:
Typographical error: "INotifcationSettings" appears to be misspelled. It might be intended to be "INotificationSettings". - Reason this comment was not posted:
Comment was on unchanged code.
8. packages/plugin-clientportal-api/src/models/ClientPortalUser.ts:454
- Draft comment:
Typo in the error message: Instead of "Must contain at least one number and one uppercase and lowercase letter, and at least 8 or more characters", it should read something like "Must contain at least one number, one uppercase letter, one lowercase letter, and be at least 8 characters long". - Reason this comment was not posted:
Comment was on unchanged code.
9. packages/plugin-clientportal-api/src/models/ClientPortalUser.ts:465
- Draft comment:
Consider revising the error message "Password already used before" to "Password has already been used" for clearer grammar. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a minor grammar/style suggestion for an error message. While the suggested wording is slightly better grammatically, this falls under "code quality refactors" territory. The rules state that "Comments that suggest code quality refactors are good! But only if they are actionable and clear." This suggestion is actionable and clear. However, I need to consider if this is "clearly a code change required" or if it's just a minor stylistic preference. The current error message is understandable and functional, even if not perfectly grammatical. The rules also say "Do NOT comment unless there is clearly a code change required" and "Do NOT make comments that are obvious or unimportant." This seems like it falls into the "unimportant" category - it's a very minor wording change that doesn't affect functionality or clarity significantly. The suggested change does improve grammar slightly, and error messages should be clear and professional. This could be considered a legitimate quality improvement rather than just a trivial change. The message will be shown to end users, so clarity and proper grammar matter. While user-facing messages should be clear, the current message "Password already used before" is perfectly understandable, even if not grammatically perfect. This is a very minor stylistic preference that doesn't meaningfully improve clarity or fix any real issue. The rules emphasize not making "obvious or unimportant" comments, and this falls into that category. This comment suggests a minor grammar improvement to an error message. While the suggestion is slightly better, the current message is clear and functional. This is an unimportant stylistic change that doesn't meet the threshold of "clearly a code change required." The comment should be deleted.
10. packages/plugin-clientportal-api/src/models/ClientPortalUser.ts:714
- Draft comment:
Typo: The parameter name 'codeLenth' in generateVerificationCode appears to be a typo. It should likely be 'codeLength' to match its usage in subsequent code. - Reason this comment was not posted:
Comment was on unchanged code.
11. packages/plugin-clientportal-api/src/models/ClientPortalUser.ts:1195
- Draft comment:
Typographical suggestion: Consider changing the error message from "Can not create user" to "Cannot create user" for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
12. packages/plugin-clientportal-api/src/models/ClientPortalUser.ts:1241
- Draft comment:
Typographical note: The error message reads "Can not create user". Consider changing it to "Cannot create user" for proper wording. - Reason this comment was not posted:
Comment was on unchanged code.
13. packages/plugin-loans-api/src/graphql/schema/schedule.ts:50
- Draft comment:
Typo: There is an extraneous comma afterleaseAmount: Floatin the function parameters. Please remove it to ensure correct syntax. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. packages/plugin-sales-api/src/graphql/resolvers/mutations/utils.ts:972
- Draft comment:
Typographical note: The string "loyalties" may be a misspelling. If the intended feature flag is related to loyalty (or a similar term), consider checking if it should be "loyalty" instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is speculative ("may be a misspelling") and asks the author to check if it should be "loyalty" instead. Looking at the file context, I see multiple references to "loyalties" in function names likesendLoyaltiesMessage,confirmLoyalties,refundScoreCampaignwhich deals with loyalty scores, etc. This strongly suggests "loyalties" is the correct and intentional service name. The comment violates the rule about not making speculative comments and asking the author to confirm/check things. There's no strong evidence this is actually wrong. Could "loyalties" be a legitimate plural form referring to a service that handles multiple loyalty programs? The presence ofsendLoyaltiesMessagethroughout the file suggests this is intentional naming. The consistent use of "loyalties" throughout the codebase (in function names likesendLoyaltiesMessage,confirmLoyalties, and message actions like "refundLoyaltyScore") strongly indicates this is intentional naming, not a typo. The comment is purely speculative without evidence. This comment should be deleted. It's speculative, asks the author to check/verify something, and there's strong evidence in the file that "loyalties" is the correct and intentional service name used consistently throughout the codebase.
Workflow ID: wflow_ySY5wnN2iTSysF9C
You can customize 
| } | ||
|
|
||
| if (password) { | ||
| const { hashedPassword, passwordHistory } = await models.ClientPortalUsers.checkPassword(user, password); |
Contributor
There was a problem hiding this comment.
Bug: checkPassword is called with user which is undefined. In this context no user exists yet, so the function may throw an error (or iterate over an undefined passwordHistory).
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Enforce client portal password history with validation across all password flows, extend loans schedules querying and UI filtering capabilities, and simplify loan research detail fetching while tightening dependencies on optional services.
Bug Fixes:
Enhancements:
Important
Enhance password management, add new GraphQL queries, and improve UI components for better functionality and security.
PASSWORD_HISTORY_LIMITinconstants.tsto limit password reuse.checkPassword()inClientPortalUser.tsto check against password history and enforce complexity.passwordHistoryfield toIUserandIUserDocumentinclientPortalUser.ts.userSchedulesquery inschedules.tsto fetch schedules by user ID.contractresolver inschedule.tsto fetch contract details for a schedule.SchedulesList.tsxto allow toggling schedule years.ScheduleRow.tsxto filter schedules by selected year.onClickYearprop inContractSection.tsx.This description was created by
for 26e5165. You can customize this summary. It will automatically update as commits are pushed.