[somiaj]

When viewing a problem in editMode (such as viewing a problem from the problem editor with the "Open in new window" checked) don't use the past answers from the effectiveUser if the seeds do not match. This prevents showing past answers if the seed is different, but past answers could be shown if the seeds match for debugging a problem.

Hide the student navigation bar and problem grader when in editMode. It could be the wrong seed or even a temporary edit that hasn't been saved, and could be confusing to have the problem grader shown in this case.

Prevent submitting/recording answers when in edit mode. This is to prevent submitting answers for wrong seeds for you or for students if that permission is configured.

Always include the editMode hidden input when in editMode (not just for temporary files) so a check/preview/show correct form submit does not loose track of being in editMode.

This fixes some issues I ran across when testing editing seeds while acting as users. Overall it felt that when viewing a problem in edit mode the above should be adjusted. It could be that I went too far, or more could be done when when viewing a problem in its own window while editing it.

[drgrice1]

I think it may be time to remove the "Open in new window" option from the PG editor. The code in Problem.pm for this is getting to be a mess. Furthermore, it is getting to be dangerous. If something is missed in development it could easily become the case that student data could modified when an instructor uses this when acting as a student. This is not even really a good feature of the PG problem editor. The problem can already be viewed by the instructor in the set after editing the problem. That is a much safer way for this to be done.

[drgrice1]

I am not sure about this. The thing is that if a problem has been edited, the saved data might not be valid anymore even if the seed has not changed. The answers that are saved might not even match up with the questions the problem now has. So maybe the saved data should not be used at all when viewing a problem in edit mode.

[somiaj]

I think that is probably a reasonable approach. I don't use saved data so unsure if anyone would find it useful if trying to debug a problem or not.

[drgrice1]

You say that you do not use saved data, but you do. Saved data here refers to the problem_data in addition to submitted answers. Everyone uses submitted answers.

[drgrice1]

I don't like the repeated case from earlier in this massively nested ternary expression.

[somiaj]

I didn't either, but not repeating it required doing a test multiple times.

($prEnabled && !$problem->{prCount}) || (!$useSavedData && !defined $formFields->{problem_data})
? (problemData => '{}')
: ($can{checkAnswers} && defined $formFields->{problem_data})
? (problemData => $formFields->{problem_data})
: ()

Though a defined test is probably quick, I didn't think that cleared things up.

[somiaj]

I'm okay with removing the new window feature altogether. Now that the preview window can be resized easily in the editor, it provides a nice way to view it at different window sizes so needing a new window isn't needed. And as you stated, once the problem is in a set, better to just view it in the instructor's set. I was just surprised when testing this with the seed changes that answers were included in the first place when editing a live problem.

There is a rare use case I would have liked to know this was possible. I have been debugging a problem where a student did have an answer I wanted to be marked correct, so being able to use the student's answer vs typing it in each time could have been useful. But this is a very limited use case and probably not worth keeping this feature around. Typing in answers and checking them works just fine in the editor.

[drgrice1]

This pull request is certainly needed. However, looking into this I see that there are a host of related issues with the WeBWork::ContentGenerator::Problem and WeBWorK::ContentGenerator::ShowMeAnother packages in relation to using the "Open in new window" from the problem editor as well as just generally acting as a student. There are numerous ways that an instructor can inadvertently modify student data in the database when acting as a student depending on which optional modules are in use, as well as some things that just do not work right.

[somiaj]

I added a second commit that goes a lot further. This starts out by doing an early validity check/permission check with either editMode or Undefined_Set is used. This way future tests can just check for editMode directly and not have to compare the setID against Undefined_Set. I also prevent creating a set whose ID is Undefined_Set.

I assumed that editMode should mostly only show what an Undefined_Set would see, so the nav bar, siblings links, past answers, additional messages about number of attempts used, etc are all hidden. If an instructor wants to see these, they should view the set outside of editMode. In effect editMode focuses mostly on just being able to view a problem outside of an iframe. Problem specific stuff is mostly disabled. There are a few messages still around such as is the set visible/hidden. Read the commit message for a more detail description of changes.

There were some existing bugs that I fixed. Some links to Undefined_Set were broken, and a TA who entered in a path to the Undefined_Set would get errors because they lack some permissions.

Things not fixed here, but should be considered:

• The show_me_another path never goes through checkSet. This means it could be used to view stuff that is otherwise restricted.
• The GatewayQuiz module has handling for Undefined_Set, but doesn't work with editMode. It should probably have similar changes be made. I left this alone for now (let's review just Problem to start with). It could also be the solution here is don't let tests show an Undefined_Set and just open problem in a new window always use the Problem route so there are not multiple pages that need to deal with this extra handling.

I have still opted to allow using the student's past answers if the problem seed matches. I think this has a rare use case when if trying to fix a bug in a problem using the student's answers vs having to type them in each time. If others think this is not worth allowing, this can be removed too.

[somiaj]

I went another step further. checkSet was to liberal with allwoing Undefined_Set, which is what breaks going to the ProblemSet page with that setID. Instead only allow Undefined_Set to be used on very specific routes (this is also useful if show_me_another route is added to checkSet). I still disable the links, but now there won't be an error message if manually going to the Undefined_Set link.

[somiaj]

I went ahead and added show_me_another to checkSet. It probably should have been there in the first place.

[drgrice1]

The more that I look at this the more that I think the "Open in New Window" option for the problem editor needs to be removed. Furthermore, the "Try it" button for problems in the library browser needs to be removed. That is a feature that is no longer needed. It used to be the case that when problems were rendered in the library browser they were inert. There was not way to submit answers or actually work the problems. That is no longer the case, and the problems are fully functional directly in the library browser. So opening the problem in a new window to "try it" is no longer useful.

[drgrice1]

Other than the permissions issue, the changes look good.

[drgrice1]

This changes the permission for this. The permission used before was access_instructor_tools, not modify_problem_sets. I don't think that the modify_problem_sets permission is correct for this either. It should be changed back.

The problem editor requires both permissions and the library browser requires the modify_problem_sets permission. Usually someone with the modify_problem_sets permission will have the access_instructor_tools permission (it doesn't really make sense otherwise). This particular case does not give someone access to modify a set, and so it really should just be the lesser permission.

[somiaj]

This change was due to a possible issue Claude found with TA's who can access_instructor_tools but cannot modify_problem_sets. Without this change there was some extra logic needed in Problem.pm to deal with a TA getting a link to view an Undefined_Set. I will go look at this again, but I made this change to avoid the issue with extra checking for TAs and the modify_problem_sets permission, as I didn't see a use case where a TA should be able to view a problem in an Undefined_Set.

[somiaj]

I can change this back. I think we should check both access_instructor_tools and modify_problem_sets here (I used the assumption that modify_problem_sets permission would also include access_instructor_tools). Then modules that use the Undefined_Set no longer have to double check permissions, and the error message is more about the set does not exist, instead of a permissions error. Another option would be to add a message in invalidSet here that gets passed through about incorrect permissions.

The issue I ran into that cause me to change this was fixed by adding show_me_another to checkSet. Before that, the lower permissions would cause a crash in the show_me_another route as a TA and required extra handling. So the original reason for my change is no longer needed.

[drgrice1]

I think it is moot. I think that #3271 is the way to go, and this pull request is not. That allows for the removal and clean up of a whole lot of code. There are now much better ways of doing things without all of that hackery.

[somiaj]

We are most likely going to #3271. Leaving this as a draft in case someone wants to advocate this approach.