Update permissions for viewing info on ProblemSet page. #2869
Conversation
somiaj
force-pushed
the
unopened-set-preview
branch
2 times, most recently
from
ba53b4d to
1bc4f49
Compare
templates/layouts/system.html.ep
Outdated
| <%= $c->info =%> | ||
| % # Only show info div if the content is not empty. | ||
| % my $info = $c->info; | ||
| % if ($info !~ /^\s*$/) { |
There was a problem hiding this comment.
This should be
| % if ($info !~ /^\s*$/) { | |
| % if ($info =~ /\S/) { |
That is the usual check that a string contains non white space characters and is a simpler regular expression. Benchmarking shows it is generally faster too!
There was a problem hiding this comment.
Actually, this change is not right. The problem is that if this actually happens, then the result is an improperly formatted page. The check on line 135 above and the case that the info is actually shown really should match up perfectly, and this is not doing that.
What case are you trying to catch here and avoid showing an empty info? The first thing to check is if it really is a case to be concerned about. If so, then there are better ways to do this.
There was a problem hiding this comment.
I missed the other check that made used the col-8 vs col-12 style above, but I didn't think it mattered, is it really not formatted correctly if the divs don't use all the columns in the grid? Anyways, if you go to a bad set or a set with an error, https://server/webwork2/CourseName/BadSetName, there is an empty gray div floating to the right with nothing in it. I was trying to prevent that from happening. I am unsure if any other pages that use this info div have similar issues where it is just empty.
There was a problem hiding this comment.
In my opinion yes. Generally, the info box display needs work regardless of this pull request. Not only for this case, but for others where the content may be empty. For example, if your course has an empty course_info.txt file, then that also currently results in the empty gray box. These cases should all not show the info box, and the page should be properly formatted as intended for the layout.
It seems that to make this work right, all modules that provide the info box and such that the info box might be empty should also override the can method (as the ProblemSets.pm module already does), and that method should not only return 0 for permissions reasons but for the empty info box case.
Note that there is another case where $c->can('info') is called that affects the layout. That is in the go method of the base ContentGenerator module (line 113). That sets the layout for the footer to match the main content (note that some modules override that setting later also). So that also needs to choose the correct layout setting for these cases.
There is an efficiency issue with the current setup. The ProblemSets.pm is already performing extra file access due to this approach. The can method is called three times and each time it opens the course info file and reads its contents and compares it to the default. Then the info method opens that file yet again and reads the contents to actually display (assuming the can method returned true). So to do this well with the current setup, the can method really should do all of the work and stash the content, but with protections to ensure it doesn't execute inefficiency multiple times, and the info method should just use the stashed result. Care also needs to be taken for the ProblemSet.pm module, because that doesn't know if it has content until after the initialize method runs (that is when the set header is currently rendered), and that runs after the current can call in the go method of the base ContentGenerator module.
To simplify all of this I think that really we should just do away with the info box entirely. Students really should just be left in the dark anyway, and don't really need or even want instructions! 😈
There was a problem hiding this comment.
Yea, the fact that $c->can('info') is called before $c->initialize does make things harder to just override the can method. Any way that call in the go method could be moved until after the initialize method? I found something that appears to work, I'll just update the setting for the footerWidthClass on a future can call which appears to work. Patch incoming.
There was a problem hiding this comment.
The call in the go method could be moved after the initialize method. It would just need to use //= instead of =.
There was a problem hiding this comment.
Any preference to moving the method vs updating the value of footerWidthClass at the end of the initialize method as I am currently doing?
There was a problem hiding this comment.
If you are just updating it with the same value that the go method assignment uses based on the can call result, then I would prefer moving the go method assignment.
There was a problem hiding this comment.
Updated, the method has now been moved.
somiaj
force-pushed
the
unopened-set-preview
branch
5 times, most recently
from
98dab46 to
e5ef385
Compare
drgrice1
left a comment
drgrice1
left a comment
There was a problem hiding this comment.
There is another problem with this. Currently if $LTIGradeMod is "homework" and $permissionLevels{navigation_allowed} is set to "student" or lower, then when the student views the assignments page they are shown:
However, now the message stating that you need to log in to the set via the LMS is not shown and the links for those sets are active. Furthermore, if you click on one of those sets to open it you get:
That isn't good, and needs to be rethought with this pull request.
Rework when a user is able to see set information on the ProblemSet page. This includes being able to see the set description and information header before a set is open. The goal is to make the ProblemSet page friendlier when a student accesses it from an LMS before the open date or if the set is not currently visible. In addition the set headers can be used to provide information to students before the set opens. This is done by adding a new permission `view_unoppend_set_info` which defaults to `guest` (this allows instructors to change this to `ta` to get the old behavior). Users with this permission will be shown a ProblemSet page that includes when the set opens along with the set information header and a button to email the instructor before the open date. In addition users with this permission will be shown a warning alert instead of a danger alert if they try to access a hidden set (the set header will not be included in this case). Note, that the permission `view_unoppend_sets` description in the course configuration is just being able to view problems on sets which are not open, so this new permission is consistent with the previous description, it just separates seeing set info from seeing set problems. Care is taken to ensure that students can view (thus access) any existing test versions for tests that have ip restrictions, or if the template open date was changed. Only show the right info panel div on the ProblemSet page when the set header exists and the user has permissions to see it. Translations were added to messages about IP restrictions.
When viewing the set information before access a set via an LMS for homework grade passback, show a warning that they must access the set via the LMS before starting. They will be able to see the set info, they just won't be able to access any problems or start any test versions.
somiaj
force-pushed
the
unopened-set-preview
branch
from
e5ef385 to
89f1867
Compare
|
@drgrice1 I updated it to show a warning message about accessing the set via the LMS in the case you described. I don't have a way to test LTI in my development setup, so I'm mostly 'guessing' it works (sorry about that, I just don't have time to setup a way to test that at the moment). |
|
I am not quite sure if the links should be active and/or the message should be shown on the |
drgrice1
left a comment
drgrice1
left a comment
There was a problem hiding this comment.
I still really don't like this pull request as it is. I guess I just really don't like that this is a permission at all. I think we need to decide if set headers really need such a permission. I really don't think there is such a need, and I think this is just yet another rather confusing permission.
The addition of this permission also adds a bunch of redundancy and now out of sync wording for text shown in different cases. The wording on the problem sets page if a student is missing the requisite grade pass back data and the user does not have this new permission is You must log into this set via your Learning Management System ([_1])., but the new wording if the user does have this new permission is You must access this set via your Learning Management System ([_1]) before starting. Those should use the same wording. Then there is also the message that is in the Authz::checkSet method that says You must use your Learning Management System ([_1]) to access this set. Try logging in to the Learning Management System and visiting the set from there. and is shown if the user finds and uses a link that directly enters the set, but doesn't have the new permission.
Ultimately we need to decide if this needs a permission at all. I vote no. I don't see the need for the security for set headers.
| % # Show a warning that the user must access the set via their LMS before they can start. | ||
| % if ($c->{viewSetCheck} && $c->{viewSetCheck} eq 'lti_restricted') { | ||
| <div class="alert alert-warning"> | ||
| <%= maketext('You must access this set via your Learning Management System ([_1]) before starting.', |
There was a problem hiding this comment.
This needs to be
| <%= maketext('You must access this set via your Learning Management System ([_1]) before starting.', | |
| <%== maketext('You must access this set via your Learning Management System ([_1]) before starting.', |
Otherwise the link is displayed literally as <a href="https://myschool.edu/lms/">the LMS</a> instead of being a link.
|
I thought I had posted to this thread but can't find it now. I wonder if I
drafted something and moved away before clicking to post.
No matter. The gist of what I wrote was why not just always show set
descriptions and headers, even when the user cannot access the set? If an
instructor wants to hide something, just don't put in in the header in the
first place ppl. Or if they need to, get fancy and code pg within the
header to conditionally reveal certain things.
Even if there were a global permission, an instructor might want one set to
reveal the header always, and another only under the right conditions.
On Tue, Jan 13, 2026, 6:48 PM Glenn Rice ***@***.***> wrote:
***@***.**** commented on this pull request.
I still really don't like this pull request as it is. I guess I just
really don't like that this is a permission at all. I think we need to
decide if set headers really need such a permission. I really don't think
there is such a need, and I think this is just yet another rather confusing
permission.
The addition of this permission also adds a bunch of redundancy and now
out of sync wording for text shown in different cases. The wording on the
problem sets page if a student is missing the requisite grade pass back
data and the user does not have this new permission is You must log into
this set via your Learning Management System ([_1])., but the new wording
if the user does have this new permission is You must access this set via
your Learning Management System ([_1]) before starting. Those should use
the same wording. Then there is also the message that is in the
Authz::checkSet method that says You must use your Learning Management
System ([_1]) to access this set. Try logging in to the Learning Management
System and visiting the set from there. and is shown if the user finds
and uses a link that directly enters the set, but doesn't have the new
permission.
Ultimately we need to decide if this needs a permission at all. I vote no.
I don't see the need for the security for set headers.
------------------------------
In templates/ContentGenerator/ProblemSet.html.ep
<https://urldefense.com/v3/__https://github.com/openwebwork/webwork2/pull/2869*discussion_r2688712338__;Iw!!Ka_JY85zDv0FFw!k5pH44OTON2tqfqraCMeu-PhGhvwC6NB1lKo_2ZWynBJ8zENbNMjPgRRzRMFdWALexDFxfKstEMAIplpsTBi7-03QBY$>
:
> % }
%
% my $set = $c->{set};
%
% # Stats message displays the current status of the set and states the next important date.
<%= include 'ContentGenerator/Base/set_status', set => $set =%>
%
+% # Shows warning about restricted IP settings.
+% if ($c->{viewSetCheck} && $c->{viewSetCheck} eq 'restricted') {
+ <div class="alert alert-warning"><%= $c->{invalidSet} %></div>
+% }
+% # Show a warning that the user must access the set via their LMS before they can start.
+% if ($c->{viewSetCheck} && $c->{viewSetCheck} eq 'lti_restricted') {
+ <div class="alert alert-warning">
+ <%= maketext('You must access this set via your Learning Management System ([_1]) before starting.',
This needs to be
⬇️ Suggested change
- <%= maketext('You must access this set via your Learning Management System ([_1]) before starting.',
+ <%== maketext('You must access this set via your Learning Management System ([_1]) before starting.',
Otherwise the link is displayed literally as <a href="
https://myschool.edu/lms/
<https://urldefense.com/v3/__https://myschool.edu/lms/__;!!Ka_JY85zDv0FFw!k5pH44OTON2tqfqraCMeu-PhGhvwC6NB1lKo_2ZWynBJ8zENbNMjPgRRzRMFdWALexDFxfKstEMAIplpsTBiaAGszrI$>">the
LMS</a> instead of being a link.
—
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/openwebwork/webwork2/pull/2869*pullrequestreview-3658593873__;Iw!!Ka_JY85zDv0FFw!k5pH44OTON2tqfqraCMeu-PhGhvwC6NB1lKo_2ZWynBJ8zENbNMjPgRRzRMFdWALexDFxfKstEMAIplpsTBi92ZqezY$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ABEDOAG6LJEQCRGEW776E2D4GWVBPAVCNFSM6AAAAACP6ZZAKOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTMNJYGU4TGOBXGM__;!!Ka_JY85zDv0FFw!k5pH44OTON2tqfqraCMeu-PhGhvwC6NB1lKo_2ZWynBJ8zENbNMjPgRRzRMFdWALexDFxfKstEMAIplpsTBi_41jQFM$>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
Alex Jordan
Mathematics Instructor
Portland Community College
|
|
All that is to say the goal of making the situation friendlier is good. And
I see it as an improvement just to always show headers.
Alex Jordan
Mathematics Instructor
Portland Community College
…On Tue, Jan 13, 2026, 6:58 PM Alex Jordan ***@***.***> wrote:
I thought I had posted to this thread but can't find it now. I wonder if I
drafted something and moved away before clicking to post.
No matter. The gist of what I wrote was why not just always show set
descriptions and headers, even when the user cannot access the set? If an
instructor wants to hide something, just don't put in in the header in the
first place ppl. Or if they need to, get fancy and code pg within the
header to conditionally reveal certain things.
Even if there were a global permission, an instructor might want one set
to reveal the header always, and another only under the right conditions.
On Tue, Jan 13, 2026, 6:48 PM Glenn Rice ***@***.***> wrote:
> ***@***.**** commented on this pull request.
>
> I still really don't like this pull request as it is. I guess I just
> really don't like that this is a permission at all. I think we need to
> decide if set headers really need such a permission. I really don't think
> there is such a need, and I think this is just yet another rather confusing
> permission.
>
> The addition of this permission also adds a bunch of redundancy and now
> out of sync wording for text shown in different cases. The wording on the
> problem sets page if a student is missing the requisite grade pass back
> data and the user does not have this new permission is You must log into
> this set via your Learning Management System ([_1])., but the new
> wording if the user does have this new permission is You must access
> this set via your Learning Management System ([_1]) before starting.
> Those should use the same wording. Then there is also the message that is
> in the Authz::checkSet method that says You must use your Learning
> Management System ([_1]) to access this set. Try logging in to the Learning
> Management System and visiting the set from there. and is shown if the
> user finds and uses a link that directly enters the set, but doesn't have
> the new permission.
>
> Ultimately we need to decide if this needs a permission at all. I vote
> no. I don't see the need for the security for set headers.
> ------------------------------
>
> In templates/ContentGenerator/ProblemSet.html.ep
> <https://urldefense.com/v3/__https://github.com/openwebwork/webwork2/pull/2869*discussion_r2688712338__;Iw!!Ka_JY85zDv0FFw!k5pH44OTON2tqfqraCMeu-PhGhvwC6NB1lKo_2ZWynBJ8zENbNMjPgRRzRMFdWALexDFxfKstEMAIplpsTBi7-03QBY$>
> :
>
> > % }
> %
> % my $set = $c->{set};
> %
> % # Stats message displays the current status of the set and states the next important date.
> <%= include 'ContentGenerator/Base/set_status', set => $set =%>
> %
> +% # Shows warning about restricted IP settings.
> +% if ($c->{viewSetCheck} && $c->{viewSetCheck} eq 'restricted') {
> + <div class="alert alert-warning"><%= $c->{invalidSet} %></div>
> +% }
> +% # Show a warning that the user must access the set via their LMS before they can start.
> +% if ($c->{viewSetCheck} && $c->{viewSetCheck} eq 'lti_restricted') {
> + <div class="alert alert-warning">
> + <%= maketext('You must access this set via your Learning Management System ([_1]) before starting.',
>
> This needs to be
> ⬇️ Suggested change
>
> - <%= maketext('You must access this set via your Learning Management System ([_1]) before starting.',
> + <%== maketext('You must access this set via your Learning Management System ([_1]) before starting.',
>
> Otherwise the link is displayed literally as <a href="
> https://myschool.edu/lms/
> <https://urldefense.com/v3/__https://myschool.edu/lms/__;!!Ka_JY85zDv0FFw!k5pH44OTON2tqfqraCMeu-PhGhvwC6NB1lKo_2ZWynBJ8zENbNMjPgRRzRMFdWALexDFxfKstEMAIplpsTBiaAGszrI$>">the
> LMS</a> instead of being a link.
>
> —
> Reply to this email directly, view it on GitHub
> <https://urldefense.com/v3/__https://github.com/openwebwork/webwork2/pull/2869*pullrequestreview-3658593873__;Iw!!Ka_JY85zDv0FFw!k5pH44OTON2tqfqraCMeu-PhGhvwC6NB1lKo_2ZWynBJ8zENbNMjPgRRzRMFdWALexDFxfKstEMAIplpsTBi92ZqezY$>,
> or unsubscribe
> <https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ABEDOAG6LJEQCRGEW776E2D4GWVBPAVCNFSM6AAAAACP6ZZAKOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTMNJYGU4TGOBXGM__;!!Ka_JY85zDv0FFw!k5pH44OTON2tqfqraCMeu-PhGhvwC6NB1lKo_2ZWynBJ8zENbNMjPgRRzRMFdWALexDFxfKstEMAIplpsTBi_41jQFM$>
> .
> You are receiving this because you are subscribed to this thread.Message
> ID: ***@***.***>
>
Alex Jordan
Mathematics Instructor
Portland Community College
|
|
So it sounds like you are on the same page as me then. |
|
I agree with you on that this shouldn't be a permission. When I was originally thinking about adding this I was just going to give access to the set info. I ultimately went with a permission so if someone didn't like this change they could just disable it by setting the permission, as I wasn't sure if my use case was the only use case to consider. But it seems you two would also rather just always show the |
|
@somiaj: By the way, you don't need actual LTI authentication to test this for the LTI setup. Just set up the course so that it uses LTI authentication with homework grade passback, but allow direct login. That will still show the behavior that needs to be tested here. |
drgrice1
left a comment
drgrice1
left a comment
There was a problem hiding this comment.
There is another case that is not working right with this pull request. That is the case of restricted release sets. If a set has a release condition on another set, the with the develop branch you see
on the problem sets page.
With this pull request, that message telling the student that the condition that needs to be met is still shown, and the link to the set is active. When you click on the link, then you see
That also needs to be done differently.
3 participants
Rework when a user is able to see set information on the ProblemSet page. This includes being able to see the set description and information header before a set is open.
The goal is to make the ProblemSet page friendlier when a student accesses it from an LMS before the open date or if the set is not currently visible. In addition the set headers can be used to provide information to students before the set opens.
This is done by adding a new permission
view_unoppend_set_infowhich defaults toguest(this allows instructors to change this totato get the old behavior). Users with this permission will be shown a ProblemSet page that includes when the set opens along with the set information header and a button to email the instructor before the open date. In addition users with this permission will be shown a warning alert instead of a danger alert if they try to access a hidden set (the set header will not be included in this case).Note, that the permission
view_unoppend_setsdescription in the course configuration is just being able to view problems on sets which are not open, so this new permission is consistent with the previous description, it just separates seeing set info from seeing set problems.Care is taken to ensure that students can view (thus access) any existing test versions for tests that have ip restrictions, or if the template open date was changed.
Last, only show the right info panel div when the panel is not empty and doesn't include only whitespace. And translations were added to messages about IP restrictions.