Repository navigation
Conversation
| my $globalSet = $c->{setID} ? $c->db->getGlobalSet($c->{setID}) : undef; | ||
|
|
||
| $c->authen->flash(status_message => $c->{status_message}->join('')); | ||
| $c->reply_with_redirect($c->systemLink( | ||
| defined $globalSet && $globalSet->assignment_type =~ /gateway/ | ||
| ? $c->url_for('gateway_quiz', setID => 'Undefined_Set') | ||
| : $c->url_for('problem_detail', setID => $c->{setID}, problemID => $c->{problemID}), | ||
| ? $c->url_for('gateway_quiz', setID => 'Undefined_Set') | ||
| : $c->url_for( | ||
| 'problem_detail', | ||
| setID => $c->{setID} // 'Undefined_Set', | ||
| problemID => $c->{problemID} // 1 | ||
| ), |
There was a problem hiding this comment.
Should line 761 be to allow for setID equal to 0?
my $globalSet = defined $c->{setID} ? $c->db->getGlobalSet($c->{setID}) : undef;
This matches line 769 using // to. Though, what about setID as an empty string '', does this case need to be worried about as well, such as?
my $globalSet = defined $c->{setID} && $c->{setID} ne '' ? $c->db->getGlobalSet($c->{setID}) : undef;
In which case does the logic in 769 need to dissallow setID being '', such as:
setID => $c->{setID} && $c->{setID} ne '' ? $c->{setID} : 'Undefined_Set'
There was a problem hiding this comment.
This package is entirely inconsistent on this. Almost all of the checks are just if $setID, but most uses for extracting the value are $setID // ''. So the implementation in this pull request is quite consistent with the rest of the module. However, the module in general does not support a setID of 0. So much of it needs to be rewritten if that is desired.
There was a problem hiding this comment.
We have gone though effort to allow a set id of 0 else where, so maybe something to be done in the future. Either that or we just drop supporting a set id of 0 everywhere. But if this is consistent with this package, this can be filed as a future work if needed.
There was a problem hiding this comment.
It certainly should be fixed. I could add it to this pull request. I don't think it will be that big of a change to do so.
There was a problem hiding this comment.
I went ahead and made that change.
…o set. When the problem editor is opened without a set (for example from the File Manager's "Edit" action), viewing or saving with "Open in new window" checked called `getGlobalSet` with an undefined set id and failed with "argument 1 must contain a set_id". Now the set is only looked up if there is a set id, and the redirect goes to problem 1 of the `Undefined_Set` in that case, as is already done for blank and sample problems. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
78fe114 to
2d97c90
Compare
| ? $c->url_for('gateway_quiz', setID => 'Undefined_Set') | ||
| : $c->url_for( | ||
| 'problem_detail', | ||
| setID => $c->{setID} // 'Undefined_Set', |
There was a problem hiding this comment.
We probably over check everything, but sure we don't need to deal with $c->{setID} being the empty string here (and similar below)?
There was a problem hiding this comment.
There is no need to ever check $c->{setID} ne '' because that really cannot happen in this module.
There was a problem hiding this comment.
Note the $c->{setID} is set from the route stash value, and it cannot be the empty string.
Currently most of the checks in the editor will not work for a set named "0" as they are usually just `if $setID`. So switch the checks to `if defined $setId` instead.
2d97c90 to
39294d2
Compare
When the problem editor is opened without a set (for example from the File Manager's "Edit" action), viewing or saving with "Open in new window" checked called
getGlobalSetwith an undefined set id and failed with "argument 1 must contain a set_id". Now the set is only looked up if there is a set id, and the redirect goes to problem 1 of theUndefined_Setin that case, as is already done for blank and sample problems.Edit: Also make set "0" work with the editor.