When sorting achievements, make level achievements last.

[somiaj]

Level achievements need to be processed last, and currently an instructor can edit achievement numbers which could place some achievements after the level achievements.

Update the sortAchievements utility method to always place level achievements at the end of the list.

This was brought up in the review of #2579.

[drgrice1]

I don't think this is the right way to go. The achievement "Number" is supposed to be its evaluation order.

I don't think this is a sorting issue at all. It is that line 143 in lib/WeBWorK/ContentGenerator/Achievements.pm and other places that skip level achievements in a for loop (like your added code in #2579) should use next instead of last. Skip the level achievements, but deal with the rest. The evaluation order doesn't matter for that processing.

Also, according to the help on the "Achievements Manager" page the level achievements need to be evaluated first, not last. So that probably needs to be fixed.

I think in general we need to kill the webwork2 achievements as they are, and start over. The design is to flawed to go forward. There really is no need for them to be Perl code executed in Safe. A versatile achievement system could certainly be implemented without that. Of course, for now we are stuck with what we have.

[somiaj]

I went with this approach for updating the sorting order instead of changing the last to next because I couldn't think of any case that level achievements should be evaluated before any other achievements (also seems the help agrees with this assuming 'first' should have been 'last' was just a typo). I also didn't see anywhere in AchievementEvaluator.pm forcing the level achievements to be last outside of their order, so forcing the sort to put level achievements last will make sure that they are always evaluated last like the help probably should have stated, and also make the last statements work as expected. I think it was just always assumed level achievements should be last.

I can go with your proposal if you think it would be better to just support allowing level achievements to not be last, I just think this would cause the issue of students passing level boundaries and then not being awarded the correct level.

I agree a newer approach to achievements could be useful, I was just trying to apply a bandaid to what we currently have.

[somiaj]

I also updated the help to state level achievements are processed last, not first.

[Alex-Jordan]

Maybe this is clear but level achievements should be processed last. Because some other achievement you just earned might give you the last few achievement points you need to level up.

I think if it is given that level achievements are coming last, then the use of last like at line 143 in lib/WeBWorK/ContentGenerator/Achievements.pm is sort of a hack. As in now that you've hit a level achievement, we know this one and all that come after it are level achievements. They should all be skipped since that subroutine is about processing the badges. Geoff probably assumed (hoped?) people would follow the guidance (which came with an unfortunate typo) to always place the level achievements last.

[somiaj]

I agree that the intent was always that level achievements should be sorted last. All this does is enforce that intent so users cannot accidentally order them incorrectly.

[drgrice1]

There is another incongruency in the help for the achievements manager that this pull request creates. The help on the "Number" achievement option "Sets the order in which achievements are listed and evaluated".

[somiaj]

Few more updates to clarify that level achievements are always listed and evaluated last.

[drgrice1]

Lets just go with this.