Correct the problem numbers when a set definition file is imported.

[drgrice1]

Currently the problem numbers in the set defintion file are completely ignored due to a minor mistake. problem_id is used in the set definition file, and problemID is used by the WeBWorK::Utils::Instructor::addProblemToSet method, and I made didn't translate that correctly.

This fixes the issue noted by @Alex-Jordan in #2194 (comment).

Note that this does not fix the issue noted in the next comment there (#2194 (comment)). I tested that on both WeBWorK 2.17, 2.18, and with this pull request, and all have that issue. With the current develop branch without this pull request this issue does not occur because of the error in translation fixed in this pull request which basically results in a set definition import always having problems numbered consecutively starting at 1. I know why the issue occurs, and will look into finding a fix and submit another pull request for that.

Edit: Now this does fix the issue noted above with the simplest possible approach of using the max of the valid problem ids from the set definition file plus 1 (and incrementing from there).

[drgrice1]

Also note that if you shift the problem numbers as you suggest, then you are honoring the order the instructor set, but not the original problem ids that the instructor set. So you still are not disobeying the intent of the instructor, and really there is no way to do so in this case.

[drgrice1]

By the way, when an invalid problem id is found in the set definition file, then the problem id is set to the empty string. So if a set definition file has the problem ids -2, -1, 0, 1, 2, then this will get '', '', 0, 1, 2. Or if it has 1, 2, a, b, c, 6, 7, then this will get 1, 2, '', '', '', 6, 7.

[somiaj]

I agree no way to preserve the complete intent, to me when I design a set problem order is more important than the actual number assigned to the problem, hence why I suggested a shift, as moving the problem out of order would be more disruptive to me.

Seems there is already code in place to deal with invalid problem IDs, maybe 0 should just be added to that ? What happens to the empty problem ids when added to the database?

[somiaj]

Oh wait, looking at the actual pull request, I see that you are just adding 0 to the previous method of dealing with invalid problem IDs, that is probably best for now.

[drgrice1]

Yes, this just fixes the handling of invalid problem ids with the same behavior it had before.

This doesn't technically add 0 to the previous method of dealing with invalid problem ids. That was already included in the invalid problem ids. Previously it was $rh_problem->{problemID} ? $rh_problem->{problemID} : $freeProblemID++ which is equivalent to $rh_problem->{problemID} || $freeProblemID++. If $rh_problem->{problemID} is 0 in either case, then it is falsy and so the $freeProblemID is used.