instructorFeedbackSessionEditQuestion: visibility options don't match the selected recipient

[damithc]

From dam...@gmail.com on February 22, 2014 16:57:34

If the recipient is already selected (based on previous question), the visibility options don't change. seems it is tied to 'select' event of the dropbox.

Original issue: http://code.google.com/p/teammatespes/issues/detail?id=1635

[damithc]

From franklin...@gmail.com on March 14, 2014 08:17:35

can you elaborate more on this issue? I was testing this on local server, I am not sure if I get what you mean. can you like append two screenshot here?

[damithc]

From dam...@gmail.com on March 14, 2014 08:44:05

When you create a new question, the giver/receiver is pre-selected based on the previous question. However, the visibility options do not change accordingly. For example, if the receiver is 'nobody specific', one row of the visibility options should disappear, but it doesn't. Got it?

[damithc]

From franklin...@gmail.com on March 14, 2014 19:28:10

Some points to clarify:

1. For giver(self feedback), other students in the course, giver's team members and giver, the instructor who is currently editing can see 5 rows. For instructor in the course, other teams in the course, giver's team, giver's team members, s/he can see 4 rows. For nobody specific, s/he can see only 3 rows. Is this the case?
2. If the previous question's recipient is set to be "nobody" specific, now you want only 3 rows to be displayed instead of 5 rows and for 4-row option, it just displays 4 rows. Is that the effect we want to achieve?
3. I notice that if the previous question was selected as "nobody specific" and the current question will have 5 rows--even if I select a row or two rows that are not supposed to be shown when "nobody specific", I will still be able to create the question successfully. is that intended or is it a bug?

[damithc]

From dam...@gmail.com on March 15, 2014 00:08:37

Your point 3 is the bug we are trying to fix. :-)

[damithc]

From franklin...@gmail.com on March 15, 2014 00:40:43

so if I want to do this issue, I need to work to get point 2 done right?

[damithc]

From franklin...@gmail.com on March 15, 2014 00:51:30

I mean if the rows are hidden properly, then do we still need to verify in back-end that the number of rows match selected recipients?

[damithc]

From dam...@gmail.com on March 15, 2014 00:54:16

Not quite clear what your point 1,2 say. What we want is to pre-hide the visibility to match the pre-selected giver/receiver. Currently, hiding works when the option is selected manually, but doesn't work if the options is preselected when the form appears for the first time. I'm guessing the function to hide irrelevant visibilities is tied to the 'select' event of the list box. When it is pre-selected, the event is not fired and the function is not activated. Just a guess.

[damithc]

From dam...@gmail.com on March 15, 2014 01:16:39

The backend should check data correctness in all cases. We should not depend on the front end to submit correct data. But the hiding of options here is for user convenience, nor for correctness reasons.

[damithc]

From franklin...@gmail.com on March 15, 2014 01:42:50

I have just used js to toggle the visibility of rows successfully--running tests now and I see this is not hard. (if no test fails, that is a lot of more work then since I have to write a test case about this that fails the default but lets my code pass)

previously, I think I saw the external behavior suggests that back-end checking for this is not done, although I did not look at the code yet. let us say I have to write the back-end checking part, there are two possibilities I can think of: 1) msg saying the question is not "legal" 2)based on giver&recipient's option, ignore the row(s) passed in as params and see them as "unchecked". which way is preferred?

[damithc]

From franklin...@gmail.com on March 15, 2014 01:46:10

and if our back-end checking is Ok and we have tests for back-end checking, do we need to do a ui test in which we create a "Nobody Specific" question first and then verify and only 3 rows are visible?

[damithc]

From dam...@gmail.com on March 15, 2014 01:49:15

Preferably, yes. If we had such a test, we wouldn't be having this bug right now :-)

[damithc]

From franklin...@gmail.com on March 15, 2014 02:05:56

https://codereview.appspot.com/76410043/ the test is not done yet. I think a core developer may verify this first--if this is not exactly the effect we want, I will change the code first until I achieve the effect and then I will finish the test. btw, it back-end checking part of this issue? can we just separate that from this(if the back-end does count, why this is just "Medium"--i will just finish back-end checking if this is part of this issue however)

[damithc]

From dam...@gmail.com on March 15, 2014 02:08:17

Yes, backend checking can be done separately.

Owner: franklin...@gmail.com
Cc: arnold.k...@gmail.com
Labels: Reviewer-Arnold

[damithc]

From franklin...@gmail.com on March 15, 2014 03:35:47

https://codereview.appspot.com/76410043/ the test is not done yet. I think Arnold could verify this first--if this is not exactly the effect we want, I will change the code first until I achieve the effect and then I will finish the test.

Status: ReadyForReview

[damithc]

From arnold.k...@gmail.com on March 16, 2014 22:27:34

Yes, I think that is the intended effect.

One thing though, when editing an existing question and viewing the visibility options, all 5 rows are displayed regardless of the giver/recipient type, which I think is the same bug.

Unless this is deliberate?

Status: ChangesRequested
Cc: dam...@gmail.com

[damithc]

From franklin...@gmail.com on March 16, 2014 22:32:15

Okay. I see. (I thought this issue is only about new question & previous question) Then I will fix that issue with the editing exiting question and start writing UI tests?

[damithc]

From arnold.k...@gmail.com on March 16, 2014 22:38:24

Yes, that's right

[damithc]

From franklin...@gmail.com on March 18, 2014 23:10:19

https://codereview.appspot.com/76410043/ Issue updated with tests created

[damithc]

From franklin...@gmail.com on March 19, 2014 00:44:27

Status: ReadyForReview

[damithc]

From arnold.k...@gmail.com on March 19, 2014 20:05:01

Junchao, can you merge and reupload? I think the changes clash with a recent commit.

Status: ChangesRequested

[damithc]

From franklin...@gmail.com on March 19, 2014 21:56:49

Yesterday it did not. The commit is new. Kai's part is somehow overlapping with my part(just part of it). And we are editing on the same files of the project

[damithc]

From arnold.k...@gmail.com on March 19, 2014 22:11:55

Yes, that is the reason.

[damithc]

From franklin...@gmail.com on March 19, 2014 22:24:06

https://codereview.appspot.com/76410043/

Status: ReadyForReview
Cc: -dam...@gmail.com

[damithc]

From arnold.k...@gmail.com on March 19, 2014 23:49:05

Added comments. Do take note of formatting and indentation

Status: ChangesRequested

[damithc]

From franklin...@gmail.com on March 20, 2014 08:53:52

https://codereview.appspot.com/76410043/

Status: ReadyForReview

[damithc]

From arnold.k...@gmail.com on March 20, 2014 20:30:52

This issue was updated by revision 4b2a103dded3 .

Status: Delivered

[damithc]

From dam...@gmail.com on March 22, 2014 05:50:54

Status: Deployed
Labels: Milestone-V4.93