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

Jump to bottom

[#12652] Copying feedback session: mark session name as mandatory field when copying feedback session #12670

Merged
merged 7 commits into from
Dec 23, 2023
Merged

[#12652] Copying feedback session: mark session name as mandatory field when copying feedback session #12670

merged 7 commits into from
Dec 23, 2023

Conversation

andreiyusupau
Copy link
Contributor

Fixes #12652

Outline of Solution

Added the required tag to input for Name of copied session.
Added div with exclamation mark that is shown, form is submitted with empty Name of copied session field.
Added * to label of Name of copied session.

@github-actions GitHub Actions
Copy link

Hi @andreiyusupau, 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.

@andreiyusupau andreiyusupau changed the title [#12652 ] Copying feedback session: mark session name as mandatory field when copying feedback session [#12652] Copying feedback session: mark session name as mandatory field when copying feedback session Dec 18, 2023
@weiquu
Copy link
Contributor

weiquu commented Dec 19, 2023

Hi @andreiyusupau, do fix the failing snapshot tests before we review the PR. You can update the snapshots by running npm run test and pressing a to run all test cases. After that, check through the snapshots to make sure the changes are as expected, then press u to update them. You can find more details on snapshot testing here.

(Also, I think lines 15-19 of copy-session-modal.component.html has extra indent.)

weiquu
weiquu requested changes Dec 20, 2023
View reviewed changes
Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

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

Small nit, otherwise looks great!

src/web/app/components/copy-session-modal/copy-session-modal.component.html Outdated Show resolved Hide resolved
weiquu
weiquu approved these changes Dec 20, 2023
View reviewed changes
Copy link
Contributor

@weiquu weiquu left a comment

Choose a reason for hiding this comment

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

LGTM

@weiquu weiquu added the s.FinalReview The PR is ready for final review label Dec 20, 2023
domlimm
domlimm approved these changes Dec 23, 2023
View reviewed changes
Copy link
Contributor

@domlimm domlimm left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for your contribution!

@domlimm
Copy link
Contributor

domlimm commented Dec 23, 2023 •
edited
Loading

@weiquu Hello! Some things for us to note in the future..

  1. Since [#8931] Instructor: Feedback Session: Mark mandatory fields for creating and editing session #12617 has a similar set of code, we could extract it into a component, not sure if it's worth the trouble.
  2. I realised that if we add whitespace into the textbox, we're also able to copy the course.
    Here there is a whitespace added:
    image
    After clicking on Copy:
    image

@domlimm domlimm merged commit 08a6195 into TEAMMATES:master Dec 23, 2023
9 checks passed
@weiquu
Copy link
Contributor

weiquu commented Dec 23, 2023

Good catch @domlimm! If you're free, could you open a new issue for it? If not, I'll get to it next week (:

For the whitespace one, I think we need a custom validator (see here and here) - if you agree, maybe we can include these links as suggestions in the issue

@domlimm domlimm mentioned this pull request Dec 24, 2023
Open
@ArunErram
Copy link

hi can i work on it please

@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging c.Feature User-facing feature; can be new feature or enhancement to existing feature and removed s.FinalReview The PR is ready for final review labels Jan 21, 2024
@wkurniawan07 wkurniawan07 modified the milestones: V8.29.0, V8.30.0 Jan 21, 2024
cedricongjh pushed a commit to cedricongjh/teammates that referenced this pull request Feb 20, 2024
@andreiyusupau @domlimm @cedricongjh
[TEAMMATES#12652] Copying feedback session: mark session name as man…
101c476 
…datory field when copying feedback session (TEAMMATES#12670)

* Fix Copying feedback session: mark session name as mandatory field when copying feedback session TEAMMATES#12652

* Fix Copying feedback session: mark session name as mandatory field when copying feedback session TEAMMATES#12652

* Fix Copying feedback session: mark session name as mandatory field when copying feedback session TEAMMATES#12652

---------

Co-authored-by: Dominic Lim <46486515+domlimm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weiquu weiquu weiquu approved these changes

@domlimm domlimm domlimm approved these changes

@cedricongjh cedricongjh Awaiting requested review from cedricongjh

Assignees
No one assigned
Labels
c.Feature User-facing feature; can be new feature or enhancement to existing feature s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V8.30.0
Development

Successfully merging this pull request may close these issues.

Copying feedback session: mark session name as mandatory field when copying feedback session
5 participants
@andreiyusupau @weiquu @domlimm @ArunErram @wkurniawan07