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

[#10397] Add frontend validation for feedback submission #10398

Merged
merged 8 commits into from
Aug 21, 2020

Conversation

madanalogy
Copy link
Contributor

@madanalogy madanalogy commented Jul 24, 2020

Fixes #10397

Jul-29-2020 17-13-44

@madanalogy madanalogy added the s.Ongoing The PR is being worked on by the author(s) label Jul 24, 2020
@madanalogy madanalogy 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 Jul 29, 2020
@madanalogy madanalogy marked this pull request as ready for review July 29, 2020 09:19
@madanalogy madanalogy added s.Ongoing The PR is being worked on by the author(s) and removed s.ToReview The PR is waiting for review(s) labels Jul 31, 2020
@madanalogy
Copy link
Contributor Author

madanalogy commented Jul 31, 2020

@xpdavid The idea I had was that once a constraint component was initialised in a question component, it would emit an Observable<boolean> to be tracked by the question. The question will then update the model whenever there is any change in the form by polling the observable to retrieve the latest isValid: boolean.

This design approach was my answer to "when should the constraint component emit the validity boolean?" because the constraint component doesn't have access to the parent question component's formModelChange.

@xpdavid
Copy link
Contributor

xpdavid commented Jul 31, 2020

You can make *-contrains component emit isValid event and let question-submission-form.component to change the model.

I think we are also facing a new issue here: There are constraints targeted to a group of responses and also a single response.

  • If your isValid is only defined for QuestionSubmissionFormModel, how about the individual response?
  • If your isValid is only defined for FeedbackResponseRecipientSubmissionFormModel, how about the group response?

@madanalogy
Copy link
Contributor Author

You can make *-contrains component emit isValid event and let question-submission-form.component to change the model.

Yes this is what I'm already doing, just that I'm using a single observable emit so the responsibility then becomes on the question component to poll the value whenever necessary, rather than the constraint component constantly emitting at some arbitrary benchmark.

I think we are also facing a new issue here: There are constraints targeted to a group of responses and also a single response.

  • If your isValid is only defined for QuestionSubmissionFormModel, how about the individual response?
  • If your isValid is only defined for FeedbackResponseRecipientSubmissionFormModel, how about the group response?

isValid here is only define for QuestionSubmissionFormModel because constraints are only defined on a per question basis. Individual responses can easily be validated within their corresponding response component or even on a per response basis prior to sending to the backend and even easily validated at the backend. My only purpose here is to validate the group response (i.e. per-question basis). I do not see individual response validation within the scope of this PR.

@xpdavid
Copy link
Contributor

xpdavid commented Aug 1, 2020

so the responsibility then becomes on the question component to poll the value whenever necessary

You don't need use EventEmitter<Observable<boolean>> to separate the responsibility. If the constraint component use EventEmitter<boolean>, it is still up to the question component to "listen" to the event or not.

In addition, your end goal is to update isValid attribute in the questionModal which can be easily achieved by EventEmitter<boolean>.

My only purpose here is to validate the group response (i.e. per-question basis). I do not see individual response validation within the scope of this PR.

I think this is not closed for modification. I think we can ask ourselves, how many lines of code shall we change if we want the logic (e.g., disable the submission button or pop up the status message when any response is invalid) for single response also?

@madanalogy
Copy link
Contributor Author

You don't need use EventEmitter<Observable<boolean>> to separate the responsibility. If the constraint component use EventEmitter<boolean>, it is still up to the question component to "listen" to the event or not.

In addition, your end goal is to update isValid attribute in the questionModal which can be easily achieved by EventEmitter<boolean>.

When should the constraint component emit the value then? It doesn't have access to the parent formModelChange. Unless I set up an emitter in the parent as well possibly?

I think this is not closed for modification. I think we can ask ourselves, how many lines of code shall we change if we want the logic (e.g., disable the submission button or pop up the status message when any response is invalid) for single response also?

Okay noted on this, I will think it through and see how I can make this work for both grouped responses and individual responses

@madanalogy madanalogy requested a review from xpdavid August 8, 2020 07:58
Copy link
Contributor

@xpdavid xpdavid left a comment

Choose a reason for hiding this comment

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

Almost there :)

# Conflicts:
#	src/web/app/pages-session/session-submission-page/session-submission-page.component.ts
Copy link
Contributor

@xpdavid xpdavid left a comment

Choose a reason for hiding this comment

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

💯

@madanalogy madanalogy 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 Aug 21, 2020
@madanalogy madanalogy merged commit af7995b into TEAMMATES:master Aug 21, 2020
@madanalogy madanalogy deleted the 10397-submission-validation branch August 21, 2020 02:38
@madanalogy madanalogy added this to the v7.0.1 milestone Aug 22, 2020
@madanalogy madanalogy added the c.Feature User-facing feature; can be new feature or enhancement to existing feature label Aug 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Feedback submission propagates invalid question responses
4 participants