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.

Sign up for GitHub

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

Check and apply max deduction for handgrading annotations #512

Closed
wants to merge 7 commits into from
Closed

Check and apply max deduction for handgrading annotations #512

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a9cb0f5
Make "Actual exit status" always black when return code is not checked
broad-well Apr 20, 2024
10d8be3
Check max deduction for handgrading annotations
broad-well Apr 20, 2024
234c117
Undo changes inherited from other PR
broad-well Apr 20, 2024
83093ed
Added comment for modifying return_code_correct
Normanwqn Apr 23, 2024
38e2171
Replace forceUpdate with checking for w.vm.saving
Normanwqn Apr 23, 2024
09c91dd
Use checking w.vm.saving in test_handgrading
Normanwqn Apr 23, 2024
6e477cd
Merge branch 'develop' into handgrading
Normanwqn Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 40 additions & 2 deletions src/components/project_view/handgrading/handgrading.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@
import { Component, Inject, Prop, Vue, Watch } from 'vue-property-decorator';

import {
Annotation,
AppliedAnnotation,
AppliedAnnotationObserver,
Comment,
Expand Down Expand Up @@ -495,24 +496,61 @@ export default class Handgrading extends Vue implements AppliedAnnotationObserve
});
}

unchecked_total_deduction_by_annotation_pk(annotation: Annotation) {
return this.d_handgrading_result!.applied_annotations
.filter(result_annotation => result_annotation.annotation.pk === annotation.pk)
.map(applied_annotation => applied_annotation.annotation.deduction)
.reduce((a, b) => a + b, 0);
}

limited_deduction_by_annotation_pk(unchecked_total_deduction: number, annotation: Annotation) {
if (annotation.max_deduction !== null && unchecked_total_deduction < annotation.max_deduction) {
return annotation.max_deduction;
}
return unchecked_total_deduction;
}

// Must call before applying change to this.d_handgrading_result.applied_annotations
deduction_change(annotation: Annotation, unchecked_score_change: number) {
const unchecked_deduction_before = this.unchecked_total_deduction_by_annotation_pk(annotation);
const actual_deduction_before = this.limited_deduction_by_annotation_pk(
unchecked_deduction_before,
annotation
);
const unchecked_deduction_after = unchecked_deduction_before + unchecked_score_change;
const actual_deduction_after = this.limited_deduction_by_annotation_pk(
unchecked_deduction_after,
annotation
);
return actual_deduction_after - actual_deduction_before;
}

update_applied_annotation_created(applied_annotation: AppliedAnnotation): void {
if (applied_annotation.handgrading_result === this.d_handgrading_result!.pk) {
const score_change = this.deduction_change(
applied_annotation.annotation,
applied_annotation.annotation.deduction
);
this.d_handgrading_result!.applied_annotations.push(applied_annotation);
// tslint:disable-next-line no-floating-promises
this.update_score(applied_annotation.annotation.deduction);
this.update_score(score_change);
}
}

update_applied_annotation_deleted(applied_annotation: AppliedAnnotation): void {
if (applied_annotation.handgrading_result === this.d_handgrading_result!.pk) {
const score_change = this.deduction_change(
applied_annotation.annotation,
-applied_annotation.annotation.deduction
);
this.d_handgrading_result!.applied_annotations.splice(
this.d_handgrading_result!.applied_annotations.findIndex(
item => item.pk === applied_annotation.pk
),
1
);
// tslint:disable-next-line no-floating-promises
this.update_score(-applied_annotation.annotation.deduction);
this.update_score(score_change);
}
}

Expand Down
116 changes: 116 additions & 0 deletions tests/test_components/test_project_view/test_handgrading/test_handgrading.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,7 @@ describe('Comment tests', () => {

let annotation_with_long_description: ag_cli.Annotation;
let annotation_empty_long_description: ag_cli.Annotation;
let annotation_unaligned_max_deduction: ag_cli.Annotation;

// The number at the end of these names describes the ordering
// of these elements in the displayed comment/applied annotation
Expand Down Expand Up @@ -460,6 +461,16 @@ describe('Comment tests', () => {
}
);

annotation_unaligned_max_deduction = data_ut.make_annotation(
result.handgrading_rubric.pk,
{
short_description: "I have unaligned max deduction",
deduction: -3,
// Not a multiple of -3
max_deduction: -5
}
);

result.handgrading_rubric.annotations = [
annotation_with_long_description, annotation_empty_long_description
];
Expand All @@ -475,6 +486,8 @@ describe('Comment tests', () => {
result, annotation_with_long_description,
{filename: 'file2.cpp', first_line: 10, last_line: 10}
);
result.total_points_possible = 10;
result.total_points = 7;

data_ut.set_global_user_roles(data_ut.make_user_roles({is_staff: true}));
wrapper = managed_mount(Handgrading, {
Expand Down Expand Up @@ -637,6 +650,109 @@ describe('Comment tests', () => {
[applied_long_description_annotation_1]);
expect(delete_stub.calledOnce).toBe(true);
});

test('Add applied annotations beyond max deduction', async () => {
for (let i = 1; i <= 3; ++i) {
let applied_annotation = data_ut.make_applied_annotation(
result,
annotation_empty_long_description,
{filename: 'file1.txt', first_line: i, last_line: i}
);
ag_cli.AppliedAnnotation.notify_applied_annotation_created(applied_annotation);
}
wrapper.vm.$forceUpdate();
Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to avoid $forceUpdate to make sure that the page is re-rendered appropriately. Does it work with expect(await wait_until(wrapper, w => !w.vm.saving)).toBe(true); instead?

Copy link
Contributor

@Normanwqn Normanwqn Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I will test with that. Is there a reason that $forceUpdate is used here?

 test('Handgraders allowed to leave custom comments', async () => {
        wrapper.vm.d_handgrading_result!.handgrading_rubric.handgraders_can_leave_comments = true;
        data_ut.set_global_user_roles(data_ut.make_user_roles({is_handgrader: true}));
        wrapper.vm.$forceUpdate();
        await wrapper.vm.$nextTick();
        expect(wrapper.find('#new-comment').exists()).toBe(true);

        expect(wrapper.findAll('.comment').at(0).find('.delete').exists()).toBe(true);
        expect(wrapper.findAll('.comment').at(3).find('.delete').exists()).toBe(true);
        // Can always delete applied annotations
        expect(wrapper.findAll('.comment').at(1).find('.delete').exists()).toBe(true);
        expect(wrapper.findAll('.comment').at(2).find('.delete').exists()).toBe(true);
    });

await wrapper.vm.$nextTick();
expect(wrapper.find('.grading-sidebar-header .score').text()).toEqual('5/10');
});

test('Delete all applied annotations after exceeding max deduction', async () => {
for (let i = 1; i <= 4; ++i) {
let applied_annotation = data_ut.make_applied_annotation(
result,
annotation_empty_long_description,
{filename: 'file1.txt', first_line: i, last_line: i}
);
ag_cli.AppliedAnnotation.notify_applied_annotation_created(applied_annotation);
}
wrapper.vm.$forceUpdate();
await wrapper.vm.$nextTick();
expect(wrapper.vm.d_handgrading_result!.applied_annotations.length).toEqual(6);
expect(wrapper.find('.grading-sidebar-header .score').text()).toEqual('5/10');

const delete_stubs =
wrapper.vm.d_handgrading_result!.applied_annotations.map(annotation => {
return sinon.stub(
annotation, 'delete'
).callsFake(() => {
ag_cli.AppliedAnnotation.notify_applied_annotation_deleted(annotation);
return Promise.resolve();
});
});
for (let i = 0; i < 6; ++i) {
wrapper.findAll('.comment').at(1).find('.delete').trigger('click');
expect(await wait_until(wrapper, w => !w.vm.saving)).toBe(true);
}
expect(wrapper.vm.d_handgrading_result!.applied_annotations.length).toEqual(0);
expect(wrapper.find('.grading-sidebar-header .score').text()).toEqual('10/10');

for (const stub of delete_stubs) {
expect(stub.calledOnce).toBe(true);
}
});

test('Add applied annotation across unaligned max deduction', async () => {
const add_annotation = (i: number) => {
let applied_annotation = data_ut.make_applied_annotation(
result,
annotation_unaligned_max_deduction,
{filename: 'file3.py', first_line: i, last_line: i}
);
ag_cli.AppliedAnnotation.notify_applied_annotation_created(applied_annotation);
};

add_annotation(1);
wrapper.vm.$forceUpdate();
await wrapper.vm.$nextTick();
expect(wrapper.vm.d_handgrading_result!.applied_annotations.length).toEqual(3);
expect(wrapper.find('.grading-sidebar-header .score').text()).toEqual('4/10');

add_annotation(2);
wrapper.vm.$forceUpdate();
await wrapper.vm.$nextTick();
expect(wrapper.vm.d_handgrading_result!.applied_annotations.length).toEqual(4);
expect(wrapper.find('.grading-sidebar-header .score').text()).toEqual('2/10');
});

test('Delete applied annotation across unaligned max deduction', async () => {
const add_annotation = (i: number) => {
let applied_annotation = data_ut.make_applied_annotation(
result,
annotation_unaligned_max_deduction,
{filename: 'file3.py', first_line: i, last_line: i}
);
ag_cli.AppliedAnnotation.notify_applied_annotation_created(applied_annotation);
};

add_annotation(1);
add_annotation(2);
wrapper.vm.$forceUpdate();
await wrapper.vm.$nextTick();
expect(wrapper.vm.d_handgrading_result!.applied_annotations.length).toEqual(4);
expect(wrapper.find('.grading-sidebar-header .score').text()).toEqual('2/10');

let to_delete = wrapper.vm.d_handgrading_result!.applied_annotations[3];
let delete_stub = sinon.stub(
to_delete, 'delete'
).callsFake(() => {
ag_cli.AppliedAnnotation.notify_applied_annotation_deleted(to_delete);
return Promise.resolve();
});
wrapper.findAll('.comment').at(3).find('.delete').trigger('click');
expect(await wait_until(wrapper, w => !w.vm.saving)).toBe(true);

expect(delete_stub.calledOnce).toBe(true);
expect(wrapper.find('.grading-sidebar-header .score').text()).toEqual('4/10');
});
});

describe('Annotations reference list', () => {
Expand Down