[#12653] Copy course modal: Mandatory fields not highlighted by XiangyuTan-learning · Pull Request #13180 · TEAMMATES/teammates

XiangyuTan-learning · 2024-10-18T03:52:54Z

Fixed validation issues in the "Copy Course" feature to ensure that users properly fill in all required fields (Course ID and Course Name) before submitting the form. Previously, there was a key issue where the validation messages did not update immediately when users cleared the input fields, leading to a suboptimal user experience.

changes have been made:

HTML File Changes:
1.1 Added red error messages below the Course ID and Course Name input fields:
Displays "The field Course ID should not be empty" when the Course ID is empty.
Displays "The field Course Name should not be empty" when the Course Name is empty.
1.2 Used the (ngModelChange) event to monitor user input to ensure that error messages are displayed immediately when an input field is cleared
TypeScript File (copy-course-modal.component.ts) Changes:
2.1 Added courseIdEmptyError and courseNameEmptyError variables to track the error state of the Course ID and Course Name input fields.
Modified the ngOnInit() method to initialize these error states.
2.2 Added onCourseIdChange(value: string) and onCourseNameChange(value: string) methods to update the error state in real-time.
2.3 Modified the copy() method to check the input values when the user clicks the "Copy" button. If any required field is empty, the copy logic is blocked, and an error message is displayed.

New UI afterwards:

github-actions · 2024-10-18T03:53:08Z

Hi @XiangyuTan-learning, thank you for your interest in contributing to TEAMMATES!
However, your PR does not appear to follow our contribution guidelines:

Title must start with the issue number the PR is fixing in square brackets, e.g. [#<issue-number>]

Please address the above before we proceed to review your PR.

XiangyuTan-learning · 2024-10-18T04:01:15Z

Anyone knows how to solve this title format issue? I believe I have definitely used the Square Bracket for the issue number, any suggestions appreciated.

XiangyuTan-learning · 2024-10-18T04:14:30Z

And how to add label, seem like just type in s.ToReview?

damithc · 2024-10-18T04:52:28Z

@XiangyuTan-learning Thanks for the PR. Note that the TEAMMATES currently in low-activity phase (the dev team is busy with some other tasks). So, PRs are unlikely to get speedy responses for a while. We expect to resume active development around Jan 2025.

XiangyuTan-learning · 2024-10-18T05:15:12Z

@damithc Thank you for your reply, can you please give me some advice about how can I solve the title format issue and Component Tests? thanks

damithc · 2024-10-18T07:00:15Z

@damithc Thank you for your reply, can you please give me some advice about how can I solve the title format issue and Component Tests? thanks

Seems to be OK now.

XiangyuTan-learning · 2024-10-18T07:03:43Z

@damithc Thank you for your reply, can you please give me some advice about how can I solve the title format issue and Component Tests? thanks感谢您的回复，您能否给我一些关于如何解决标题格式问题和组件测试的建议？谢谢

Seems to be OK now.现在似乎没问题。
I have add some unit testing for the changing I made, and I have passed the static analysis
./gradlew int and npm run lint in my laptop. and pass the npm test -- -u as well. Hoping this can ensure the consistency.

XiangyuTan-learning · 2024-10-18T07:19:10Z

Can anyone give me some help about the Component Test for ubuntu, I don't know much about this issue, any suggestions really appreciated!

Copilot

Pull request overview

Improves the “Copy Course” modal validation UX by adding inline required-field warnings for Course ID and Course Name, aiming to make mandatory fields more obvious and update warnings in real time.

Changes:

Added courseIdEmptyError / courseNameEmptyError flags and onCourseIdChange / onCourseNameChange handlers to track empty-field state.
Updated the modal template to show inline “should not be empty” messages and apply invalid styling.
Updated unit tests and snapshots to reflect the new validation state/UI.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.

File	Description
`src/web/app/components/copy-course-modal/copy-course-modal.component.ts`	Adds empty-field state flags, real-time change handlers, and updates `copy()` validation gating.
`src/web/app/components/copy-course-modal/copy-course-modal.component.html`	Renders new inline error messages and hooks `ngModelChange`/focus behavior into validation UI.
`src/web/app/components/copy-course-modal/copy-course-modal.component.spec.ts`	Adds/updates tests for new error flags and UI rendering.
`src/web/app/components/copy-course-modal/__snapshots__/copy-course-modal.component.spec.ts.snap`	Updates snapshots to include the new component fields.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-23T17:34:44Z

src/web/app/components/copy-course-modal/copy-course-modal.component.ts

+    this.courseIdEmptyError = !this.newCourseId;
+    this.courseNameEmptyError = !this.newCourseName;
+
+    if (this.courseIdEmptyError || this.courseNameEmptyError) {
      return;
    }



copy() uses truthiness checks (!this.newCourseId / !this.newCourseName) while onCourseIdChange/onCourseNameChange use trim(). This inconsistency allows whitespace-only values (e.g., ' ') to bypass copy() validation and proceed to conflict checks / modal close with an effectively empty ID/name. Use a consistent trimmed validation in copy() (and ideally normalize the stored values) so whitespace-only input is treated as empty everywhere.

Suggested change

this.courseIdEmptyError = !this.newCourseId;

this.courseNameEmptyError = !this.newCourseName;

if (this.courseIdEmptyError || this.courseNameEmptyError) {

return;

}

const trimmedCourseId: string = this.newCourseId.trim();

const trimmedCourseName: string = this.newCourseName.trim();

this.courseIdEmptyError = trimmedCourseId.length === 0;

this.courseNameEmptyError = trimmedCourseName.length === 0;

if (this.courseIdEmptyError || this.courseNameEmptyError) {

return;

}

// Normalize stored values to trimmed versions to ensure consistent handling.

this.newCourseId = trimmedCourseId;

this.newCourseName = trimmedCourseName;

Copilot · 2026-01-23T17:34:44Z

src/web/app/components/copy-course-modal/copy-course-modal.component.html

+        <input [class.invalid]="newCourseIdIsConflicting || courseIdEmptyError" id="copy-course-id" type="text" class="form-control" placeholder="e.g. CS3215-2013Semester1"
+          [(ngModel)]="newCourseId" [maxlength]="COURSE_ID_MAX_LENGTH" (focus)="this.newCourseIdIsConflicting = false; courseIdEmptyError = false" (ngModelChange)="onCourseIdChange($event)">
        <span>{{ COURSE_ID_MAX_LENGTH - newCourseId.length }} characters left</span>
+        <div *ngIf="courseIdEmptyError" class="text-danger">
+          <i class="fa fa-exclamation-circle" aria-hidden="true"></i>The field Course ID should not be empty.</div>


The new empty-field warnings rely on ngModelChange and also clear the flags on (focus). If a user clicks into an empty field and then clicks away without typing, ngModelChange will not fire, so the required-field warning still won’t appear (the original issue asks for warning on click/leave-empty). Consider switching to the existing template-driven validation pattern used elsewhere (e.g., required + #...="ngModel" + show when touched/dirty), or at least validate on (blur) instead of clearing errors on focus.

Copilot · 2026-01-23T17:34:44Z

src/web/app/components/copy-course-modal/copy-course-modal.component.html

        <label>Course Name:</label>
        <input id="copy-course-name" class="form-control" type="text" placeholder="e.g. Software Engineering" [(ngModel)]="newCourseName"
-          [maxlength]="COURSE_NAME_MAX_LENGTH"/>
+          [maxlength]="COURSE_NAME_MAX_LENGTH" (focus)="courseNameEmptyError = false" (ngModelChange)="onCourseNameChange($event)"/>


Course Name is missing the same visual invalid styling applied to Course ID. Course ID toggles the .invalid border via [class.invalid], but Course Name only shows the message. If the goal is to highlight mandatory fields consistently, apply the same invalid styling to the Course Name input when courseNameEmptyError is true.

Suggested change

[maxlength]="COURSE_NAME_MAX_LENGTH" (focus)="courseNameEmptyError = false" (ngModelChange)="onCourseNameChange($event)"/>

[maxlength]="COURSE_NAME_MAX_LENGTH" [class.invalid]="courseNameEmptyError" (focus)="courseNameEmptyError = false" (ngModelChange)="onCourseNameChange($event)"/>

Copilot · 2026-01-23T17:34:45Z

src/web/app/components/copy-course-modal/copy-course-modal.component.html

+        <div *ngIf="courseIdEmptyError" class="text-danger">
+          <i class="fa fa-exclamation-circle" aria-hidden="true"></i>The field Course ID should not be empty.</div>


This modal adds required-field warnings using .text-danger plus manual boolean flags. Elsewhere in the codebase, required-field warnings are typically driven by ngModel validity/touched state and rendered with the invalid-field pattern (e.g., src/web/app/components/copy-session-modal/copy-session-modal.component.html:15-18). Aligning with that convention would reduce custom state/handlers and keep the UX consistent.

Copilot · 2026-01-23T17:34:45Z

src/web/app/components/copy-course-modal/copy-course-modal.component.spec.ts

+      fixture.detectChanges();
+
+      component.newCourseName = '';
+      component.onCourseNameChange(component.newCourseId);


Test bug: onCourseNameChange is invoked with component.newCourseId instead of component.newCourseName, so the test can pass even if the Course Name change handler is broken. Pass the cleared course name value to actually validate the intended behavior.

Suggested change

component.onCourseNameChange(component.newCourseId);

component.onCourseNameChange(component.newCourseName);

Copilot · 2026-01-23T17:34:45Z

src/web/app/components/copy-course-modal/copy-course-modal.component.spec.ts

+    const errorMessage = fixture.debugElement.query(By.css('.text-danger'));
+
+      // Verify that the error message element exists and contains the correct text
+    expect(errorMessage).toBeTruthy(); // Ensure the element is present
+    expect(errorMessage.nativeElement.textContent).toContain('The field Course ID should not be empty.');


These tests query the first .text-danger element in the template, which becomes ambiguous if another .text-danger is added or both errors render. Prefer a more specific selector (e.g., an id/data-testid for each error message, or selecting relative to #copy-course-id / #copy-course-name) to avoid false positives/negatives.

Copilot · 2026-01-23T17:34:45Z

src/web/app/components/copy-course-modal/copy-course-modal.component.ts

+    * Real time check user input, triggered by ngModelChange
+    */
+  onCourseIdChange(value: string): void {
+    this.courseIdEmptyError = !value.trim();
+   }


Minor formatting/JSdoc issue: the new comment block has inconsistent indentation and the onCourseIdChange closing brace is misaligned compared to surrounding methods. Please format this block to match the file’s existing style (helps keep diffs clean and avoids potential formatter/lint failures).

Suggested change

* Real time check user input, triggered by ngModelChange

*/

onCourseIdChange(value: string): void {

this.courseIdEmptyError = !value.trim();

}

* Real time check user input, triggered by ngModelChange

*/

onCourseIdChange(value: string): void {

this.courseIdEmptyError = !value.trim();

}

mingyuancode · 2026-01-23T17:36:00Z

@XiangyuTan-learning I believe copliot has made some goodpoints, do look through and let me know what you think

samuelfangjw · 2026-02-03T18:29:07Z

It looks like this issue is already closed, so I'm going to close this PR. Thanks for taking the time to contribute!

Error message will be thrown when null input

c73e52f

XiangyuTan-learning changed the title ~~[#12653]Copy course modal: Mandatory fields not highlighted~~ [#12653] Copy course modal: Mandatory fields not highlighted Oct 18, 2024

Test implementation

1605d40

XiangyuTan-learning and others added 4 commits October 24, 2024 09:34

Add red exclamation mark

dee83dd

Merge branch 'master' into 12653-Copy_fields_not_highlighted

fd5fd46

Merge branch 'master' into 12653-Copy_fields_not_highlighted

3e7c4b6

Merge branch 'master' into 12653-Copy_fields_not_highlighted

9872809

mingyuancode requested a review from Copilot January 23, 2026 17:26

Copilot started reviewing on behalf of mingyuancode January 23, 2026 17:26 View session

Copilot AI reviewed Jan 23, 2026

View reviewed changes

samuelfangjw closed this Feb 3, 2026

-    this.courseIdEmptyError = !this.newCourseId;
-    this.courseNameEmptyError = !this.newCourseName;
-    if (this.courseIdEmptyError || this.courseNameEmptyError) {
-      return;
-    }
+    const trimmedCourseId: string = this.newCourseId.trim();
+    const trimmedCourseName: string = this.newCourseName.trim();
+    this.courseIdEmptyError = trimmedCourseId.length === 0;
+    this.courseNameEmptyError = trimmedCourseName.length === 0;
+    if (this.courseIdEmptyError || this.courseNameEmptyError) {
+      return;
+    }
+    // Normalize stored values to trimmed versions to ensure consistent handling.
+    this.newCourseId = trimmedCourseId;
+    this.newCourseName = trimmedCourseName;

	[maxlength]="COURSE_NAME_MAX_LENGTH" (focus)="courseNameEmptyError = false" (ngModelChange)="onCourseNameChange($event)"/>
	[maxlength]="COURSE_NAME_MAX_LENGTH" [class.invalid]="courseNameEmptyError" (focus)="courseNameEmptyError = false" (ngModelChange)="onCourseNameChange($event)"/>

		<div *ngIf="courseIdEmptyError" class="text-danger">
		<i class="fa fa-exclamation-circle" aria-hidden="true"></i>The field Course ID should not be empty.</div>

	component.onCourseNameChange(component.newCourseId);
	component.onCourseNameChange(component.newCourseName);

Conversation

XiangyuTan-learning commented Oct 18, 2024

Uh oh!

github-actions bot commented Oct 18, 2024

Uh oh!

XiangyuTan-learning commented Oct 18, 2024

Uh oh!

XiangyuTan-learning commented Oct 18, 2024

Uh oh!

damithc commented Oct 18, 2024

Uh oh!

XiangyuTan-learning commented Oct 18, 2024

Uh oh!

damithc commented Oct 18, 2024

Uh oh!

XiangyuTan-learning commented Oct 18, 2024

Uh oh!

XiangyuTan-learning commented Oct 18, 2024

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

mingyuancode commented Jan 23, 2026

Uh oh!

samuelfangjw commented Feb 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants