-
Notifications
You must be signed in to change notification settings - Fork 15
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
fix: Fix tests for tasks page stats changes [2] #2928
Conversation
bistaastha
commented
Apr 30, 2024
•
edited
Loading
edited
- Fixes test for task page service
PR title must start with "fix:" or "feat:" or "test:" |
PR description must contain a link to a clickup |
PR title must start with "fix:" or "feat:" or "test:" |
PR description must contain a link to a clickup |
PR description must contain a link to a clickup |
Your free trial has expired. To continue using Ellipsis, sign up at https://app.ellipsis.dev for $20/developer/month. If you have any questions, reach us at [email protected] |
@bistaastha code coverage is falling below the threshold, will this be fixed in later PRs? |
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.
One comment
@@ -1,17 +1,18 @@ | |||
import { ReportsStatsResponsePlatform } from '../models/platform/v1/report-stats-response.model'; |
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.
Prefix the word Platform
. Reason - We prefix the word in case of file names. So helps in maintaining consistency.
@@ -58,6 +58,15 @@ describe('ApproverReportsService', () => { | |||
}); | |||
}); | |||
|
|||
it('generateStatsQueryParams(): should generate stats query params', () => { | |||
const queryParams = { | |||
state: 'eq.DRAFT', |
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.
Use enum.
@@ -44,7 +44,7 @@ export class ApproverReportsService { | |||
return this.getReportsByParams(params).pipe(map((res) => res.count)); | |||
} | |||
|
|||
getReportsByParams(queryParams: ReportsQueryParams = {}): Observable<PlatformApiResponse<Report>> { | |||
getReportsByParams(queryParams: ReportsQueryParams): Observable<PlatformApiResponse<Report>> { |
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 have updated the PlatformApiResponse
model recently. If the response data contains an array you will have to pass Report[]
instead of just Report
.
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.
Minor comments.
…obile-app into FYLE-86cuqn6db-tasks-tests
…obile-app into FYLE-86cuqn6db-tasks-tests
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.
One minor question.
|