account for test that starts at a number other than 1

[Alex-Jordan]

This addresses #2247, and should possibly be considered for a hotfix.

As part of checking whether it is legal for a user's scores to be recorded, the code wants to know how many times the quiz has been graded already and confirm the number of graded submissions have not been used up. We don't record the number of graded submissions anywhere. But we record the number of attempts on individual problems, which all get submitted simultaneously when a test is graded. So as a proxy for what we really want, there is this place in the code that checks how many attempts have been used on the first problem. The code was assuming the first problem is numbered 1, and uses that to look it up in the database to get its attempt count. This changes that code to get the actual first problem number.

To test, first reproduce the error described in #2247. Then see if this fixes that.

[somiaj]

I think we use this method to figure out attempts for quizzes in multiple locations. Grepping for getMergedProblemVersion I notice that lib/WeBWorK/ContentGenerator/Hardcopy.pm is also hard coded to get problem 1. The other locations look fine with my initial glance (all use a variable, which I am assuming is a valid problemID).

Should probably fix Hardcopy.pm as well.

[drgrice1]

This looks good. Although @somiaj's assessment is accurate. Fix the place in Hardcopy.pm in the same way. Then maybe (crossing fingers) this is the last of the gateway quiz consecutive numbering or randomly ordered problem issues!

[pstaabp]

Now all the problem numbering bug have been squashed.

[Alex-Jordan]

Even when a quiz/test starts its numbering at 2 (for example) the exercise numbers that are printed on screen or in PDF are still numbered consecutively from 1. Is that desirable, or is that also something to fix?

[somiaj]

It probably isn't ideal, but how difficult would it be to fix, since isn't the enumerate creating the numbers?

Though couldn't you just add an appropriate \setcounter{enumi}{$ProblemID - 1} before each \item if there is a gap in the problem id numbers?

Edit: I'm just talking about hardcopy here, I thought the correct problem number was already printed to the screen.

[Alex-Jordan]

I just force pushed with a commit that includes the Hardcopy.pm edit.

[somiaj]

Actually looks like there is an inconsistency between tests and homework sets here, homework sets use the problem ID when enumerating the problems but tests always start at one and go consecutively.

I would side on consistency here, but have no real preference on if we display the problemID or always go in constitutive order.

[Alex-Jordan]

I would have assumed that too. But I just double checked. In my test quiz he actual numbering starts at 2:

But both on screen and in hardcopy, it is numbered 1:

[Alex-Jordan]

I think it could be confusing when the numbering actually is like 2, 5, 11, ... but users (including the instructor) see 1, 2, 3. The instructor may not even realize the actual numbering is different if they forgot, or if they just loaded some set def file. This has led to confusion about bugs that ultimately boil down to these numbering issues.

But now that I think about it, quizzes can have the problems displayed in random order. So we have to be careful if we are going to change the current behavior that if problems are in a random order, I guess then we do use 1, 2, 3 as the visual labels.

[somiaj]

@Alex-Jordan That conclusion is probably why that inconsistency exists between homework sets and tests, and is probably a good reason for it. The homework set shows the actual problem ID to reflect how it is set in the editor, but tests have to consecutively order things to deal with random ordering .

[drgrice1]

I was going to chime in, but it looks like @somiaj figured out why numbering in the actual test is different than in the set deifinition and must be.

[somiaj]

What about showing the actual problem ID for tests when viewed as an user with appropriate permissions, this way if an instructor is looking at the test, not only can they see if the problem ID's aren't consecutive, they can see which ID is being used when problems are displayed in random order?

[Alex-Jordan]

Of course a lot of this thread now is for a separate topic, and should be addressed (or not) separately from this PR. But I'll keep going anyway :) To me it seems that if a test is not using random ordering, then it would be reasonable to have the visual numbering match the numbering in the database. And in principle that distinction would be doable.

[somiaj]

@Alex-Jordan Though that could give some indication to a student if a quiz is not in random order. Which is why I suggested we just show (maybe along side the problem number) the actual problem ID to instructors (users with correct permissions).

[drgrice1]

It might be reasonable, but does make a mess with implementation. Furthermore, I think that with the we might actually have all of the consecutive numbering issues straightened out, so I really don't want to do down the road that you are proposing.

[drgrice1]

I think that @somiaj's suggestion sounds like a good idea. That would be useful when instructors want to know which problem it actually was, and would not be to hard to implement without messing anything else up.

[somiaj]

To get back on track, this looks good and now has three approvals, so ready to merge.

I suggest a hotfix as well, to fix this bug in 2.18.

[drgrice1]

Well, that does make that method name inconsistent with some of the others. Particularly countProblemVersionsWhere and getProblemVersionsWhere which are dealing with the same set of things in the database.

[drgrice1]

Also, countProblemVersions, getProblemVersions, and getAllProblemVersions

[Alex-Jordan]

I force pushed after rolling back that last commit.

[drgrice1]

That is probably best. I think that a revision of some of the database methods could be used, but maybe that is best left for another pull request.

[Alex-Jordan]

If someone merges this, probably also merge #2249 and close #2247.

[drgrice1]

I will merge this since it has three approvals. I will also merge #2249.