Skip to content
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

Add documentation score #1339

Merged
merged 20 commits into from
Jan 13, 2025
Merged

Conversation

monsieurswag
Copy link
Contributor

No description provided.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


if (always_enabled) {
$isScored = true;
}

export let is_scored = $isScored;
export let score = $value;
export let documentation_score = $documentationScore;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the Score component generic by moving the documentation score logic to the parent component where it's specifically needed.

Comment on lines 380 to 393
<Score
form={superForm(requirementAssessment.scoreForm)}
form={superForm(requirementAssessment.scoreForm, { id: requirementAssessment.id })}
min_score={data.compliance_assessment.min_score}
max_score={data.compliance_assessment.max_score}
scores_definition={data.compliance_assessment.scores_definition}
show_documentation_score={data.compliance_assessment.show_documentation_score}
field="score"
label=""
styles="w-full p-1"
bind:score={requirementAssessment.score}
bind:is_scored={requirementAssessment.is_scored}
bind:score={requirementAssessment.score}
bind:documentation_score={requirementAssessment.documentation_score}
on:change={() => updateScore(requirementAssessment)}
/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's split this into two separate Score components: one for score and one for documentation_score. Having both in one component creates unnecessary coupling, thus reducing reusability and maintainability.

@nas-tabchiche nas-tabchiche force-pushed the CA-747-Add-optional-secondary-score branch from e358ada to 7226ca2 Compare January 11, 2025 00:09
Copy link
Collaborator

@eric-intuitem eric-intuitem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
We will update report generation in a future PR.

Copy link
Contributor

@nas-tabchiche nas-tabchiche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Would appreciate @Mohamed-Hacene's eyes for reviewing the changes made to the Score component, particularly regarding its interactions with the table-mode page.

@nas-tabchiche nas-tabchiche force-pushed the CA-747-Add-optional-secondary-score branch from 472f660 to 2007b71 Compare January 13, 2025 09:50
@Mohamed-Hacene
Copy link
Collaborator

0k. The caching effect can sometimes make the documentation score still visible in a requirementAssessment for a very short time after it has been deactivated (and vice versa when activated). This will be fixed by another PR on cache invalidation.

@nas-tabchiche
Copy link
Contributor

nas-tabchiche commented Jan 13, 2025

To pick up on @Mohamed-Hacene's last comment, this is a tradeoff.
Dropping the caching on compliance assessments hurts the usability of the audit detail view, as costly processing is made on the backend at that time. We will make finer-grained cache invalidation a shortly.

@eric-intuitem eric-intuitem merged commit 4a9cf29 into main Jan 13, 2025
20 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 13, 2025
@Mohamed-Hacene Mohamed-Hacene deleted the CA-747-Add-optional-secondary-score branch January 24, 2025 13:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants