diff --git a/lib/angular-ui/src/lib/atoms/card/card.component.ts b/lib/angular-ui/src/lib/atoms/card/card.component.ts index 80592610..fa5f7b4f 100644 --- a/lib/angular-ui/src/lib/atoms/card/card.component.ts +++ b/lib/angular-ui/src/lib/atoms/card/card.component.ts @@ -8,8 +8,6 @@ import { DefineCustomElement } from '../../define-custom-element'; selector: 'cg-card', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class CardComponent {} diff --git a/lib/angular-ui/src/lib/atoms/icon-btn/icon-btn.component.ts b/lib/angular-ui/src/lib/atoms/icon-btn/icon-btn.component.ts index e796992f..2f18490e 100644 --- a/lib/angular-ui/src/lib/atoms/icon-btn/icon-btn.component.ts +++ b/lib/angular-ui/src/lib/atoms/icon-btn/icon-btn.component.ts @@ -8,8 +8,6 @@ import { DefineCustomElement } from '../../define-custom-element'; selector: 'cg-icon-btn', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class IconBtnComponent {} diff --git a/lib/angular-ui/src/lib/atoms/icons/check-circle-icon/check-circle-icon.component.ts b/lib/angular-ui/src/lib/atoms/icons/check-circle-icon/check-circle-icon.component.ts index 0d5105a4..be5146c9 100644 --- a/lib/angular-ui/src/lib/atoms/icons/check-circle-icon/check-circle-icon.component.ts +++ b/lib/angular-ui/src/lib/atoms/icons/check-circle-icon/check-circle-icon.component.ts @@ -8,8 +8,6 @@ import { DefineCustomElement } from '../../../define-custom-element'; selector: 'cg-check-circle-icon', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class CheckCircleIconComponent {} diff --git a/lib/angular-ui/src/lib/atoms/icons/dash-circle-icon/dash-circle-icon.component.ts b/lib/angular-ui/src/lib/atoms/icons/dash-circle-icon/dash-circle-icon.component.ts index 7c5f5ef5..c7bb5135 100644 --- a/lib/angular-ui/src/lib/atoms/icons/dash-circle-icon/dash-circle-icon.component.ts +++ b/lib/angular-ui/src/lib/atoms/icons/dash-circle-icon/dash-circle-icon.component.ts @@ -8,8 +8,6 @@ import { DefineCustomElement } from '../../../define-custom-element'; selector: 'cg-dash-circle-icon', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class DashCircleIconComponent {} diff --git a/lib/angular-ui/src/lib/atoms/icons/profile-icon/profile-icon.component.ts b/lib/angular-ui/src/lib/atoms/icons/profile-icon/profile-icon.component.ts index 04d40b1f..14d7f835 100644 --- a/lib/angular-ui/src/lib/atoms/icons/profile-icon/profile-icon.component.ts +++ b/lib/angular-ui/src/lib/atoms/icons/profile-icon/profile-icon.component.ts @@ -8,8 +8,6 @@ import { DefineCustomElement } from '../../../define-custom-element'; selector: 'cg-profile-icon', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class ProfileIconComponent {} diff --git a/lib/angular-ui/src/lib/atoms/radio-input/radio-input.component.ts b/lib/angular-ui/src/lib/atoms/radio-input/radio-input.component.ts index fc891bfb..85e14708 100644 --- a/lib/angular-ui/src/lib/atoms/radio-input/radio-input.component.ts +++ b/lib/angular-ui/src/lib/atoms/radio-input/radio-input.component.ts @@ -29,9 +29,7 @@ type TouchedFn = () => void; multi: true, }, ], - template: ` - - `, + template: ``, }) export class RadioInputComponent implements ControlValueAccessor { private notifyChange: ChangeFn = () => {}; diff --git a/lib/angular-ui/src/lib/atoms/text-btn/text-btn.component.ts b/lib/angular-ui/src/lib/atoms/text-btn/text-btn.component.ts index 3af01161..abb84cf8 100644 --- a/lib/angular-ui/src/lib/atoms/text-btn/text-btn.component.ts +++ b/lib/angular-ui/src/lib/atoms/text-btn/text-btn.component.ts @@ -8,8 +8,6 @@ import { DefineCustomElement } from '../../define-custom-element'; selector: 'cg-text-btn', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class TextBtnComponent {} diff --git a/lib/angular-ui/src/lib/molecules/collapsible/collapsible.component.ts b/lib/angular-ui/src/lib/molecules/collapsible/collapsible.component.ts index 2f7dff16..a53324ba 100644 --- a/lib/angular-ui/src/lib/molecules/collapsible/collapsible.component.ts +++ b/lib/angular-ui/src/lib/molecules/collapsible/collapsible.component.ts @@ -8,8 +8,6 @@ import { DefineCustomElement } from '../../define-custom-element'; selector: 'cg-collapsible', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class CollapsibleComponent {} diff --git a/lib/angular-ui/src/lib/molecules/copy-to-clipboard/copy-to-clipboard.component.ts b/lib/angular-ui/src/lib/molecules/copy-to-clipboard/copy-to-clipboard.component.ts index 3198ba35..2af3a1eb 100644 --- a/lib/angular-ui/src/lib/molecules/copy-to-clipboard/copy-to-clipboard.component.ts +++ b/lib/angular-ui/src/lib/molecules/copy-to-clipboard/copy-to-clipboard.component.ts @@ -15,9 +15,7 @@ import { DefineCustomElement } from '../../define-custom-element'; selector: 'cg-copy-to-clipboard', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class CopyToClipboardComponent { public readonly value = diff --git a/lib/angular-ui/src/lib/molecules/dropdown/dropdown-btn-menu-item/dropdown-btn-menu-item.component.ts b/lib/angular-ui/src/lib/molecules/dropdown/dropdown-btn-menu-item/dropdown-btn-menu-item.component.ts index 1457ec31..12dd364e 100644 --- a/lib/angular-ui/src/lib/molecules/dropdown/dropdown-btn-menu-item/dropdown-btn-menu-item.component.ts +++ b/lib/angular-ui/src/lib/molecules/dropdown/dropdown-btn-menu-item/dropdown-btn-menu-item.component.ts @@ -8,8 +8,6 @@ import { DefineCustomElement } from '../../../define-custom-element'; selector: 'cg-dropdown-btn-menu-item', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class DropdownBtnMenuItemComponent {} diff --git a/lib/angular-ui/src/lib/molecules/dropdown/dropdown-link-menu-item/dropdown-link-menu-item.component.ts b/lib/angular-ui/src/lib/molecules/dropdown/dropdown-link-menu-item/dropdown-link-menu-item.component.ts index 1c7558e7..6faa1e69 100644 --- a/lib/angular-ui/src/lib/molecules/dropdown/dropdown-link-menu-item/dropdown-link-menu-item.component.ts +++ b/lib/angular-ui/src/lib/molecules/dropdown/dropdown-link-menu-item/dropdown-link-menu-item.component.ts @@ -17,9 +17,7 @@ import { DefineCustomElement } from '../../../define-custom-element'; selector: 'cg-dropdown-link-menu-item', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class DropdownLinkMenuItemComponent { public readonly routerLink = diff --git a/lib/angular-ui/src/lib/molecules/dropdown/dropdown-menu/dropdown-menu.component.ts b/lib/angular-ui/src/lib/molecules/dropdown/dropdown-menu/dropdown-menu.component.ts index e3b0e41d..d71d1f81 100644 --- a/lib/angular-ui/src/lib/molecules/dropdown/dropdown-menu/dropdown-menu.component.ts +++ b/lib/angular-ui/src/lib/molecules/dropdown/dropdown-menu/dropdown-menu.component.ts @@ -8,8 +8,6 @@ import { DefineCustomElement } from '../../../define-custom-element'; selector: 'cg-dropdown-menu', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class DropdownMenuComponent {} diff --git a/lib/angular-ui/src/lib/molecules/dropdown/dropdown-trigger/dropdown-trigger.component.ts b/lib/angular-ui/src/lib/molecules/dropdown/dropdown-trigger/dropdown-trigger.component.ts index ec701ada..5e22b496 100644 --- a/lib/angular-ui/src/lib/molecules/dropdown/dropdown-trigger/dropdown-trigger.component.ts +++ b/lib/angular-ui/src/lib/molecules/dropdown/dropdown-trigger/dropdown-trigger.component.ts @@ -15,9 +15,7 @@ import { DefineCustomElement } from '../../../define-custom-element'; selector: 'cg-dropdown-trigger', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class DropdownTriggerComponent { public readonly isIconBtn = diff --git a/lib/angular-ui/src/lib/molecules/dropdown/dropdown.component.ts b/lib/angular-ui/src/lib/molecules/dropdown/dropdown.component.ts index 67afb68e..34ce8c8e 100644 --- a/lib/angular-ui/src/lib/molecules/dropdown/dropdown.component.ts +++ b/lib/angular-ui/src/lib/molecules/dropdown/dropdown.component.ts @@ -15,9 +15,7 @@ import { DefineCustomElement } from '../../define-custom-element'; selector: 'cg-dropdown', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class DropdownComponent { public readonly anchorAlign = input(); diff --git a/lib/angular-ui/src/lib/molecules/image-uploader-btn/image-uploader-btn.component.ts b/lib/angular-ui/src/lib/molecules/image-uploader-btn/image-uploader-btn.component.ts index 827e6902..74353dfe 100644 --- a/lib/angular-ui/src/lib/molecules/image-uploader-btn/image-uploader-btn.component.ts +++ b/lib/angular-ui/src/lib/molecules/image-uploader-btn/image-uploader-btn.component.ts @@ -19,9 +19,7 @@ type ImagesSelectedEvent = CustomEvent< selector: 'cg-image-uploader-btn', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class ImageUploaderBtnComponent { @HostListener('imagesSelected', ['$event']) diff --git a/lib/angular-ui/src/lib/organisms/footer/footer.component.ts b/lib/angular-ui/src/lib/organisms/footer/footer.component.ts index 1154f29f..880c2b9d 100644 --- a/lib/angular-ui/src/lib/organisms/footer/footer.component.ts +++ b/lib/angular-ui/src/lib/organisms/footer/footer.component.ts @@ -14,9 +14,7 @@ import { defineCustomElement } from '@cg/ui/dist/components/cg-footer'; selector: 'cg-footer', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class FooterComponent { public readonly links = input.required(); diff --git a/lib/angular-ui/src/lib/organisms/navbar/navbar.component.ts b/lib/angular-ui/src/lib/organisms/navbar/navbar.component.ts index 17beed10..7021ef60 100644 --- a/lib/angular-ui/src/lib/organisms/navbar/navbar.component.ts +++ b/lib/angular-ui/src/lib/organisms/navbar/navbar.component.ts @@ -14,9 +14,7 @@ import { selector: 'cg-navbar', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class NavbarComponent { public readonly homeUrl = input.required(); diff --git a/lib/angular-ui/src/lib/organisms/sidenav/sidenav.component.ts b/lib/angular-ui/src/lib/organisms/sidenav/sidenav.component.ts index fd2b1f2c..4f4ddceb 100644 --- a/lib/angular-ui/src/lib/organisms/sidenav/sidenav.component.ts +++ b/lib/angular-ui/src/lib/organisms/sidenav/sidenav.component.ts @@ -17,9 +17,7 @@ export { NavLink, NavLinkCategory } from '@cg/ui/dist/types'; selector: 'cg-sidenav', standalone: true, changeDetection: ChangeDetectionStrategy.OnPush, - template: ` - - `, + template: ``, }) export class SidenavComponent { public readonly homeUrl = input.required(); diff --git a/prettier.config.cjs b/prettier.config.cjs index 536b45ba..f21081d0 100644 --- a/prettier.config.cjs +++ b/prettier.config.cjs @@ -3,7 +3,7 @@ module.exports = { singleQuote: true, trailingComma: 'all', arrowParens: 'avoid', - htmlWhitespaceSensitivity: 'ignore', + htmlWhitespaceSensitivity: 'strict', plugins: ['prettier-plugin-astro'], overrides: [ { diff --git a/src/frontend/src/app/core/api/commit-review/commit-review-api.mapper.ts b/src/frontend/src/app/core/api/commit-review/commit-review-api.mapper.ts index 683c4282..55b777ae 100644 --- a/src/frontend/src/app/core/api/commit-review/commit-review-api.mapper.ts +++ b/src/frontend/src/app/core/api/commit-review/commit-review-api.mapper.ts @@ -59,7 +59,6 @@ export function mapGetProposalReviewCommitResponse( ): GetProposalReviewCommitResponse { return { id: res.id, - listId: res.id, proposalReviewId: res.proposal_review_commit.proposal_review_id, userId: res.proposal_review_commit.user_id, createdAt: fromCandidDate(res.proposal_review_commit.created_at), @@ -74,17 +73,17 @@ export function mapGetProposalReviewCommitResponse( function mapReviewCommitRequestDetails( req: ReviewCommitDetails, ): ReviewCommitState { - if (req.reviewed === false) { - return { not_reviewed: null }; + if (req.reviewed) { + return { + reviewed: { + highlights: req.highlights, + comment: toCandidOpt(req.comment), + matches_description: toCandidOpt(req.matchesDescription), + }, + }; } - return { - reviewed: { - highlights: req.highlights, - comment: toCandidOpt(req.comment), - matches_description: toCandidOpt(req.matchesDescription), - }, - }; + return { not_reviewed: null }; } function mapReviewCommitResponseDetails( diff --git a/src/frontend/src/app/core/api/commit-review/commit-review-api.model.ts b/src/frontend/src/app/core/api/commit-review/commit-review-api.model.ts index 3fdb3ce2..189ef0bd 100644 --- a/src/frontend/src/app/core/api/commit-review/commit-review-api.model.ts +++ b/src/frontend/src/app/core/api/commit-review/commit-review-api.model.ts @@ -14,13 +14,12 @@ export interface DeleteProposalReviewCommitRequest { } export interface GetProposalReviewCommitResponse { - id: string | null; - listId: string; + id: string; proposalReviewId: string; userId: string; createdAt: Date; lastUpdatedAt: Date | null; - commitSha: string; + commitSha: string | null; details: ReviewCommitDetails; } @@ -36,5 +35,5 @@ export interface ReviewedCommitDetails { } export interface NotReviewedCommitDetails { - reviewed: false; + reviewed: false | null; } diff --git a/src/frontend/src/app/core/api/commit-review/commit-review-api.service.spec.ts b/src/frontend/src/app/core/api/commit-review/commit-review-api.service.spec.ts index 76afc8ac..218e145f 100644 --- a/src/frontend/src/app/core/api/commit-review/commit-review-api.service.spec.ts +++ b/src/frontend/src/app/core/api/commit-review/commit-review-api.service.spec.ts @@ -66,7 +66,7 @@ describe('CommitReviewApiService', () => { proposal_review_id: 'proposalReviewId', user_id: 'userId', commit_sha: 'commitSha', - created_at: new Date(2024, 1, 1, 0, 0, 0, 0).getTime().toString(), + created_at: new Date(2024, 1, 1, 0, 0, 0, 0).toISOString(), last_updated_at: [], state: { reviewed: { @@ -80,7 +80,6 @@ describe('CommitReviewApiService', () => { }; const commonResponse: GetProposalReviewCommitResponse = { id: 'id', - listId: 'id', userId: 'userId', commitSha: 'commitSha', proposalReviewId: 'proposalReviewId', diff --git a/src/frontend/src/app/core/api/review/review-api.mapper.ts b/src/frontend/src/app/core/api/review/review-api.mapper.ts index 59f2cf16..96195709 100644 --- a/src/frontend/src/app/core/api/review/review-api.mapper.ts +++ b/src/frontend/src/app/core/api/review/review-api.mapper.ts @@ -14,6 +14,7 @@ import { GetProposalReviewRequest as GetProposalReviewApiRequest, GetMyProposalReviewRequest as GetMyProposalReviewApiRequest, ProposalReviewStatus as ProposalReviewStatusApi, + ProposalVote as ApiProposalVote, } from '@cg/backend'; import { GetProposalReviewResponse, @@ -23,7 +24,6 @@ import { ListProposalReviewsRequest, GetProposalReviewRequest, ProposalReviewStatus, - ProposalReviewVote, } from './review-api.model'; export function mapCreateProposalReviewRequest( @@ -34,7 +34,7 @@ export function mapCreateProposalReviewRequest( review_duration_mins: toCandidOpt(req.reviewDurationMins), summary: toCandidOpt(req.summary), build_reproduced: toCandidOpt(req.buildReproduced), - vote: [], + vote: mapProposalVoteRequest(req.vote), }; } @@ -48,7 +48,7 @@ export function mapUpdateProposalReviewRequest( review_duration_mins: toCandidOpt(req.reviewDurationMins), summary: toCandidOpt(req.summary), build_reproduced: toCandidOpt(req.buildReproduced), - vote: [], + vote: mapProposalVoteRequest(req.vote), }; } @@ -86,8 +86,7 @@ export function mapGetProposalReviewResponse( id: res.id, proposalId: review.proposal_id, userId: review.user_id, - // [TODO] - connect with API once it's implemented - vote: ProposalReviewVote.NoVote, + vote: mapProposalVoteResponse(review.vote), createdAt: fromCandidDate(review.created_at), lastUpdatedAt: fromCandidOptDate(review.last_updated_at), status: mapProposalReviewStatusResponse(review.status), @@ -112,6 +111,32 @@ function mapProposalReviewStatusResponse( return ProposalReviewStatus.Draft; } +function mapProposalVoteRequest(vote?: boolean | null): [] | [ApiProposalVote] { + switch (vote) { + case true: { + return [{ yes: null }]; + } + case false: { + return [{ no: null }]; + } + default: { + return []; + } + } +} + +function mapProposalVoteResponse(vote: ApiProposalVote): boolean | null { + if ('yes' in vote) { + return true; + } + + if ('no' in vote) { + return false; + } + + return null; +} + function getReviewImages(): ImageSet[] { return [ { diff --git a/src/frontend/src/app/core/api/review/review-api.model.ts b/src/frontend/src/app/core/api/review/review-api.model.ts index c51f29d3..734906ab 100644 --- a/src/frontend/src/app/core/api/review/review-api.model.ts +++ b/src/frontend/src/app/core/api/review/review-api.model.ts @@ -3,17 +3,18 @@ import { ImageSet } from '@cg/angular-ui'; export interface CreateProposalReviewRequest { proposalId: string; - summary: string | null; - reviewDurationMins: number | null; - buildReproduced: boolean | null; - reproducedBuildImageId: string | null; + summary?: string | null; + reviewDurationMins?: number | null; + buildReproduced?: boolean | null; + vote?: boolean | null; } export interface UpdateProposalReviewRequest { proposalId: string; - reviewDurationMins: number | null; - summary: string | null; - buildReproduced: boolean | null; + reviewDurationMins?: number | null; + summary?: string | null; + buildReproduced?: boolean | null; + vote?: boolean | null; } export interface ListProposalReviewsRequest { @@ -33,7 +34,7 @@ export interface GetProposalReviewResponse { id: string; proposalId: string; userId: string; - vote: ProposalReviewVote; + vote: boolean | null; createdAt: Date; lastUpdatedAt: Date | null; status: ProposalReviewStatus; @@ -63,7 +64,7 @@ export interface ProposalCommitReviewHighlight { export interface ProposalCommitReviewSummary { proposalId: string; commitId: string; - commitSha: string; + commitSha: string | null; highlights: ProposalCommitReviewHighlight[]; totalReviewers: number; reviewedCount: number; diff --git a/src/frontend/src/app/core/api/review/review-api.service.spec.ts b/src/frontend/src/app/core/api/review/review-api.service.spec.ts index 382b04fe..66b2567d 100644 --- a/src/frontend/src/app/core/api/review/review-api.service.spec.ts +++ b/src/frontend/src/app/core/api/review/review-api.service.spec.ts @@ -26,7 +26,6 @@ import { GetProposalReviewResponse, ListProposalReviewsRequest, ProposalReviewStatus, - ProposalReviewVote, UpdateProposalReviewRequest, } from './review-api.model'; import { ReviewApiService } from './review-api.service'; @@ -57,7 +56,6 @@ describe('ReviewApiService', () => { summary: null, reviewDurationMins: null, buildReproduced: null, - reproducedBuildImageId: null, }; const commonApiRequest: CreateProposalReviewApiRequest = { proposal_id: 'proposalId', @@ -72,7 +70,7 @@ describe('ReviewApiService', () => { proposal_review: { proposal_id: 'proposalId', user_id: 'userId', - created_at: new Date(2024, 1, 1, 0, 0, 0, 0).getTime().toString(), + created_at: new Date(2024, 1, 1, 0, 0, 0, 0).toISOString(), last_updated_at: [], status: { draft: null, @@ -90,7 +88,7 @@ describe('ReviewApiService', () => { id: 'id', proposalId: 'proposalId', userId: 'userId', - vote: ProposalReviewVote.NoVote, + vote: null, createdAt: new Date(2024, 1, 1, 0, 0, 0, 0), lastUpdatedAt: null, status: ProposalReviewStatus.Draft, @@ -195,7 +193,7 @@ describe('ReviewApiService', () => { proposal_review: { proposal_id: 'proposalId', user_id: 'userId1', - created_at: new Date(2024, 1, 1, 0, 0, 0, 0).getTime().toString(), + created_at: new Date(2024, 1, 1, 0, 0, 0, 0).toISOString(), last_updated_at: [], status: { draft: null, @@ -213,7 +211,7 @@ describe('ReviewApiService', () => { proposal_review: { proposal_id: 'proposalId', user_id: 'userId2', - created_at: new Date(2024, 1, 2, 0, 0, 0, 0).getTime().toString(), + created_at: new Date(2024, 1, 2, 0, 0, 0, 0).toISOString(), last_updated_at: [], status: { draft: null, @@ -234,7 +232,7 @@ describe('ReviewApiService', () => { id: 'id1', proposalId: 'proposalId', userId: 'userId1', - vote: ProposalReviewVote.NoVote, + vote: null, createdAt: new Date(2024, 1, 1, 0, 0, 0, 0), lastUpdatedAt: null, status: ProposalReviewStatus.Draft, @@ -249,7 +247,7 @@ describe('ReviewApiService', () => { id: 'id2', proposalId: 'proposalId', userId: 'userId2', - vote: ProposalReviewVote.NoVote, + vote: null, createdAt: new Date(2024, 1, 2, 0, 0, 0, 0), lastUpdatedAt: null, status: ProposalReviewStatus.Draft, @@ -301,7 +299,7 @@ describe('ReviewApiService', () => { proposal_review: { proposal_id: 'proposalId', user_id: 'userId', - created_at: new Date(2024, 1, 1, 0, 0, 0, 0).getTime().toString(), + created_at: new Date(2024, 1, 1, 0, 0, 0, 0).toISOString(), last_updated_at: [], status: { draft: null, @@ -319,7 +317,7 @@ describe('ReviewApiService', () => { id: 'id', proposalId: 'proposalId', userId: 'userId', - vote: ProposalReviewVote.NoVote, + vote: null, createdAt: new Date(2024, 1, 1, 0, 0, 0, 0), lastUpdatedAt: null, status: ProposalReviewStatus.Draft, @@ -372,7 +370,7 @@ describe('ReviewApiService', () => { proposal_review: { proposal_id: 'proposalId', user_id: 'userId', - created_at: new Date(2024, 1, 1, 0, 0, 0, 0).getTime().toString(), + created_at: new Date(2024, 1, 1, 0, 0, 0, 0).toISOString(), last_updated_at: [], status: { draft: null, @@ -390,7 +388,7 @@ describe('ReviewApiService', () => { id: 'id', proposalId: 'proposalId', userId: 'userId', - vote: ProposalReviewVote.NoVote, + vote: null, createdAt: new Date(2024, 1, 1, 0, 0, 0, 0), lastUpdatedAt: null, status: ProposalReviewStatus.Draft, @@ -433,7 +431,6 @@ describe('ReviewApiService', () => { summary: null, reviewDurationMins: null, buildReproduced: null, - reproducedBuildImageId: null, }; const commonApiCreateRequest: CreateProposalReviewApiRequest = { proposal_id: 'proposalId', @@ -451,7 +448,7 @@ describe('ReviewApiService', () => { proposal_review: { proposal_id: 'proposalId', user_id: 'userId', - created_at: new Date(2024, 1, 1, 0, 0, 0, 0).getTime().toString(), + created_at: new Date(2024, 1, 1, 0, 0, 0, 0).toISOString(), last_updated_at: [], status: { draft: null, @@ -469,7 +466,7 @@ describe('ReviewApiService', () => { id: 'id', proposalId: 'proposalId', userId: 'userId', - vote: ProposalReviewVote.NoVote, + vote: null, createdAt: new Date(2024, 1, 1, 0, 0, 0, 0), lastUpdatedAt: null, status: ProposalReviewStatus.Draft, diff --git a/src/frontend/src/app/core/state/index.ts b/src/frontend/src/app/core/state/index.ts index 8d2e8f12..61aae95b 100644 --- a/src/frontend/src/app/core/state/index.ts +++ b/src/frontend/src/app/core/state/index.ts @@ -1,3 +1,4 @@ export * from './profile'; export * from './proposal'; export * from './review'; +export * from './review-submission'; diff --git a/src/frontend/src/app/core/state/review-submission/index.ts b/src/frontend/src/app/core/state/review-submission/index.ts new file mode 100644 index 00000000..918a0a65 --- /dev/null +++ b/src/frontend/src/app/core/state/review-submission/index.ts @@ -0,0 +1 @@ +export * from './review-submission.service'; diff --git a/src/frontend/src/app/core/state/review-submission/review-submission.service.mock.ts b/src/frontend/src/app/core/state/review-submission/review-submission.service.mock.ts new file mode 100644 index 00000000..373c7532 --- /dev/null +++ b/src/frontend/src/app/core/state/review-submission/review-submission.service.mock.ts @@ -0,0 +1,17 @@ +import { ReviewSubmissionService } from './review-submission.service'; + +export type ReviewSubmissionServiceMock = + jasmine.SpyObj; + +export function reviewSubmissionServiceMockFactory(): ReviewSubmissionServiceMock { + return jasmine.createSpyObj( + 'ReviewSubmissionService', + [ + 'addCommit', + 'loadOrCreateReview', + 'removeCommit', + 'updateCommit', + 'updateReview', + ], + ); +} diff --git a/src/frontend/src/app/core/state/review-submission/review-submission.service.spec.ts b/src/frontend/src/app/core/state/review-submission/review-submission.service.spec.ts new file mode 100644 index 00000000..a5ac7fce --- /dev/null +++ b/src/frontend/src/app/core/state/review-submission/review-submission.service.spec.ts @@ -0,0 +1,424 @@ +import { TestBed } from '@angular/core/testing'; + +import { + CommitReviewApiServiceMock, + commitReviewApiServiceMockFactory, +} from '../../api/commit-review/commit-review-api.service.mock'; +import { + ReviewApiServiceMock, + reviewApiServiceMockFactory, +} from '../../api/review/review-api.service.mock'; +import { + CommitReviewApiService, + GetProposalReviewCommitResponse, + GetProposalReviewResponse, + ProposalReviewStatus, + ReviewApiService, +} from '~core/api'; +import { ApiError } from '~core/utils'; +import { + ReviewSubmissionService, + UiProposalReviewCommit, +} from './review-submission.service'; + +describe('ReviewSubmissionService', () => { + let service: ReviewSubmissionService; + + let reviewApiServiceMock: ReviewApiServiceMock; + let commitReviewApiServiceMock: CommitReviewApiServiceMock; + + const currentDate = new Date(); + + const proposalId = 'e4e6247e-fda4-4678-8590-2cf86b876c92'; + const userId = 'a36bc00d-3ead-4dd1-9115-6b68fce1783c'; + const commitSha = '96f4ab3090ad72964319346967f4ce9634df200e'; + + beforeEach(() => { + jasmine.clock().install().mockDate(currentDate); + + reviewApiServiceMock = reviewApiServiceMockFactory(); + commitReviewApiServiceMock = commitReviewApiServiceMockFactory(); + + TestBed.configureTestingModule({ + providers: [ + { provide: ReviewApiService, useValue: reviewApiServiceMock }, + { + provide: CommitReviewApiService, + useValue: commitReviewApiServiceMock, + }, + ], + }); + + service = TestBed.inject(ReviewSubmissionService); + }); + + afterEach(() => { + jasmine.clock().uninstall(); + }); + + it('should be created', () => { + expect(service).toBeTruthy(); + }); + + describe('loadOrCreateReview()', () => { + it('should load or create a review', async () => { + const reviewSpy = jasmine.createSpy(); + service.review$.subscribe(reviewSpy); + + const commitsSpy = jasmine.createSpy(); + service.commits$.subscribe(commitsSpy); + + const proposalReviewResponse = createProposalReviewResponse( + proposalId, + userId, + ); + reviewApiServiceMock.getOrCreateMyProposalReview.and.resolveTo( + proposalReviewResponse, + ); + + await service.loadOrCreateReview(proposalId); + + expect(reviewSpy).toHaveBeenCalledTimes(2); + expect(reviewSpy).toHaveBeenCalledWith(null); + expect(reviewSpy).toHaveBeenCalledWith(proposalReviewResponse); + + expect(commitsSpy).toHaveBeenCalledTimes(2); + expect(commitsSpy).toHaveBeenCalledWith(null); + expect(commitsSpy).toHaveBeenCalledWith([]); + }); + + it('should throw an error if loading or creating the review fails', async () => { + const reviewSpy = jasmine.createSpy(); + service.review$.subscribe(reviewSpy); + + const commitsSpy = jasmine.createSpy(); + service.commits$.subscribe(commitsSpy); + + const error = new ApiError({ + code: 500, + message: 'Internal server error', + }); + + reviewApiServiceMock.getOrCreateMyProposalReview.and.rejectWith(error); + + await expectAsync( + service.loadOrCreateReview(proposalId), + ).toBeRejectedWith(error); + + expect(reviewSpy).toHaveBeenCalledTimes(1); + expect(reviewSpy).toHaveBeenCalledWith(null); + + expect(commitsSpy).toHaveBeenCalledTimes(1); + expect(commitsSpy).toHaveBeenCalledWith(null); + }); + }); + + describe('addCommit()', () => { + it('should add a commit to the review', async () => { + const commitsSpy = jasmine.createSpy(); + service.commits$.subscribe(commitsSpy); + + const proposalReviewResponse = createProposalReviewResponse( + proposalId, + userId, + ); + reviewApiServiceMock.getOrCreateMyProposalReview.and.resolveTo( + proposalReviewResponse, + ); + + await service.loadOrCreateReview(proposalId); + service.addCommit(); + + expect(commitsSpy).toHaveBeenCalledTimes(3); + expect(commitsSpy).toHaveBeenCalledWith(null); + expect(commitsSpy).toHaveBeenCalledWith([]); + expect(commitsSpy).toHaveBeenCalledWith([ + createUiProposalReviewCommit({ uiId: '0' }), + ]); + }); + + it('should add subsequent commits to the review', async () => { + const subsequentDate = new Date(currentDate); + subsequentDate.setMilliseconds(currentDate.getMilliseconds() + 1_000); + + const commitsSpy = jasmine.createSpy(); + service.commits$.subscribe(commitsSpy); + + const proposalReviewResponse = createProposalReviewResponse( + proposalId, + userId, + ); + reviewApiServiceMock.getOrCreateMyProposalReview.and.resolveTo( + proposalReviewResponse, + ); + + await service.loadOrCreateReview(proposalId); + service.addCommit(); + + const createdCommit = createProposalReviewCommitResponse({ + proposalReviewId: proposalId, + createdAt: currentDate, + commitSha, + userId, + }); + commitReviewApiServiceMock.createProposalCommitReview.and.resolveTo({ + ...createdCommit, + id: '0', + }); + commitReviewApiServiceMock.updateProposalReviewCommit.and.resolveTo(null); + await service.updateCommit(null, commitSha, { reviewed: false }); + + jasmine.clock().mockDate(subsequentDate); + service.addCommit(); + + expect(commitsSpy).toHaveBeenCalledTimes(5); + expect(commitsSpy).toHaveBeenCalledWith(null); + expect(commitsSpy).toHaveBeenCalledWith([]); + expect(commitsSpy).toHaveBeenCalledWith([ + createUiProposalReviewCommit({ uiId: '0' }), + ]); + expect(commitsSpy).toHaveBeenCalledWith([ + createUiProposalReviewCommit({ + apiId: '0', + uiId: '0', + commit: { + commitSha, + details: { + reviewed: false, + }, + }, + }), + ]); + expect(commitsSpy).toHaveBeenCalledWith([ + createUiProposalReviewCommit({ + apiId: '0', + uiId: '0', + commit: { + commitSha, + details: { + reviewed: false, + }, + }, + }), + createUiProposalReviewCommit({ uiId: '1' }), + ]); + }); + + it('should throw an error if the proposal review already has an empty commit', async () => { + const commitsSpy = jasmine.createSpy(); + service.commits$.subscribe(commitsSpy); + + const proposalReviewResponse = createProposalReviewResponse( + proposalId, + userId, + ); + reviewApiServiceMock.getOrCreateMyProposalReview.and.resolveTo( + proposalReviewResponse, + ); + + await service.loadOrCreateReview(proposalId); + service.addCommit(); + + expect(() => service.addCommit()).toThrowError( + `An existing empty commit for proposal with Id ${proposalId} already exists`, + ); + }); + + it('should throw an error if a proposal review has not been selected', async () => { + expect(() => service.addCommit()).toThrowError( + `Tried to add a commit to a review without selecting a proposal`, + ); + }); + }); + + describe('removeCommit()', () => { + it('should delete a commit from the review', async () => { + const commitsSpy = jasmine.createSpy(); + service.commits$.subscribe(commitsSpy); + + const proposalReviewResponse = createProposalReviewResponse( + proposalId, + userId, + ); + reviewApiServiceMock.getOrCreateMyProposalReview.and.resolveTo( + proposalReviewResponse, + ); + + await service.loadOrCreateReview(proposalId); + service.addCommit(); + + await service.removeCommit(null); + + expect(commitsSpy).toHaveBeenCalledTimes(4); + expect(commitsSpy).toHaveBeenCalledWith(null); + expect(commitsSpy).toHaveBeenCalledWith([]); + expect(commitsSpy).toHaveBeenCalledWith([ + createUiProposalReviewCommit({ uiId: '0' }), + ]); + expect(commitsSpy).toHaveBeenCalledWith([]); + }); + + it('should throw an error if the proposal review does not exist', async () => { + await expectAsync(service.removeCommit(commitSha)).toBeRejectedWithError( + `Tried to remove a commit from a review without selecting a proposal`, + ); + }); + + it('should throw an error if the commit does not exist', async () => { + const proposalReviewResponse = createProposalReviewResponse( + proposalId, + userId, + ); + reviewApiServiceMock.getOrCreateMyProposalReview.and.resolveTo( + proposalReviewResponse, + ); + + await service.loadOrCreateReview(proposalId); + + await expectAsync(service.removeCommit(commitSha)).toBeRejectedWithError( + `Tried to remove a commit with SHA ${commitSha} from proposal with Id ${proposalId} but it does not exist`, + ); + }); + }); + + describe('updateCommit()', () => { + it('should update a commit in the review', async () => { + const commitsSpy = jasmine.createSpy(); + service.commits$.subscribe(commitsSpy); + + const proposalReviewResponse = createProposalReviewResponse( + proposalId, + userId, + ); + reviewApiServiceMock.getOrCreateMyProposalReview.and.resolveTo( + proposalReviewResponse, + ); + + await service.loadOrCreateReview(proposalId); + service.addCommit(); + + const updatedCommit = createProposalReviewCommitResponse({ + proposalReviewId: proposalId, + createdAt: currentDate, + commitSha, + userId, + }); + commitReviewApiServiceMock.createProposalCommitReview.and.resolveTo({ + ...updatedCommit, + id: '0', + }); + commitReviewApiServiceMock.updateProposalReviewCommit.and.resolveTo(null); + + await service.updateCommit(null, commitSha, { reviewed: false }); + + expect(commitsSpy).toHaveBeenCalledTimes(4); + expect(commitsSpy).toHaveBeenCalledWith(null); + expect(commitsSpy).toHaveBeenCalledWith([]); + expect(commitsSpy).toHaveBeenCalledWith([ + createUiProposalReviewCommit({ uiId: '0' }), + ]); + expect(commitsSpy).toHaveBeenCalledWith([ + createUiProposalReviewCommit({ + apiId: '0', + uiId: '0', + commit: { + commitSha, + details: { + reviewed: false, + }, + }, + }), + ]); + }); + + it('should throw an error if the proposal review does not exist', async () => { + await expectAsync( + service.updateCommit(commitSha, commitSha, { + reviewed: true, + comment: null, + matchesDescription: null, + highlights: [], + }), + ).toBeRejectedWithError( + 'Tried to update a commit for a review without selecting a proposal', + ); + }); + + it('should throw an error if the commit does not exist', async () => { + const proposalReviewResponse = createProposalReviewResponse( + proposalId, + userId, + ); + reviewApiServiceMock.getOrCreateMyProposalReview.and.resolveTo( + proposalReviewResponse, + ); + + await service.loadOrCreateReview(proposalId); + + await expectAsync( + service.updateCommit(commitSha, commitSha, { + reviewed: true, + comment: null, + matchesDescription: null, + highlights: [], + }), + ).toBeRejectedWithError( + `Tried to update a commit with SHA ${commitSha} from proposal with Id ${proposalId} but it does not exist`, + ); + }); + }); +}); + +function createProposalReviewResponse( + proposalId: string, + userId: string, +): GetProposalReviewResponse { + return { + id: proposalId, + proposalId, + userId, + vote: null, + createdAt: new Date('2021-09-01T00:00:00Z'), + lastUpdatedAt: null, + status: ProposalReviewStatus.Draft, + summary: null, + reviewDurationMins: null, + buildReproduced: null, + reproducedBuildImageId: [], + commits: [], + }; +} + +function createProposalReviewCommitResponse( + opts: Partial, +): GetProposalReviewCommitResponse { + return { + id: '', + commitSha: '', + createdAt: new Date(), + lastUpdatedAt: null, + proposalReviewId: '', + userId: '', + ...opts, + details: { + reviewed: false, + ...opts.details, + }, + }; +} + +function createUiProposalReviewCommit( + opts: Partial, +): UiProposalReviewCommit { + return { + apiId: opts.apiId ?? null, + uiId: opts.uiId ?? '0', + commit: { + commitSha: opts.commit?.commitSha ?? null, + details: { + reviewed: null, + ...opts.commit?.details, + }, + }, + }; +} diff --git a/src/frontend/src/app/core/state/review-submission/review-submission.service.ts b/src/frontend/src/app/core/state/review-submission/review-submission.service.ts new file mode 100644 index 00000000..397ecbd5 --- /dev/null +++ b/src/frontend/src/app/core/state/review-submission/review-submission.service.ts @@ -0,0 +1,317 @@ +import { DestroyRef, Injectable, inject } from '@angular/core'; +import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; +import { + BehaviorSubject, + Subscription, + distinctUntilChanged, + from, + of, +} from 'rxjs'; + +import { + ReviewApiService, + GetProposalReviewResponse, + CommitReviewApiService, + GetProposalReviewCommitResponse, + UpdateProposalReviewRequest, + ReviewCommitDetails, +} from '../../api'; +import { batchApiCall, filterNotNil, isNil, isNotNil } from '../../utils'; + +export interface UiProposalReviewCommit { + apiId: string | null; + uiId: string; + commit: { + commitSha: string | null; + details: ReviewCommitDetails; + }; +} + +@Injectable({ + providedIn: 'root', +}) +export class ReviewSubmissionService { + private readonly reviewApiService = inject(ReviewApiService); + private readonly commitReviewApiService = inject(CommitReviewApiService); + private readonly destroyRef = inject(DestroyRef); + + private readonly reviewSubject = + new BehaviorSubject(null); + public readonly review$ = this.reviewSubject + .asObservable() + .pipe(distinctUntilChanged()); + private get review(): GetProposalReviewResponse { + const review = this.reviewSubject.value; + if (isNil(review)) { + throw new Error('Review is not loaded'); + } + + return review; + } + + private readonly commitsSubject = new BehaviorSubject< + UiProposalReviewCommit[] | null + >(null); + public readonly commits$ = this.commitsSubject + .asObservable() + .pipe(distinctUntilChanged()); + + private readonly pendingReviewSubject = + new BehaviorSubject(null); + + private commits = new Map(); + private commitObservables = new Map< + string | null, + BehaviorSubject + >(); + private commitSubscriptions = new Map(); + private commitCreatePromises = new Map< + string | null, + Promise + >(); + + private listId = 0; + private proposalId: string | null = null; + + constructor() { + this.pendingReviewSubject + .pipe( + takeUntilDestroyed(), + filterNotNil(), + batchApiCall(review => + from( + this.reviewApiService.updateProposalReview({ + proposalId: this.proposalId!, + summary: review.summary, + reviewDurationMins: review.reviewDurationMins, + buildReproduced: review.buildReproduced, + vote: review.vote, + }), + ), + ), + ) + .subscribe({}); + } + + public async loadOrCreateReview(proposalId: string): Promise { + this.commits.clear(); + this.commitObservables.clear(); + this.commitSubscriptions.forEach(subscription => { + subscription.unsubscribe(); + }); + this.commitSubscriptions.clear(); + + this.reviewSubject.next(null); + this.proposalId = proposalId; + + const res = await this.reviewApiService.getOrCreateMyProposalReview({ + proposalId, + }); + + res.commits.forEach(commit => { + this.commits.set(commit.commitSha, { + apiId: commit.id, + uiId: String(this.listId++), + commit: { + commitSha: commit.commitSha, + details: commit.details, + }, + }); + + const [observable, subscription] = this.createCommitObservable(); + this.commitObservables.set(commit.commitSha, observable); + this.commitSubscriptions.set(commit.commitSha, subscription); + }); + + this.reviewSubject.next(res); + this.commitsSubject.next(Array.from(this.commits.values())); + } + + public updateReview(updatedReview: UpdateProposalReviewRequest): void { + if (isNil(this.proposalId)) { + throw new Error('Tried to update a review without selecting a proposal'); + } + + this.pendingReviewSubject.next(updatedReview); + } + + public addCommit(): void { + if (isNil(this.proposalId)) { + throw new Error( + 'Tried to add a commit to a review without selecting a proposal', + ); + } + + if (isNotNil(this.commits.get(null))) { + throw new Error( + `An existing empty commit for proposal with Id ${this.proposalId} already exists`, + ); + } + + this.commits.set(null, { + apiId: null, + uiId: String(this.listId++), + commit: { + commitSha: null, + details: { + reviewed: null, + }, + }, + }); + this.commitsSubject.next(Array.from(this.commits.values())); + } + + public async removeCommit(commitSha: string | null): Promise { + if (isNil(this.proposalId)) { + throw new Error( + 'Tried to remove a commit from a review without selecting a proposal', + ); + } + + const commit = this.commits.get(commitSha); + if (isNil(commit)) { + throw new Error( + `Tried to remove a commit with SHA ${commitSha} from proposal with Id ${this.proposalId} but it does not exist`, + ); + } + + this.commits.delete(commitSha); + this.commitsSubject.next(Array.from(this.commits.values())); + + if (isNotNil(commit.apiId)) { + commit; + await this.commitReviewApiService.deleteProposalReviewCommit({ + proposalReviewCommitId: commit.apiId, + }); + } + } + + public async updateCommit( + oldCommitSha: string | null, + newCommitSha: string | null, + newCommit: ReviewCommitDetails, + ): Promise { + if (isNil(this.proposalId)) { + throw new Error( + 'Tried to update a commit for a review without selecting a proposal', + ); + } + + const oldLocalCommit = this.commits.get(oldCommitSha); + if (isNil(oldLocalCommit)) { + throw new Error( + `Tried to update a commit with SHA ${oldCommitSha} from proposal with Id ${this.proposalId} but it does not exist`, + ); + } + + if (oldCommitSha !== newCommitSha) { + this.commits.delete(oldCommitSha); + } + const updatedCommit: UiProposalReviewCommit = { + apiId: oldLocalCommit.apiId, + uiId: oldLocalCommit.uiId, + commit: { + commitSha: newCommitSha, + details: newCommit, + }, + }; + this.commits.set(newCommitSha, updatedCommit); + this.commitsSubject.next(Array.from(this.commits.values())); + + const existingPromise = this.commitCreatePromises.get(oldCommitSha); + if (isNotNil(existingPromise)) { + const oldRemoteCommit = await existingPromise; + + this.commits.set(newCommitSha, { + apiId: oldRemoteCommit.id, + uiId: oldLocalCommit.uiId, + commit: { + commitSha: newCommitSha, + details: newCommit, + }, + }); + } + + if ( + isNil(oldLocalCommit.apiId) && + isNotNil(newCommitSha) && + isNotNil(newCommit.reviewed) + ) { + const promise = this.commitReviewApiService.createProposalCommitReview({ + commitSha: newCommitSha, + proposalReviewId: this.review.id, + reviewed: newCommit.reviewed, + }); + this.commitCreatePromises.set(oldCommitSha, promise); + + const createdCommit = await promise; + updatedCommit.apiId = createdCommit.id; + + this.commitCreatePromises.delete(oldCommitSha); + this.commits.set(newCommitSha, updatedCommit); + + const [observable, subscription] = this.createCommitObservable(); + this.commitObservables.set(oldCommitSha, observable); + this.commitSubscriptions.set(oldCommitSha, subscription); + } + + if (isNil(oldLocalCommit.apiId)) { + return; + } + + if (oldCommitSha !== newCommitSha) { + this.handleCommitShaChange(oldCommitSha, newCommitSha); + } + + this.commitObservables.get(newCommitSha)?.next(updatedCommit); + } + + private handleCommitShaChange( + previousCommitSha: string | null, + newCommitSha: string | null, + ): void { + const prevObservable = this.commitObservables.get(previousCommitSha); + const prevSubscription = this.commitSubscriptions.get(previousCommitSha); + + if (isNil(prevObservable) || isNil(prevSubscription)) { + throw new Error( + `Missing observable or subscription for commit ${previousCommitSha}`, + ); + } + + this.commitObservables.delete(previousCommitSha); + this.commitSubscriptions.delete(previousCommitSha); + + this.commitObservables.set(newCommitSha, prevObservable); + this.commitSubscriptions.set(newCommitSha, prevSubscription); + } + + private createCommitObservable(): [ + BehaviorSubject, + Subscription, + ] { + const commitSubject = new BehaviorSubject( + null, + ); + + const subscription = commitSubject + .asObservable() + .pipe( + takeUntilDestroyed(this.destroyRef), + filterNotNil(), + batchApiCall(commit => + isNotNil(commit.apiId) + ? from( + this.commitReviewApiService.updateProposalReviewCommit({ + proposalReviewCommitId: commit.apiId!, + details: commit.commit.details, + }), + ) + : of(null), + ), + ) + .subscribe({}); + + return [commitSubject, subscription]; + } +} diff --git a/src/frontend/src/app/core/ui/key-col/key-col.component.ts b/src/frontend/src/app/core/ui/key-col/key-col.component.ts index fc413b35..d55278cf 100644 --- a/src/frontend/src/app/core/ui/key-col/key-col.component.ts +++ b/src/frontend/src/app/core/ui/key-col/key-col.component.ts @@ -21,8 +21,6 @@ import { ChangeDetectionStrategy, Component } from '@angular/core'; } `, ], - template: ` - - `, + template: ``, }) export class KeyColComponent {} diff --git a/src/frontend/src/app/core/ui/key-value-grid/key-value-grid.component.ts b/src/frontend/src/app/core/ui/key-value-grid/key-value-grid.component.ts index 8c8b94a8..fd235fd6 100644 --- a/src/frontend/src/app/core/ui/key-value-grid/key-value-grid.component.ts +++ b/src/frontend/src/app/core/ui/key-value-grid/key-value-grid.component.ts @@ -34,9 +34,7 @@ import { } `, ], - template: ` - - `, + template: ``, }) export class KeyValueGridComponent { public readonly columnNumber = input(1); diff --git a/src/frontend/src/app/core/ui/tooltip/tooltip.component.ts b/src/frontend/src/app/core/ui/tooltip/tooltip.component.ts index d24187fb..74ba89b9 100644 --- a/src/frontend/src/app/core/ui/tooltip/tooltip.component.ts +++ b/src/frontend/src/app/core/ui/tooltip/tooltip.component.ts @@ -26,9 +26,7 @@ import { ChangeDetectionStrategy, Component, input } from '@angular/core'; } `, ], - template: ` - {{ tooltipText() }} - `, + template: `{{ tooltipText() }}`, }) export class TooltipComponent { public readonly tooltipText = input.required(); diff --git a/src/frontend/src/app/core/ui/tooltip/tooltip.directive.spec.ts b/src/frontend/src/app/core/ui/tooltip/tooltip.directive.spec.ts index 1a369a3f..f635cc9d 100644 --- a/src/frontend/src/app/core/ui/tooltip/tooltip.directive.spec.ts +++ b/src/frontend/src/app/core/ui/tooltip/tooltip.directive.spec.ts @@ -7,9 +7,7 @@ import { TooltipDirective } from './tooltip.directive'; @Component({ standalone: true, imports: [TooltipDirective], - template: ` -
Test
- `, + template: `
Test
`, }) class TestComponent {} diff --git a/src/frontend/src/app/core/ui/value-col/value-col.component.ts b/src/frontend/src/app/core/ui/value-col/value-col.component.ts index 3ff2cfa8..0b05f550 100644 --- a/src/frontend/src/app/core/ui/value-col/value-col.component.ts +++ b/src/frontend/src/app/core/ui/value-col/value-col.component.ts @@ -22,8 +22,6 @@ import { ChangeDetectionStrategy, Component } from '@angular/core'; } `, ], - template: ` - - `, + template: ``, }) export class ValueColComponent {} diff --git a/src/frontend/src/app/core/utils/candid.ts b/src/frontend/src/app/core/utils/candid.ts index 350d3232..11beac99 100644 --- a/src/frontend/src/app/core/utils/candid.ts +++ b/src/frontend/src/app/core/utils/candid.ts @@ -21,5 +21,5 @@ export function fromCandidOptDate(value: [] | [string]): Date | null { } export function fromCandidDate(value: string): Date { - return new Date(Number(value)); + return new Date(value); } diff --git a/src/frontend/src/app/core/utils/form.spec.ts b/src/frontend/src/app/core/utils/form.spec.ts new file mode 100644 index 00000000..d7f3864d --- /dev/null +++ b/src/frontend/src/app/core/utils/form.spec.ts @@ -0,0 +1,64 @@ +import { Observable, delay, of } from 'rxjs'; + +import { createTestScheduler } from '../../testing'; +import { batchApiCall } from './form'; + +describe('batchApiCall()', () => { + it('should debounce values', () => { + const testScheduler = createTestScheduler(); + + testScheduler.run(helpers => { + const { cold, expectObservable } = helpers; + + const changes = ' a 1500ms c 1500ms a b c a b c a b 1500ms '; + const expected = ' 1500ms a 1500ms c - - - - - - - 1500ms b'; + const values = { a: 'a', b: 'b', c: 'c' }; + + const source$ = cold(changes, values); + const apiCall = (value: string): Observable => of(value); + + const output$ = source$.pipe(batchApiCall(apiCall, testScheduler)); + + expectObservable(output$).toBe(expected, values); + }); + }); + + it('should throttle values while API calls are in progress', () => { + const testScheduler = createTestScheduler(); + + testScheduler.run(helpers => { + const { cold, expectObservable } = helpers; + + const changes = ' a 1500ms a 1500ms c 1500ms 3000ms '; + const expected = ' 1500ms 1500ms 1500ms a - 3000ms c'; + const values = { a: 'a', b: 'b', c: 'c' }; + + const source$ = cold(changes, values); + const apiCall = (value: string): Observable => + of(value).pipe(delay(3_000)); + + const output$ = source$.pipe(batchApiCall(apiCall, testScheduler)); + + expectObservable(output$).toBe(expected, values); + }); + }); + + it('should filter duplicate values', () => { + const testScheduler = createTestScheduler(); + + testScheduler.run(helpers => { + const { cold, expectObservable } = helpers; + + const changes = ' a 1500ms a 1500ms b 1500ms b 1500ms a 1500ms a '; + const expected = ' 1500ms a 1500ms - 1500ms b 1500ms - 1500ms a 1500ms '; + const values = { a: 'a', b: 'b' }; + + const source$ = cold(changes, values); + const apiCall = (value: string): Observable => of(value); + + const output$ = source$.pipe(batchApiCall(apiCall, testScheduler)); + + expectObservable(output$).toBe(expected, values); + }); + }); +}); diff --git a/src/frontend/src/app/core/utils/form.ts b/src/frontend/src/app/core/utils/form.ts new file mode 100644 index 00000000..0ebd3e78 --- /dev/null +++ b/src/frontend/src/app/core/utils/form.ts @@ -0,0 +1,73 @@ +import { + BehaviorSubject, + Observable, + SchedulerLike, + UnaryFunction, + debounceTime, + distinctUntilChanged, + exhaustMap, + filter, + pipe, + take, + tap, + throttle, +} from 'rxjs'; + +export function batchApiCall( + apiCall: (value: T) => Observable, + scheduler?: SchedulerLike | undefined, +): UnaryFunction, Observable> { + const apiCallInProgress = new BehaviorSubject(false); + + return pipe( + distinctUntilChanged(), + debounceTime(1_500, scheduler), + throttle(() => + apiCallInProgress.pipe( + filter(inProgress => !inProgress), + take(1), + ), + ), + exhaustMap(value => { + apiCallInProgress.next(true); + + return apiCall(value).pipe( + tap(() => { + apiCallInProgress.next(false); + }), + ); + }), + ); +} + +export function boolToRadio(value: boolean | null | undefined): 0 | 1 | null { + switch (value) { + case true: { + return 1; + } + + case false: { + return 0; + } + + default: { + return null; + } + } +} + +export function radioToBool(value: 0 | 1 | null | undefined): boolean | null { + switch (value) { + case 1: { + return true; + } + + case 0: { + return false; + } + + default: { + return null; + } + } +} diff --git a/src/frontend/src/app/core/utils/index.ts b/src/frontend/src/app/core/utils/index.ts index 8f756899..3dfd16ba 100644 --- a/src/frontend/src/app/core/utils/index.ts +++ b/src/frontend/src/app/core/utils/index.ts @@ -1,5 +1,6 @@ export * from './api-response'; export * from './candid'; +export * from './form'; export * from './keys-of'; export * from './nil'; export * from './route-param'; diff --git a/src/frontend/src/app/pages/profile-edit/anonymous-profile/anonymous-profile.component.ts b/src/frontend/src/app/pages/profile-edit/anonymous-profile/anonymous-profile.component.ts index 3e9a6aea..d119b0ec 100644 --- a/src/frontend/src/app/pages/profile-edit/anonymous-profile/anonymous-profile.component.ts +++ b/src/frontend/src/app/pages/profile-edit/anonymous-profile/anonymous-profile.component.ts @@ -21,8 +21,7 @@ import { AnonymousGetMyUserProfileResponse } from '~core/api';

If you would like to become a reviewer, - apply now - . + apply now.

When requested, provide this ID to a CodeGov admin:

diff --git a/src/frontend/src/app/pages/proposal-details/closed-proposal-summary/closed-proposal-summary.component.ts b/src/frontend/src/app/pages/proposal-details/closed-proposal-summary/closed-proposal-summary.component.ts index c53c721d..5f387541 100644 --- a/src/frontend/src/app/pages/proposal-details/closed-proposal-summary/closed-proposal-summary.component.ts +++ b/src/frontend/src/app/pages/proposal-details/closed-proposal-summary/closed-proposal-summary.component.ts @@ -5,7 +5,6 @@ import { OnInit, computed, input, - signal, } from '@angular/core'; import { toSignal } from '@angular/core/rxjs-interop'; import { RouterLink } from '@angular/router'; @@ -15,11 +14,7 @@ import { DashCircleIconComponent, CheckCircleIconComponent, } from '@cg/angular-ui'; -import { - GetProposalResponse, - ProposalCommitReviewSummary, - ProposalReviewVote, -} from '~core/api'; +import { GetProposalResponse, ProposalCommitReviewSummary } from '~core/api'; import { ReviewService } from '~core/state'; import { KeyColComponent, @@ -180,10 +175,8 @@ import { {{ review.vote }} @@ -282,7 +275,6 @@ import { `, }) export class ClosedProposalSummaryComponent implements OnInit { - public readonly ProposalReviewVote = signal(ProposalReviewVote); public readonly proposal = input.required(); public readonly reviewList = toSignal(this.reviewService.proposalReviewList$); @@ -291,14 +283,8 @@ export class ClosedProposalSummaryComponent implements OnInit { () => this.reviewList()?.reduce( (accum, review) => ({ - adopt: - review.vote === ProposalReviewVote.Adopt - ? accum.adopt + 1 - : accum.adopt, - reject: - review.vote === ProposalReviewVote.Reject - ? accum.reject + 1 - : accum.reject, + adopt: review.vote ? accum.adopt + 1 : accum.adopt, + reject: review.vote === false ? accum.reject + 1 : accum.reject, buildReproduced: review.buildReproduced ? accum.buildReproduced + 1 : accum.buildReproduced, @@ -313,10 +299,10 @@ export class ClosedProposalSummaryComponent implements OnInit { this.reviewList()?.forEach(review => { for (const commit of review.commits) { const existingCommit = - map.get(commit.id ?? commit.listId) ?? + map.get(commit.id) ?? ({ proposalId: this.proposal().id, - commitId: commit.id ?? commit.listId, + commitId: commit.id, commitSha: commit.commitSha, totalReviewers: 0, reviewedCount: 0, @@ -343,7 +329,7 @@ export class ClosedProposalSummaryComponent implements OnInit { } } - map.set(commit.id ?? commit.listId, existingCommit); + map.set(commit.id, existingCommit); } }); return Array.from(map.values()); diff --git a/src/frontend/src/app/pages/proposal-list/proposal-list.component.ts b/src/frontend/src/app/pages/proposal-list/proposal-list.component.ts index 28de7657..5ba9b87f 100644 --- a/src/frontend/src/app/pages/proposal-list/proposal-list.component.ts +++ b/src/frontend/src/app/pages/proposal-list/proposal-list.component.ts @@ -20,7 +20,6 @@ import { ValueColComponent, FormFieldComponent, InputDirective, - LabelDirective, } from '~core/ui'; enum ReviewPeriodStateFilter { @@ -50,7 +49,6 @@ interface FilterForm { ValueColComponent, FormFieldComponent, InputDirective, - LabelDirective, FormatDatePipe, RouterLink, CardComponent, diff --git a/src/frontend/src/app/pages/proposal-review-edit/proposal-review-edit.component.spec.ts b/src/frontend/src/app/pages/proposal-review-edit/proposal-review-edit.component.spec.ts index 8c01043a..a5b8a665 100644 --- a/src/frontend/src/app/pages/proposal-review-edit/proposal-review-edit.component.spec.ts +++ b/src/frontend/src/app/pages/proposal-review-edit/proposal-review-edit.component.spec.ts @@ -3,12 +3,10 @@ import { ActivatedRoute, convertToParamMap } from '@angular/router'; import { of } from 'rxjs'; import { - ProposalLinkBaseUrl, - ProposalVotingLinkType, - ProposalTopic, - ProposalState, -} from '~core/api'; -import { ProposalService, ReviewService } from '~core/state'; + ProposalService, + ReviewService, + ReviewSubmissionService, +} from '~core/state'; import { ProposalServiceMock, proposalServiceMockFactory, @@ -17,6 +15,10 @@ import { ReviewServiceMock, reviewServiceMockFactory, } from '~core/state/review/review.service.mock'; +import { + ReviewSubmissionServiceMock, + reviewSubmissionServiceMockFactory, +} from '~core/state/review-submission/review-submission.service.mock'; import { ActivatedRouteMock, activatedRouteMockFactory, @@ -30,36 +32,12 @@ describe('ProposalReviewEditComponent', () => { let proposalServiceMock: ProposalServiceMock; let reviewServiceMock: ReviewServiceMock; + let reviewSubmissionServiceMock: ReviewSubmissionServiceMock; let activatedRouteMock: ActivatedRouteMock; beforeEach(async () => { proposalServiceMock = proposalServiceMockFactory(); - defineProp( - proposalServiceMock, - 'currentProposal$', - of({ - id: '1', - nsProposalId: 1n, - title: 'title', - topic: ProposalTopic.RVM, - type: 'unknown', - state: ProposalState.InProgress, - reviewPeriodEnd: new Date(2024, 1, 17, 1, 1, 25), - votingPeriodEnd: new Date(2024, 1, 19, 1, 1, 25), - proposedAt: new Date(2024, 1, 15, 1, 1, 25), - proposedBy: 432432432423n, - decidedAt: null, - summary: 'Elect new replica binary revision', - reviewCompletedAt: null, - codeGovVote: null, - proposalLinks: [ - { - type: ProposalVotingLinkType.NNSDApp, - link: ProposalLinkBaseUrl.NNSDApp + 1, - }, - ], - }), - ); + defineProp(proposalServiceMock, 'currentProposal$', of(null)); reviewServiceMock = reviewServiceMockFactory(); defineProp(reviewServiceMock, 'currentReview$', of(null)); @@ -71,6 +49,8 @@ describe('ProposalReviewEditComponent', () => { of(convertToParamMap([{ id: 1 }])), ); + reviewSubmissionServiceMock = reviewSubmissionServiceMockFactory(); + await TestBed.configureTestingModule({ imports: [ProposalReviewEditComponent], providers: [ @@ -80,6 +60,10 @@ describe('ProposalReviewEditComponent', () => { provide: ActivatedRoute, useValue: activatedRouteMock, }, + { + provide: ReviewSubmissionService, + useValue: reviewSubmissionServiceMock, + }, ], }).compileComponents(); diff --git a/src/frontend/src/app/pages/proposal-review-edit/proposal-review-edit.component.ts b/src/frontend/src/app/pages/proposal-review-edit/proposal-review-edit.component.ts index 16ef8da3..746308e0 100644 --- a/src/frontend/src/app/pages/proposal-review-edit/proposal-review-edit.component.ts +++ b/src/frontend/src/app/pages/proposal-review-edit/proposal-review-edit.component.ts @@ -12,7 +12,11 @@ import { first } from 'rxjs'; import { CardComponent } from '@cg/angular-ui'; import { GetProposalReviewCommitResponse, ProposalState } from '~core/api'; -import { ProposalService, ReviewService } from '~core/state'; +import { + ProposalService, + ReviewService, + ReviewSubmissionService, +} from '~core/state'; import { filterNotNil, routeParam, toSyncSignal } from '~core/utils'; import { ReviewCommitsFormComponent } from './review-commits-form'; import { ReviewDetailsFormComponent } from './review-details-form'; @@ -70,6 +74,7 @@ export class ProposalReviewEditComponent implements OnInit { private readonly router = inject(Router); private readonly proposalService = inject(ProposalService); private readonly reviewService = inject(ReviewService); + private readonly reviewSubmissionService = inject(ReviewSubmissionService); public readonly currentProposal = toSyncSignal( this.proposalService.currentProposal$, @@ -82,6 +87,7 @@ export class ProposalReviewEditComponent implements OnInit { constructor() { routeParam('id').subscribe(proposalId => { this.proposalService.setCurrentProposalId(proposalId); + this.reviewSubmissionService.loadOrCreateReview(proposalId); }); this.proposalService.currentProposal$ diff --git a/src/frontend/src/app/pages/proposal-review-edit/review-commits-form/review-commits-form.component.spec.ts b/src/frontend/src/app/pages/proposal-review-edit/review-commits-form/review-commits-form.component.spec.ts index 962e1beb..db552f0c 100644 --- a/src/frontend/src/app/pages/proposal-review-edit/review-commits-form/review-commits-form.component.spec.ts +++ b/src/frontend/src/app/pages/proposal-review-edit/review-commits-form/review-commits-form.component.spec.ts @@ -1,14 +1,31 @@ import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { of } from 'rxjs'; +import { defineProp } from '../../../testing'; +import { ReviewSubmissionService } from '~core/state'; +import { + ReviewSubmissionServiceMock, + reviewSubmissionServiceMockFactory, +} from '~core/state/review-submission/review-submission.service.mock'; import { ReviewCommitsFormComponent } from './review-commits-form.component'; describe('ReviewCommitsFormComponent', () => { let component: ReviewCommitsFormComponent; let fixture: ComponentFixture; + let reviewSubmissionServiceMock: ReviewSubmissionServiceMock; beforeEach(async () => { + reviewSubmissionServiceMock = reviewSubmissionServiceMockFactory(); + defineProp(reviewSubmissionServiceMock, 'commits$', of(null)); + await TestBed.configureTestingModule({ imports: [ReviewCommitsFormComponent], + providers: [ + { + provide: ReviewSubmissionService, + useValue: reviewSubmissionServiceMock, + }, + ], }).compileComponents(); fixture = TestBed.createComponent(ReviewCommitsFormComponent); diff --git a/src/frontend/src/app/pages/proposal-review-edit/review-commits-form/review-commits-form.component.ts b/src/frontend/src/app/pages/proposal-review-edit/review-commits-form/review-commits-form.component.ts index 4689559d..0d48b395 100644 --- a/src/frontend/src/app/pages/proposal-review-edit/review-commits-form/review-commits-form.component.ts +++ b/src/frontend/src/app/pages/proposal-review-edit/review-commits-form/review-commits-form.component.ts @@ -3,18 +3,25 @@ import { ChangeDetectionStrategy, Component, ElementRef, - signal, + OnDestroy, + computed, + effect, + inject, viewChildren, } from '@angular/core'; import { + AbstractControl, FormControl, FormGroup, ReactiveFormsModule, + ValidatorFn, Validators, } from '@angular/forms'; import { Subscription } from 'rxjs'; import { CardComponent, RadioInputComponent } from '@cg/angular-ui'; +import { ReviewCommitDetails } from '~core/api'; +import { ReviewSubmissionService } from '~core/state'; import { FormFieldComponent, InputDirective, @@ -24,6 +31,7 @@ import { KeyValueGridComponent, ValueColComponent, } from '~core/ui'; +import { boolToRadio, isNil, radioToBool, toSyncSignal } from '~core/utils'; @Component({ selector: 'app-review-commits-form', @@ -41,6 +49,7 @@ import { InputHintComponent, RadioInputComponent, ], + changeDetection: ChangeDetectionStrategy.OnPush, styles: [ ` @import '@cg/styles/common'; @@ -51,20 +60,20 @@ import { `, ], template: ` - @for (commitForm of commitForms(); track i; let i = $index) { - + @for (commit of commits(); track commit.uiId; let i = $index) { +
- + @@ -77,18 +86,22 @@ import { Please enter a valid commit hash. + + Another commit entry already has this hash. + + - @if (commitForm.controls.id.value) { + @if (commitForms()[i].controls.commitSha.value) { https://github.com/dfinity/ic/commit/{{ - commitForm.controls.id.value + commitForms()[i].controls.commitSha.value }} } @else { @@ -131,7 +144,9 @@ import { - +
Matches description
 @@ -164,14 +179,14 @@ import { - + @@ -180,20 +195,6 @@ import { - - - - - - - - - @@ -218,15 +219,48 @@ import {
`, - changeDetection: ChangeDetectionStrategy.OnPush, }) -export class ReviewCommitsFormComponent { - public readonly commitForms = signal>>([]); - public readonly commitShaInputs = +export class ReviewCommitsFormComponent implements OnDestroy { + private readonly reviewSubmissionService = inject(ReviewSubmissionService); + + public readonly commits = toSyncSignal(this.reviewSubmissionService.commits$); + public readonly commitForms = computed(() => { + const commits = this.commits() ?? []; + + return commits.map((commit, index) => { + const commitForm = this.createCommitForm(index); + commitForm.setValue( + commitToFormValue(commit.commit.commitSha, commit.commit.details), + ); + return commitForm; + }); + }); + + private readonly commitShaInputs = viewChildren>('commitShaInput'); - private readonly commitFormReviewedSubscriptions: Subscription[] = []; - private readonly commitShaSubscriptions: Subscription[] = []; + private formSubscription = new Subscription(); + + constructor() { + effect(() => { + const commitForms = this.commitForms(); + + this.formSubscription.unsubscribe(); + this.formSubscription = new Subscription(); + + commitForms.forEach((commitForm, index) => { + this.formSubscription.add( + commitForm.valueChanges.subscribe(value => { + this.onFormValueChange(value, index, commitForm); + }), + ); + }); + }); + } + + public ngOnDestroy(): void { + this.formSubscription.unsubscribe(); + } // [TODO] - convert to signal public canAddCommitForm(): boolean { @@ -234,64 +268,73 @@ export class ReviewCommitsFormComponent { } public onAddCommitForm(): void { - if (this.canAddCommitForm()) { - const commitForm = new FormGroup({ - id: new FormControl(null, { - validators: [Validators.required, Validators.pattern(commitShaRegex)], - }), - reviewed: new FormControl(null, { - validators: [Validators.required], - }), - matchesDescription: new FormControl(null), - summary: new FormControl(null), - highlights: new FormControl(null), - }); + if (!this.canAddCommitForm()) { + return; + } - this.commitForms.set([...this.commitForms(), commitForm]); + this.reviewSubmissionService.addCommit(); - const reviewedSubscription = - commitForm.controls.reviewed.valueChanges.subscribe(reviewed => { - this.onCommitReviewedChange(reviewed === 1, commitForm); - }); - this.commitFormReviewedSubscriptions.push(reviewedSubscription); + const commits = this.commits(); + if (isNil(commits)) { + return; + } - const commitShaSubscription = - commitForm.controls.id.valueChanges.subscribe(commitSha => { - this.onCommitShaChange(commitSha, commitForm); - }); - this.commitShaSubscriptions.push(commitShaSubscription); + setTimeout(() => { + this.focusCommitForm(this.commitShaInputs().length - 1); + }); + } - this.focusCommitShaInput(this.commitForms().length - 1); + public onRemoveCommitForm(index: number): void { + const commits = this.commits(); + if (isNil(commits)) { + return; } + + const commitSha = commits[index]?.commit.commitSha; + + this.reviewSubmissionService.removeCommit(commitSha); + + setTimeout(() => { + this.focusCommitForm(Math.min(index, this.commitShaInputs().length - 1)); + }); } - public onRemoveCommitForm(index: number): void { - this.commitForms.set([ - ...this.commitForms().slice(0, index), - ...this.commitForms().slice(index + 1), - ]); - - const [reviewedSubscription] = this.commitFormReviewedSubscriptions.splice( - index, - 1, - ); - reviewedSubscription.unsubscribe(); - - const [commitShaSubscription] = this.commitShaSubscriptions.splice( - index, - 1, - ); - commitShaSubscription.unsubscribe(); - - this.focusCommitShaInput(Math.min(index, this.commitForms().length - 1)); + private onFormValueChange( + value: Partial, + index: number, + commitForm: FormGroup, + ): void { + const commits = this.commits(); + if (isNil(commits)) { + return; + } + + const commit = commits[index]; + + const reviewed = radioToBool(value.reviewed); + if (reviewed !== commit.commit.details.reviewed) { + this.onCommitReviewedChange(reviewed, commitForm); + } + + if (value.commitSha !== commit.commit.commitSha) { + this.onCommitShaChange(value.commitSha, commitForm); + } + + if (commitForm.valid) { + this.reviewSubmissionService.updateCommit( + commit.commit.commitSha ? commit.commit.commitSha : null, + value.commitSha ? value.commitSha : null, + commitFromFormValue(value), + ); + } } private onCommitReviewedChange( - reviewed: boolean, + reviewed: boolean | null, commitForm: FormGroup, ): void { const matchesDescription = commitForm.controls.matchesDescription; - const summary = commitForm.controls.summary; + const summary = commitForm.controls.comment; if (reviewed) { matchesDescription.addValidators(Validators.required); @@ -300,44 +343,84 @@ export class ReviewCommitsFormComponent { matchesDescription.removeValidators(Validators.required); summary.removeValidators(Validators.required); - matchesDescription.reset(); - summary.reset(); + matchesDescription.reset(null, { emitEvent: false }); + summary.reset(null, { emitEvent: false }); } - matchesDescription.updateValueAndValidity(); - summary.updateValueAndValidity(); + matchesDescription.updateValueAndValidity({ emitEvent: false }); + summary.updateValueAndValidity({ emitEvent: false }); } private onCommitShaChange( commitSha: string | null | undefined, commitForm: FormGroup, ): void { - if (!commitSha) { + if (isNil(commitSha)) { return; } const result = extractCommitSha(commitSha); - - if (!result) { + if (isNil(result)) { return; } - commitForm.controls.id.setValue(result, { emitEvent: false }); + commitForm.controls.commitSha.setValue(result, { emitEvent: false }); } - private focusCommitShaInput(index: number): void { - setTimeout(() => { - this.commitShaInputs()[index]?.nativeElement.focus(); - }, 0); + private focusCommitForm(index: number): void { + this.commitShaInputs()[index]?.nativeElement.focus(); + } + + private createCommitForm(index: number): FormGroup { + return new FormGroup({ + commitSha: new FormControl(null, { + validators: [ + Validators.required, + Validators.pattern(commitShaRegex), + this.uniqueCommitShaValidator(index), + ], + }), + reviewed: new FormControl(null, { + validators: [Validators.required], + }), + matchesDescription: new FormControl(null), + comment: new FormControl(null), + }); + } + + private uniqueCommitShaValidator(index: number): ValidatorFn { + return (control: AbstractControl) => { + const commits = this.commits(); + if (isNil(commits)) { + return null; + } + + const commitSha = extractCommitSha(control.value); + if (isNil(commitSha)) { + return null; + } + + const existingCommit = commits.find( + (commit, i) => i !== index && commit.commit.commitSha === commitSha, + ); + + return existingCommit ? { uniqueSha: true } : null; + }; } } +interface ReviewCommitFormValue { + commitSha: string | null; + reviewed: 0 | 1 | null; + matchesDescription: 0 | 1 | null; + comment: string | null; +} + interface ReviewCommitForm { - id: FormControl; + commitSha: FormControl; reviewed: FormControl<0 | 1 | null>; matchesDescription: FormControl<0 | 1 | null>; - summary: FormControl; - highlights: FormControl; + comment: FormControl; } const commitShaRegex = /[0-9a-f]{7,40}/; @@ -345,3 +428,42 @@ const commitShaRegex = /[0-9a-f]{7,40}/; function extractCommitSha(commitSha: string): string | null { return commitShaRegex.exec(commitSha)?.[0] ?? null; } + +function commitToFormValue( + commitSha: string | null, + commit: ReviewCommitDetails, +): ReviewCommitFormValue { + const reviewed = boolToRadio(commit.reviewed); + + let matchesDescription = null; + let comment = null; + + if (commit.reviewed) { + matchesDescription = boolToRadio(commit.matchesDescription); + comment = commit.comment; + } + + return { + commitSha: commitSha, + matchesDescription, + reviewed, + comment, + }; +} + +function commitFromFormValue( + formValue: Partial, +): ReviewCommitDetails { + const reviewed = radioToBool(formValue.reviewed); + + if (reviewed) { + return { + matchesDescription: radioToBool(formValue.matchesDescription), + reviewed: true, + comment: formValue.comment ?? null, + highlights: [], + }; + } + + return { reviewed }; +} diff --git a/src/frontend/src/app/pages/proposal-review-edit/review-details-form/review-details-form.component.spec.ts b/src/frontend/src/app/pages/proposal-review-edit/review-details-form/review-details-form.component.spec.ts index 4905f6eb..520223a6 100644 --- a/src/frontend/src/app/pages/proposal-review-edit/review-details-form/review-details-form.component.spec.ts +++ b/src/frontend/src/app/pages/proposal-review-edit/review-details-form/review-details-form.component.spec.ts @@ -1,14 +1,31 @@ import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { of } from 'rxjs'; +import { defineProp } from '../../../testing'; +import { ReviewSubmissionService } from '~core/state'; +import { + ReviewSubmissionServiceMock, + reviewSubmissionServiceMockFactory, +} from '~core/state/review-submission/review-submission.service.mock'; import { ReviewDetailsFormComponent } from './review-details-form.component'; describe('ReviewDetailsFormComponent', () => { let component: ReviewDetailsFormComponent; let fixture: ComponentFixture; + let reviewSubmissionServiceMock: ReviewSubmissionServiceMock; beforeEach(async () => { + reviewSubmissionServiceMock = reviewSubmissionServiceMockFactory(); + defineProp(reviewSubmissionServiceMock, 'review$', of(null)); + await TestBed.configureTestingModule({ imports: [ReviewDetailsFormComponent], + providers: [ + { + provide: ReviewSubmissionService, + useValue: reviewSubmissionServiceMock, + }, + ], }).compileComponents(); fixture = TestBed.createComponent(ReviewDetailsFormComponent); diff --git a/src/frontend/src/app/pages/proposal-review-edit/review-details-form/review-details-form.component.ts b/src/frontend/src/app/pages/proposal-review-edit/review-details-form/review-details-form.component.ts index a25c3499..fd5203bc 100644 --- a/src/frontend/src/app/pages/proposal-review-edit/review-details-form/review-details-form.component.ts +++ b/src/frontend/src/app/pages/proposal-review-edit/review-details-form/review-details-form.component.ts @@ -1,12 +1,21 @@ import { CommonModule } from '@angular/common'; -import { ChangeDetectionStrategy, Component, signal } from '@angular/core'; +import { + ChangeDetectionStrategy, + Component, + OnDestroy, + effect, + inject, + signal, +} from '@angular/core'; import { FormControl, FormGroup, ReactiveFormsModule } from '@angular/forms'; +import { Subscription } from 'rxjs'; import { ImageSet, ImageUploaderBtnComponent, RadioInputComponent, } from '@cg/angular-ui'; +import { ReviewSubmissionService } from '~core/state'; import { FormFieldComponent, InputDirective, @@ -14,6 +23,7 @@ import { KeyValueGridComponent, ValueColComponent, } from '~core/ui'; +import { boolToRadio, isNil, radioToBool, toSyncSignal } from '~core/utils'; @Component({ selector: 'app-review-details-form', @@ -34,14 +44,14 @@ import { - + @@ -84,6 +94,33 @@ import { + +
Vote to adopt
+ + + +
+ + Yes + + + + No + +
+ + +
Build verification images
 @@ -102,24 +139,92 @@ import { `, }) -export class ReviewDetailsFormComponent { +export class ReviewDetailsFormComponent implements OnDestroy { + private readonly reviewSubmissionService = inject(ReviewSubmissionService); + + private readonly review = toSyncSignal(this.reviewSubmissionService.review$); + public readonly reviewForm = signal( new FormGroup({ - timeSpent: new FormControl(null), + reviewDurationMins: new FormControl(null), summary: new FormControl(null), buildReproduced: new FormControl(null), + vote: new FormControl(null), }), ); - public selectedImages = signal([]); + private formSubscription = new Subscription(); + + constructor() { + effect(() => { + const reviewForm = this.reviewForm(); + + this.formSubscription.unsubscribe(); + this.formSubscription = new Subscription(); + + this.formSubscription.add( + reviewForm.valueChanges.subscribe(value => { + this.onFormValueChange(value, reviewForm); + }), + ); + }); + + effect(() => { + const review = this.review(); + if (!review) { + return; + } + + this.reviewForm().setValue( + { + reviewDurationMins: review.reviewDurationMins, + summary: review.summary, + buildReproduced: boolToRadio(review.buildReproduced), + vote: boolToRadio(review.vote), + }, + { emitEvent: false }, + ); + }); + } + + public ngOnDestroy(): void { + this.formSubscription.unsubscribe(); + } + public onImagesSelected(images: ImageSet[]): void { this.selectedImages.set(images); } + + private onFormValueChange( + value: Partial, + reviewForm: FormGroup, + ): void { + const review = this.review(); + if (!reviewForm.valid || isNil(review)) { + return; + } + + this.reviewSubmissionService.updateReview({ + proposalId: review.proposalId, + summary: value.summary ?? null, + reviewDurationMins: value.reviewDurationMins ?? null, + buildReproduced: radioToBool(value.buildReproduced), + vote: radioToBool(value.vote), + }); + } +} + +interface ReviewFormValue { + reviewDurationMins: number | null; + summary: string | null; + buildReproduced: 0 | 1 | null; + vote: 0 | 1 | null; } interface ReviewForm { - timeSpent: FormControl; + reviewDurationMins: FormControl; summary: FormControl; - buildReproduced: FormControl; + buildReproduced: FormControl<0 | 1 | null>; + vote: FormControl<0 | 1 | null>; } diff --git a/src/frontend/src/app/pages/proposal-review/proposal-review.component.ts b/src/frontend/src/app/pages/proposal-review/proposal-review.component.ts index ba980942..4710899d 100644 --- a/src/frontend/src/app/pages/proposal-review/proposal-review.component.ts +++ b/src/frontend/src/app/pages/proposal-review/proposal-review.component.ts @@ -9,11 +9,7 @@ import { toSignal } from '@angular/core/rxjs-interop'; import { RouterLink } from '@angular/router'; import { CardComponent } from '@cg/angular-ui'; -import { - ProposalReviewStatus, - ProposalReviewVote, - ProposalState, -} from '~core/api'; +import { ProposalReviewStatus, ProposalState } from '~core/api'; import { ProfileService, ProposalService, ReviewService } from '~core/state'; import { KeyColComponent, @@ -84,10 +80,8 @@ import { isNotNil, routeParam } from '~core/utils'; {{ review.vote }} @@ -200,7 +194,6 @@ import { isNotNil, routeParam } from '~core/utils'; }) export class ProposalReviewComponent { public readonly ProposalReviewStatus = signal(ProposalReviewStatus); - public readonly ProposalReviewVote = signal(ProposalReviewVote); public readonly ProposalState = signal(ProposalState); public readonly userProfile = toSignal(this.profileService.userProfile$); diff --git a/src/frontend/src/app/testing/test-utils.ts b/src/frontend/src/app/testing/test-utils.ts index 801b976a..70cc3d39 100644 --- a/src/frontend/src/app/testing/test-utils.ts +++ b/src/frontend/src/app/testing/test-utils.ts @@ -1,3 +1,5 @@ +import { TestScheduler } from 'rxjs/testing'; + export function defineProp( obj: T, key: K, @@ -8,3 +10,9 @@ export function defineProp( writable: false, }); } + +export function createTestScheduler(): TestScheduler { + return new TestScheduler((actual, expected) => { + expect(actual).toEqual(expected); + }); +}