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

Migrate:grade task modal #564

Merged
merged 57 commits into from
Nov 4, 2024

Conversation

leocomsci
Copy link

Description

Transfer PR started by @PerryRose

This pull request migrates the grade-task-modal component from AngularJS to Angular.
The grade task modal component is opened from task-service.coffee using the grade task modal service, which receives a task object and passes it onto the component via a MatDialogRef. Depending on the task object, a student's submission can either be rated using a Mat Slider, or graded with a Mat Button Toggle Group, which uses the grade-icon component which I have migrated. Clicking the submit button calls the component's close function, which passes a rating and a grade value back to the service before then being passed back to task-service.coffee, which updates the task object on the backend.

Quality Points Task

Before:

image

After:

image

Graded Task

Before:

image

After:

image

Fixes # (updates the old grade-task-modal component specified outlined in the migration task list)

Type of change

Please delete options that are not relevant.

  • Component migration

How Has This Been Tested?

  1. Log in as an admin
  2. Go to Manage Units and choose a unit
  3. Go to Tasks and choose a task
  4. Go to Other Settings
  5. To grade a task, tick the Graded option.
  6. To rate a task, select a number for the Quality Points option
  7. Save
  8. Log in as a tutor
  9. Go to Mark by Task Definition
  10. Select a task
  11. Select a submission and provide a rating

Testing Checklist:

  • Tested in latest Chrome
  • Tested in latest Safari
  • Tested in latest Firefox

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have requested a review from _ and _ on the Pull Request

tancnle and others added 30 commits April 14, 2022 12:30
This commit fixes dependencies issues that causes

- `npm install` fails
- `ng test` does not pick up any specs
This commits ensure the unit tests are setup with correct dependencies.
We use 2 Karma configurations to regulate how tests are run locally and
in CI.

- In development, the server runs forever with watch mode, using Chrome
browser
- In CI, the service runs once, using Chrome headless
refactor: remove task-sheet-viewer component
This adds ability to format code using both ESLint and Prettier rules.
There are a large number of violations in the code base. We will need to
incrementally address them in future pull requests.
This fixes 'Range out of order in character class' error when trying to
parse regex marked with `/u` unicode flag.
feat: format code using ESLint and Prettier
@macite macite changed the base branch from development to quality/entity-service-to-npm July 23, 2022 05:33
@leocomsci leocomsci closed this Aug 8, 2022
@leocomsci leocomsci reopened this Aug 8, 2022
@leocomsci leocomsci marked this pull request as ready for review August 8, 2022 05:23
@leocomsci
Copy link
Author

@macite Hi Andrew, could you review this PR? I can't add you as a reviewer for this PR so I mentioned you in this comment.
I also change name of welcome component, Jake used WelcomeWizardComponent but there's just WelcomeComponent so I adjusted it. Also, there are some problems with karma module so I couldn't run unit tests.

Copy link
Member

@macite macite left a comment

Choose a reason for hiding this comment

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

This looks pretty close. Can you check these last few things and make adjustments or show that the implementation should be ok?

gradeText: string;
gradeLetter: string;

constructor(@Inject(gradeService) private GradeService: any) {}
Copy link
Member

Choose a reason for hiding this comment

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

There should be a new equivalent of this in typescript.

Copy link
Member

Choose a reason for hiding this comment

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

Check if you can remove the GradeService from this and use the new code

<tr ng-repeat="contrib in team.memberContributions | orderBy:memberSortOrder:reverse">
<td>{{contrib.project.student.name}}</td>
<tr ng-repeat="member in team.members | orderBy:memberSortOrder:reverse">
<td>{{member.student_name}}</td>
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't look right - with the new service we do not have any snake case properties

Copy link
Member

Choose a reason for hiding this comment

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

Check everything in this...

grade="grade"
class="text-{{project.submitted_grade == grades.indexOf(grade) ? 'success' : 'muted'}}"
></grade-icon>
<grade-icon [grade]="grade"></grade-icon>
Copy link
Member

Choose a reason for hiding this comment

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

Can you check the muted state here... is this ok without it. It is not shown in the conversation.

@@ -43,7 +43,6 @@ <h4 class="panel-title">Mark Portfolios</h4>
<span
class="grade-icon"
tooltip="Show all portfolios"
class="text-{{$index == filterOptions.selectedGrade ? 'primary' : 'muted'}}"
>All</span
Copy link
Member

Choose a reason for hiding this comment

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

How does this appear now when different grades are selected?

tooltip="Select to show student's aiming for {{grade}}"
tooltip-append-to-body="true"
class="text-{{$index == filterOptions.selectedGrade ? 'primary' : 'muted'}}"
Copy link
Member

Choose a reason for hiding this comment

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

As above

@macite macite merged commit eaa40e7 into doubtfire-lms:9.x Nov 4, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants