Navigation Menu

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

[#9255] Migrate part of instructor feedback sessions page to Angular #9345

Merged

Conversation

xpdavid
Copy link
Contributor

@xpdavid xpdavid commented Jan 20, 2019

Part of #9255

teammates online peer feedback evaluation system for student team projects 5

Some components added are also shared by instructor home page also. I added them to the instructor home page.

teammates online peer feedback evaluation system for student team projects 6

@jacoblipech Less work for you.

@wkurniawan07 wkurniawan07 self-assigned this Jan 20, 2019
@wkurniawan07 wkurniawan07 added this to Ongoing in Front-end and RESTful back-end Migration via automation Jan 20, 2019
@wkurniawan07 wkurniawan07 added this to the V7.0.0-alpha.0 milestone Jan 20, 2019
@wkurniawan07 wkurniawan07 self-requested a review January 20, 2019 14:48
@wkurniawan07
Copy link
Member

@xpdavid need to resolve the conflicts.

@wkurniawan07 wkurniawan07 added the s.Ongoing The PR is being worked on by the author(s) label Jan 20, 2019
Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

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

Mostly there. Also, it looks like the sort table function is buggy? When I tried, only one column (i.e. where I pressed the icon) gets sorted.

@@ -380,10 +432,32 @@ public void validate() {
}

/**
* The basic request body format for saving of feedback session.
* The request body format for saving of feedback session.
*/
public static class FeedbackSessionSaveRequest extends FeedbackSessionBasicRequest {
Copy link
Member

Choose a reason for hiding this comment

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

If by now this has not been implemented, what is the usage of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used in SaveFeedbackSessionAction. I just fix the typo I introduced in the previous PR :P

<div class="form-group">
<label><b>Name for copied session</b></label>
<input type="text" class="form-control" [(ngModel)]="newFeedbackSessionName" maxlength="38">
<span>{{ 38 - newFeedbackSessionName.length }} characters left</span>
Copy link
Member

Choose a reason for hiding this comment

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

Magic number 38?

case FeedbackSessionSubmissionStatus.OPEN:
case FeedbackSessionSubmissionStatus.GRACE_PERIOD:
case FeedbackSessionSubmissionStatus.CLOSED:
msg += 'The feedback session has been created, is visible';
Copy link
Member

Choose a reason for hiding this comment

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

Will this not cause duplicate The feedback session has been created?

this.httpRequestService.post('/session/remind/submission', paramMap).subscribe(() => {
this.statusMessageService.showSuccessMessage(
'Reminder e-mails have been sent out to those students and instructors. '
+ 'Please allow up to 1 hour for all the notification emails to be sent out..');
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate . at the end.

/**
* Template sessions.
*/
export const templateSessions: TemplateSession[] = [
Copy link
Member

Choose a reason for hiding this comment

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

This should be a JSON file instead? This is a form of data, not application code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need interface TemplateSession {} defined somewhere. So I thought put them together would be more organised.

Copy link
Member

Choose a reason for hiding this comment

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

More organized is one thing. The issue here is you're mixing up application code and application data (independent of logic).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. One problem of serialised it to JSON file is that we will loss reference to enum type (e.g. FeedbackVisibilityType.RECIPIENT will become "RECIPIENT") Not sure whether it is a bit problem or not for project evolution.

@xpdavid
Copy link
Contributor Author

xpdavid commented Jan 22, 2019

Also, it looks like the sort table function is buggy? When I tried, only one column (i.e. where I pressed the icon) gets sorted.

Only one sorting criteria is applied each time (i.e. cannot sort by Course ID first and then Session Name). That is also the functionality in V6.

@xpdavid xpdavid added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Jan 24, 2019
Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

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

LGTM for now. Another job well done! 💯

@wkurniawan07 wkurniawan07 merged commit de975cf into TEAMMATES:master Jan 24, 2019
Front-end and RESTful back-end Migration automation moved this from Ongoing to Done Jan 24, 2019
@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.ToReview The PR is waiting for review(s) labels Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants