-
Notifications
You must be signed in to change notification settings - Fork 35
fix: remove duplicate argument in related proposals #1792
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @HayenNico - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a test case to prevent regressions of this type.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| JSON.stringify({}), | ||
| JSON.stringify(queryFilter), | ||
| ) | ||
| .proposalsControllerCount(JSON.stringify(queryFilter)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the two parameters are needed as the filter is a second parameter in the proposalsControllerCount and the first parameter that should be provided if you look in the generated sdk is the fields. That's why in this case for related proposals we are filering based on proposalId and parentProposalId but not providing any fields as those are used for text search and in the related proposals text search is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's correct, when checking the SDK code I find the following signatures for proposalControllerCount, which do not include a fields argument:
proposalsControllerCount(filters?: string, observe?: 'body', reportProgress?: boolean, options?: {
httpHeaderAccept?: 'application/json';
context?: HttpContext;
}): Observable<CountApiResponse>;
proposalsControllerCount(filters?: string, observe?: 'response', reportProgress?: boolean, options?: {
httpHeaderAccept?: 'application/json';
context?: HttpContext;
}): Observable<HttpResponse<CountApiResponse>>;
proposalsControllerCount(filters?: string, observe?: 'events', reportProgress?: boolean, options?: {
httpHeaderAccept?: 'application/json';
context?: HttpContext;
}): Observable<HttpEvent<CountApiResponse>>;
I would assume the count function specifically does not need fields since it just determines the number of entries that fit the filter. The compilation error I got when running the current master version is this, which indicates wrong arguments or a missing overload:
Build at: 2025-03-21T09:20:21.423Z - Hash: f729b4f8d95975f5 - Time: 3712ms
Error: src/app/state-management/effects/proposals.effects.ts:285:13 - error TS2769: No overload matches this call.
Overload 1 of 3, '(filters?: string, observe?: "body", reportProgress?: boolean, options?: { httpHeaderAccept?: "application/json"; context?: HttpContext; }): Observable<CountApiResponse>', gave the following error.
Overload 2 of 3, '(filters?: string, observe?: "response", reportProgress?: boolean, options?: { httpHeaderAccept?: "application/json"; context?: HttpContext; }): Observable<HttpResponse<CountApiResponse>>', gave the following error.
Argument of type 'string' is not assignable to parameter of type '"response"'.
Overload 3 of 3, '(filters?: string, observe?: "events", reportProgress?: boolean, options?: { httpHeaderAccept?: "application/json"; context?: HttpContext; }): Observable<HttpEvent<CountApiResponse>>', gave the following error.
Argument of type 'string' is not assignable to parameter of type '"events"'.
285 JSON.stringify(queryFilter),
~~~~~~~~~~~~~~~~~~~~~~~~~~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you done fresh npm install to get the latest sdk? I think you might be looking at some older version before we included this in the count endpoint. Let me know what version of the sdk you are using.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martin-trajanovski is correct in saying that there is the additional argument fields in the count function.
That said, I'm not really sure if we need it, as @HayenNico already said, we just return the count of how many proposal we found given the filter
nitrosx
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to verify the current arguments, and see if fields is really needed
| JSON.stringify({}), | ||
| JSON.stringify(queryFilter), | ||
| ) | ||
| .proposalsControllerCount(JSON.stringify(queryFilter)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martin-trajanovski is correct in saying that there is the additional argument fields in the count function.
That said, I'm not really sure if we need it, as @HayenNico already said, we just return the count of how many proposal we found given the filter
@nitrosx the fields field is not used in the related proposals count but it is used in the proposals count where we use the text search and that is done by fields. I would like to refactor this and make the filters cleaner and more flexible without fields or any additional parameters. |
|
@martin-trajanovski @nitrosx I checked and verified Martin is right, I was using an old version of the SDK which precedes the changes to |
Description
Removes a duplicate argument in src/app/state-management/effects/proposals.effects.ts line 283-285 which caused a compilation error on startup. Reverted back to code as in Release v5.1.0.
Compilation tested in local env, error is resolved.
Fixes:
Summary by Sourcery
Bug Fixes: