[#10917] Fix unexpected behaviour when reverting to template feedback path #10975

stapletonce · 2021-02-21T21:35:33Z

Outline of Solution

In the changeGiverRecipientType on line 240 of /web/app/components/question-edit-form/question-edit-form.component.ts, I added an if statement that checks if a custom feedback path is currently being used the visibility settings will not change when it is reverted to its template settings.

custom_feedback_visibility.mov

Derek-Hardy

@stapletonce One of the related e2e test has failed. It will be worthwhile to investigate whether to update e2e test workflow accordingly with latest change or to fix potential loophole in the current implementation.

It will be helpful to look into relevant parts of InstructorFeedbackEditPageE2ETest.java for debugging.

…10917-visibility-no-change-with-feedback-path-change

stapletonce · 2021-03-15T19:30:27Z

@Derek-Hardy Hello! I believe I have finished fixing this issue though I'm not sure if I have access to get the ongoing label off.

stapletonce · 2021-03-20T23:35:05Z

@madanalogy, @Derek-Hardy, @damithc This issue is no longer in progress and is ready for review (as far as I can tell!)

moziliar

Sorry for the late review. I roughly get the problem and your solution which fixes the issue of isUsingOtherFeedbackPath not being updated to true in this workflow. Can you help to address some of my doubts above?

src/web/app/components/question-edit-form/question-edit-form.component.ts

…10917-visibility-no-change-with-feedback-path-change

stapletonce · 2021-03-23T21:07:06Z

@moziliar I agree, both of those areas seem redundant to me. I have updated the code and re-performed the tests with those changes.

moziliar · 2021-03-28T12:39:52Z

Can I check what would happen if the user clicks custom feedback path again in the dropdown when the ui is already in custom feedback path? Logically, this code looks to me that it would just toggle the template back to default?

stapletonce · 2021-03-30T19:56:39Z

Screen.Recording.2021-03-30.at.3.55.43.PM.mov

It does not toggle back to default with the current changes @moziliar

moziliar · 2021-03-31T05:27:44Z

Yup, tested locally and the change seems to solve the problem partially.

Can you help to look into this problem too?

stapletonce · 2021-03-31T16:27:39Z

@moziliar What is the desired behavior here? I was under the impression that the visibility should only stay the same when switching between custom and template when the custom and template matched. In your example, you switch from Students in this course -> Giver (Self feedback) to Students in this course -> Giver's teammates so visibility resets.

moziliar · 2021-03-31T16:33:33Z

@stapletonce Sorry I didn't specify what I sensed wrong: notice how the first time I click to Giver's teammates template for the first time, the custom template part still stays, whereas the second time triggers the revert to the template display. The behaviour seems correct for people who are involved in fixing it, but might be confusing to new instructor as not much visual change has been rendered after the first click (the second is a lot more drastic than the first).

Maybe we could just make them the same as discussed in the issue, i.e. not in custom template mode at all:

User interface reverts to template display prior to selecting custom feedback path

stapletonce · 2021-03-31T16:41:35Z

@moziliar Ah, I see. Thank you for clearing this up - I can work on this now!

…er a custom feedback setting

…ath-change

moziliar · 2021-04-05T17:17:13Z

@stapletonce hi, is this PR ready for review?

stapletonce · 2021-04-05T17:18:04Z

yes please!

…

On Apr 5, 2021, at 1:17 PM, Mo Zongran ***@***.***> wrote: @stapletonce <https://github.com/stapletonce> hi, is this PR ready for review? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10975 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANJ3JJKR46XUH24ITOTMC43THHWCPANCNFSM4X7MQBXA>.

moziliar

One minor part. The rest look ok!

moziliar · 2021-04-05T17:22:51Z

src/web/app/components/question-edit-form/question-edit-form.component.ts

+    if (this.model.giverType === giverType && this.model.recipientType === newRecipientType) {
+      // do not reset the visibility settings if reverting feedback path to preset template provided
+      if (this.model.isUsingOtherFeedbackPath) {


It is perhaps not a good practice to have two such if's as they create an arrow-antipattern. You might want to just do

Suggested change

if (this.model.giverType === giverType && this.model.recipientType === newRecipientType) {

// do not reset the visibility settings if reverting feedback path to preset template provided

if (this.model.isUsingOtherFeedbackPath) {

if (this.model.giverType === giverType && this.model.recipientType === newRecipientType

&& this.model.isUsingOtherFeedbackPath) {

in which the precedence of the booleans achieves the exact same effect.

Wrong logic above^. Shouldn't omit the path where the function does nothing

…omponent.ts Co-authored-by: Mo Zongran <mzongran@comp.nus.edu.sg>

moziliar · 2021-04-05T17:37:18Z

Sorry I take that back... it seems like the logic is wrong in this case🥲 you can revert that and the rest LGTM

…10917-visibility-no-change-with-feedback-path-change

…f github.com:stapletonce/teammates into 10917-visibility-no-change-with-feedback-path-change

…t-form.component.ts" This reverts commit 3626115.

moziliar

LGTM. Wait for CI to complete

stapletonce · 2021-04-05T18:17:34Z

@moziliar success 🎉

stapletonce · 2021-04-06T16:06:00Z

@Derek-Hardy ready for review

Derek-Hardy

LGTM, thanks for the contribution.

…ath-change

Chloe Stapleton added 2 commits February 21, 2021 16:29

Fix unexpected behaviour when reverting to template feedback path

268bf9f

Fix minor linting errors

d0db372

stapletonce changed the title ~~[#10917] Fix unexpected behaviour when reverting to template feedback path~~ Work in Progress [#10917] Fix unexpected behaviour when reverting to template feedback path Feb 21, 2021

madanalogy added the s.Ongoing The PR is being worked on by the author(s) label Feb 22, 2021

Derek-Hardy reviewed Mar 1, 2021

View reviewed changes

Chloe Stapleton added 2 commits March 7, 2021 14:40

Merge branch 'master' of https://github.com/TEAMMATES/teammates into …

09388fb

…10917-visibility-no-change-with-feedback-path-change

Check if settings are same when reverting from custom to default

c99edac

stapletonce changed the title ~~Work in Progress [#10917] Fix unexpected behaviour when reverting to template feedback path~~ [#10917] Fix unexpected behaviour when reverting to template feedback path Mar 7, 2021

Derek-Hardy 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 Mar 21, 2021

moziliar requested changes Mar 23, 2021

View reviewed changes

src/web/app/components/question-edit-form/question-edit-form.component.ts Outdated Show resolved Hide resolved

src/web/app/components/question-edit-form/question-edit-form.component.ts Outdated Show resolved Hide resolved

Chloe Stapleton added 2 commits March 23, 2021 16:04

Merge branch 'master' of https://github.com/TEAMMATES/teammates into …

0889d76

…10917-visibility-no-change-with-feedback-path-change

Remove redundancies from if checks

8f4788d

stapletonce force-pushed the 10917-visibility-no-change-with-feedback-path-change branch from 24bb490 to 8f4788d Compare March 23, 2021 20:43

stapletonce requested review from moziliar and Derek-Hardy March 26, 2021 17:40

Chloe Stapleton and others added 2 commits March 31, 2021 12:59

Revert to default display after choosing common feedback settings aft…

c31b816

…er a custom feedback setting

Merge branch 'master' into 10917-visibility-no-change-with-feedback-p…

0df9eb9

…ath-change

moziliar requested changes Apr 5, 2021

View reviewed changes

Update src/web/app/components/question-edit-form/question-edit-form.c…

3626115

…omponent.ts Co-authored-by: Mo Zongran <mzongran@comp.nus.edu.sg>

Chloe Stapleton added 3 commits April 5, 2021 13:46

Merge branch 'master' of https://github.com/TEAMMATES/teammates into …

e324ce8

…10917-visibility-no-change-with-feedback-path-change

Merge branch '10917-visibility-no-change-with-feedback-path-change' o…

82b54aa

…f github.com:stapletonce/teammates into 10917-visibility-no-change-with-feedback-path-change

Revert "Update src/web/app/components/question-edit-form/question-edi…

90786d8

…t-form.component.ts" This reverts commit 3626115.

moziliar approved these changes Apr 5, 2021

View reviewed changes

moziliar added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Apr 6, 2021

Derek-Hardy approved these changes Apr 8, 2021

View reviewed changes

Merge branch 'master' into 10917-visibility-no-change-with-feedback-p…

9c62d84

…ath-change

Derek-Hardy assigned moziliar and Derek-Hardy Apr 8, 2021

madanalogy approved these changes Apr 12, 2021

View reviewed changes

madanalogy added c.Bug Bug/defect report s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Apr 12, 2021

madanalogy added this to the V7.15.0 milestone Apr 12, 2021

madanalogy self-assigned this Apr 12, 2021

Merge branch 'master' into 10917-visibility-no-change-with-feedback-p…

1268f00

…ath-change

madanalogy merged commit 0e533b2 into TEAMMATES:master Apr 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#10917] Fix unexpected behaviour when reverting to template feedback path #10975

[#10917] Fix unexpected behaviour when reverting to template feedback path #10975

stapletonce commented Feb 21, 2021

Derek-Hardy left a comment •

edited

stapletonce commented Mar 15, 2021

stapletonce commented Mar 20, 2021

moziliar left a comment

stapletonce commented Mar 23, 2021

moziliar commented Mar 28, 2021

stapletonce commented Mar 30, 2021

moziliar commented Mar 31, 2021

stapletonce commented Mar 31, 2021

moziliar commented Mar 31, 2021 •

edited

stapletonce commented Mar 31, 2021

moziliar commented Apr 5, 2021

stapletonce commented Apr 5, 2021 via email

moziliar left a comment

moziliar Apr 5, 2021

moziliar Apr 5, 2021 •

edited

moziliar commented Apr 5, 2021

moziliar left a comment

stapletonce commented Apr 5, 2021

stapletonce commented Apr 6, 2021

Derek-Hardy left a comment •

edited

[#10917] Fix unexpected behaviour when reverting to template feedback path #10975

[#10917] Fix unexpected behaviour when reverting to template feedback path #10975

Conversation

stapletonce commented Feb 21, 2021

Derek-Hardy left a comment • edited

Choose a reason for hiding this comment

stapletonce commented Mar 15, 2021

stapletonce commented Mar 20, 2021

moziliar left a comment

Choose a reason for hiding this comment

stapletonce commented Mar 23, 2021

moziliar commented Mar 28, 2021

stapletonce commented Mar 30, 2021

moziliar commented Mar 31, 2021

stapletonce commented Mar 31, 2021

moziliar commented Mar 31, 2021 • edited

stapletonce commented Mar 31, 2021

moziliar commented Apr 5, 2021

stapletonce commented Apr 5, 2021 via email

moziliar left a comment

Choose a reason for hiding this comment

moziliar Apr 5, 2021

Choose a reason for hiding this comment

moziliar Apr 5, 2021 • edited

Choose a reason for hiding this comment

moziliar commented Apr 5, 2021

moziliar left a comment

Choose a reason for hiding this comment

stapletonce commented Apr 5, 2021

stapletonce commented Apr 6, 2021

Derek-Hardy left a comment • edited

Choose a reason for hiding this comment

Derek-Hardy left a comment •

edited

moziliar commented Mar 31, 2021 •

edited

moziliar Apr 5, 2021 •

edited

Derek-Hardy left a comment •

edited