improvements to Instructor Tools

Alex-Jordan commented 9 months ago

This makes changes to Instructor Tools. Also it makes changes to scrollingRecordList which is used by Instructor Tools, and also by the Assigner Tool, Email, Hardcopy, and Past Answer pages. So testing should look at all five pages.

There are cosmetic changes that could be better. I won't describe them here; feel free to make suggestions for improving anything cosmetic. The functional improvements are:

The sort options are only ones that make sense for the available display formats.
There are now three format options for displaying sets. (There was only one before.) The two I added might not be best, but it's clear how to change them or add more. Note: the new format methods for sets include displaying the set's due date. This calls on formatDateTime and presently generates an error message. But #2263 will make that go away.
Previously there were no filter options for sets. Now you can filter by type (e.g. "Proctored Test") and you can filter by Visible/NotVisible. Note that you have to have at least two types to see those filtering options. And you have to have both visible and not visible sets present to see those filtering options.
I tried to make the functionality of all buttons more clear with changes to the wording.
"Statistics for one user" is a silly thing, that is really just "Progress for one user". So I removed "Statistics for one user".
If you are using this to add new users, you can choose ahead of time how many you want to add. Previously it was fixedat 5.
Previously there was a button to take you to the Set Detail page for a set, and a separate button to take you to the Set Detail page for a set but with some subset of users selected. I combined these into one button. De-select all users if you want the regular Set Detail page.

A screenshot of what to expect:

Alex-Jordan commented 9 months ago

I forgot something. Before these changes, the Instructor Tools page has a button to take you to the Email page, and another button to take you to the File Manager. I removed these. They seem unnecessary with those things being in the nav menu, and also having nothing to do with selecting users or sets.

somiaj commented 9 months ago

@Alex-Jordan would having a button 'Email selected users' be useful? They can do this directly from the email page, so not really needed but could be an alternative to just removing it. Note, I think removing it is fine, this is just a though that passed my mind.

drgrice1 commented 9 months ago

I agree that the "Email" and "File Manager" buttons are not needed by the way.

Alex-Jordan commented 9 months ago

Thanks @drgrice1 for the review and improvements.

More tools could be added. I'm polling to see what we would like.

@somiaj asked about "Email all selected users"
"View past answers for one user for one set"
"Generate hardcopy for all selected sets for all selected users"

And as mentioned at #1151, I'd like to add "Randomize seeds for all selected assignments for all selected users". But that can come later when that feature is a thing in the first place.

Alex-Jordan commented 9 months ago

I added the three tools mentioned in my previous comment.

The text description for those tools was long and I needed to grow the 350px size to 370px. This approach has fragility in at least two ways. A user might rescale font size and then these lines are still not long enough. Or a localization might be longer.

Since the prob_id_sort sorting routine is now used by both Instructor Tools as well as the Answer Log, I moved it to Utils and both pages import it from there.

There was an adjacent byData subroutine that appears to have not been used since 2012, and I removed that.

I had to change the name of the user select element in the Email page to make it match the name of the user select element from the Instructor Tools page. There was one place in htdocs where I then followed up with the same name change.

drgrice1 commented 9 months ago

Switch to using the em instead of px, and that should resolve the fragility of the width.

drgrice1 commented 9 months ago

That is with the font scaling. Not the translation issue.

drgrice1 commented 9 months ago

Another option that will generally resolve most issues is to just use a large enough width to leave excess. That won't entirely resolve the issue, but probably nothing will.

Alex-Jordan commented 9 months ago

I did a little of that. 27em was enough, but I took it to 28.

drgrice1 commented 9 months ago

It seems that 28em is not enough. The "View answers for selected users, for selected sets" is wrapping for me. I tested this in both Firefox and Chrome. If I increase it to 29em it is enough, but not by much.

somiaj commented 9 months ago

The size seems fine for me, I don't see the wrapping for "View answers for selected users, for selected sets" here. Could this one be shortened? Maybe "View selected users' answers for selected sets".

Also "Score selected sets for ALL users", ALL is not fully valid here, maybe "Score selected sets for active users" or "score selected sets for active students"?

drgrice1 commented 9 months ago

Also "Score selected sets for ALL users", ALL is not fully valid here, maybe "Score selected sets for active users" or "score selected sets for active students"?

I think that "all" is fine here, and I think is better than "active". "active" can mean many things. Actively logged into the course. Actively working on the assignment in question. Furthermore, when a set is scored, all users that the set is assigned to are scored, even if they have not "actively" worked the set.

drgrice1 commented 9 months ago

Actually, testing further, it really is ALL users. Even those that are not assigned the set are scored.

drgrice1 commented 9 months ago

Okay, so dropped students are not scored, but there is no reason to really be that detailed here.

drgrice1 commented 9 months ago

There is another sizing aspect to fiddle with. The buttons could possibly be made a little smaller. Currently we are using a width of 25%. That could be reduced to 20%. Unfortunately, bootstrap doesn't have a class for that, so it would have to be done with an inline style attribute. Of course, if a translation of one of the button labels is longer there could be problems with that.

drgrice1 commented 9 months ago

I suppose you could also go back to the different sizing of the three categories that you had in this pull request to start with. Make the second category with all of the longer phrases a bit bigger, and the other two smaller.

Alex-Jordan commented 9 months ago

Since "Score" is not dependent on any user selections, I could move it to the left column. Is that a good move?

I should add at least one user format option that makes use of permission level and role. Then you would be able to sort by those too. And I could add filtering by those things as well.

The 28 em (and I believe 27em) is working for me with FF, Chrome, Safari. I wonder if it's something about me (and I assume @somiaj ) using a Mac. Anyway I will make it bigger the next round of edits. BTW I was thinking to make that a setting in some css file, but if we really are adjusting it to accommodate certain string lengths, it seems appropriate to set the width inline.

We could use width:20%; on the buttons instead of the bootstrapw-25 class, and that recovers some space.

We could move to a tabbed menu for the three categories to make the spacing issues moot. At the cost of not seeing all the tools right away.

drgrice1 commented 9 months ago

Since "Score" is not dependent on any user selections, I could move it to the left column. Is that a good move?

Although all of the left column's actions involve only users. Also, then the left column would have more than all of the others, making it unbalanced.

I should add at least one user format option that makes use of permission level and role. Then you would be able to sort by those too. And I could add filtering by those things as well.

That would be nice to have. That would take some rewriting of the scrolling record list though since the permission level is not in the same database table.

The 28 em (and I believe 27em) is working for me with FF, Chrome, Safari. I wonder if it's something about me (and I assume @somiaj ) using a Mac. Anyway I will make it bigger the next round of edits. BTW I was thinking to make that a setting in some css file, but if we really are adjusting it to accommodate certain string lengths, it seems appropriate to set the width inline.

I also thought that a css file might be needed. Although, we don't have that many inline styles here (yet).

We could move to a tabbed menu for the three categories to make the spacing issues moot. At the cost of not seeing all the tools right away.

I think a different approach like that is what is going to be needed in the end. This approaches of trying to fit the three categories in at either a fixed width or a variable width using the bootstrap grid layout are just not entirely working.

Alex-Jordan commented 9 months ago

Oh I meant whichever side has the set selection, so the right side. I got mixed up. With that move plus eventually adding "randomize" to the middle, I think the three columns end up with equal size.

On Wed, Nov 29, 2023, 7:44 PM Glenn Rice @.***> wrote:

Since "Score" is not dependent on any user selections, I could move it to the left column. Is that a good move?

Although all of the left column's actions involve only users. Also, then the left column would have more than all of the others, making it unbalanced.

I should add at least one user format option that makes use of permission level and role. Then you would be able to sort by those too. And I could add filtering by those things as well.

That would be nice to have. That would take some rewriting of the scrolling record list though since the permission level is not in the same database table.

The 28 em (and I believe 27em) is working for me with FF, Chrome, Safari. I wonder if it's something about me (and I assume @somiaj https://protect2.fireeye.com/v1/url?k=31323334-501cfaeb-3132feb7-454455535732-a2dbbe6078f1ac06&q=1&e=c50cfcc8-aaa5-4622-af2b-12b8b2c2a383&u=https%3A%2F%2Fgithub.com%2Fsomiaj ) using a Mac. Anyway I will make it bigger the next round of edits. BTW I was thinking to make that a setting in some css file, but if we really are adjusting it to accommodate certain string lengths, it seems appropriate to set the width inline.

I also thought that a css file might be needed. Although, we don't have that many inline styles here (yet).

We could move to a tabbed menu for the three categories to make the spacing issues moot. At the cost of not seeing all the tools right away.

I think a different approach like that is what is going to be needed in the end. This approaches of trying to fit the three categories in at either a fixed width or a variable width using the bootstrap grid layout are just not entirely working.

— Reply to this email directly, view it on GitHub https://protect2.fireeye.com/v1/url?k=31323334-501cfaeb-3132feb7-454455535732-019234f71c6b2c39&q=1&e=c50cfcc8-aaa5-4622-af2b-12b8b2c2a383&u=https%3A%2F%2Fgithub.com%2Fopenwebwork%2Fwebwork2%2Fpull%2F2266%23issuecomment-1833060208, or unsubscribe https://protect2.fireeye.com/v1/url?k=31323334-501cfaeb-3132feb7-454455535732-f0b3387a4ef73567&q=1&e=c50cfcc8-aaa5-4622-af2b-12b8b2c2a383&u=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABEDOADH6YQMXMDP3SHPGTDYG76IBAVCNFSM6AAAAAA75EW5B6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZTGA3DAMRQHA . You are receiving this because you were mentioned.Message ID: @.***>

drgrice1 commented 9 months ago

That makes more sense. That is a good move then.

Alex-Jordan commented 9 months ago

Okay, so dropped students are not scored, but there is no reason to really be that detailed here.

In defaults.conf we define which roles are included in scoring. Only "Enrolled" is included there. Not "Drop", "Audit", "Proctor", or "Observer". Whether or not we think this is the right configuration, an installation could always customize it. So even something like "Score selected sets for enrolled users" would be wrong.

I could change it to "Score selected sets for scorable users"?

Alex-Jordan commented 9 months ago

If I go to 29em, then even at full screen width on a MacBookPro, if the nav toc is there, the three columns do not fit on one row. Anyway I will explore a tab setup for this now.

drgrice1 commented 9 months ago

I could change it to "Score selected sets for scorable users"?

How about just "Score selected set" and don't say anything about users just like it does on the scoring page itself?

drgrice1 commented 9 months ago

If I go to 29em, then even at full screen width on a MacBookPro, if the nav toc is there, the three columns do not fit on one row.

That happened for me as well. I think it was wishful thinking to believe that all the could really fit well into one row though.

somiaj commented 9 months ago

I could change it to "Score selected sets for scorable users"?

How about just "Score selected set" and don't say anything about users just like it does on the scoring page itself?

I second this suggestion.

Alex-Jordan commented 9 months ago

Minor changes mentioned in the thread now made. Plus now it uses pills to navigate between the three categories. This is mainly done with bootstrap, but in a few places there is some CSS that I put into the math4 theme.

Alex-Jordan commented 9 months ago

It still has the clunky look on a narrow screen when the button and text break on two lines. Maybe there is no need for .input-group now and we can fix this by dropping that?

somiaj commented 9 months ago

What is your reasoning for not keeping all three buttons left aligned? I don't know if having them center/right aligned helps. Also why buttons and not tabs?

Alex-Jordan commented 9 months ago

What is your reasoning for not keeping all three buttons left aligned?

So that, for example, the menu of buttons associated with users is over on the right, which is where the user selection is.

why buttons and not tabs?

I thought the pills look better. It's ultimately just a small change to bootstrap classes.

Alex-Jordan commented 9 months ago

So that, for example, the menu of buttons associated with users is over on the right, which is where the user selection is.

Er, once again I forgot which thing in on which side. Set actions are over on the right.

Alex-Jordan commented 9 months ago

Are you seeing it like this? Did you run npm ci and restart webwork2?

somiaj commented 9 months ago

Yea, I ran npm ci and am seeing the updates. I was just curious, I would have just used tabs and kept things left aligned.

Alex-Jordan commented 9 months ago

I may be overthinking it, but if you are up on the right part of the screen selecting some sets to Score for example, it seems helpful to have the relevant button be closer. It's also a visual cue that the action buttons available to you at that time are relevant to the right select menu only.

Alex-Jordan commented 9 months ago

bootstrap is putting tabindex -1 on the non-active pills...not sure why yet. But it makes it so you can't keyboard navigate to the other button menus. Will need to fix that.

Alex-Jordan commented 9 months ago

The bootstrap tabs/pills get tabindex="-1" when bootstrap js does its thing. It even overrides if I put tabindex="0" in the source. I don't get why bootstrap would do this. Why make the tabs untabbable?

Alex-Jordan commented 9 months ago

Here is the default styling as "tabs" instead of "pills", if you are interested.

drgrice1 commented 9 months ago

The bootstrap tabs/pills get tabindex="-1" when bootstrap js does its thing. It even overrides if I put tabindex="0" in the source. I don't get why bootstrap would do this. Why make the tabs untabbable?

That is okay here. You use the arrow keys to switch tabs.

drgrice1 commented 9 months ago

@Alex-Jordan: I added another pull request to this pull request branch that tweaks the styles some. I am seeing a lot of wrapping of the input groups at various window widths with the current grid layout (in fact really bad below the sm breakpoint). The pull request tweaks the col sizes for those narrower window widths to fix that, and a few other minor tweaks.

drgrice1 commented 9 months ago

By the way, there is more about why the tabs do not individually receive tab focus at https://getbootstrap.com/docs/5.3/components/navs-tabs/#accessibility.

Alex-Jordan commented 9 months ago

Thanks for the assist. I'm becoming more familiar with bootstrap, but I'm not there yet.

Alex-Jordan commented 9 months ago

Now you can filter users by permission level or role.

Question: If you selected to filter by Section: Foo and Role: Enrolled, would you expect to get:

the students from section Foo who are Enrolled
all students from section Foo, union with all students who are Enrolled

You currently get the second outcome. To me, a "filter" is more of an intersection thing than a union thing, and I would expect the first outcome.

Which outcome is actually useful for a WeBWorK instructor?

Alex-Jordan commented 9 months ago

Another question. Are the "Section: ⟨blank⟩" and "Recitation: ⟨blank⟩" filters useful, or just clutter?

drgrice1 commented 9 months ago

Question: If you selected to filter by Section: Foo and Role: Enrolled, would you expect to get:

the students from section Foo who are Enrolled

all students from section Foo, union with all students who are Enrolled You currently get the second outcome. To me, a "filter" is more of an intersection thing than a union thing, and I would expect the first outcome. Which outcome is actually useful for a WeBWorK instructor?

I think that the most useful would be the intersection in the example given. However, for some other combinations of the filters a union is more useful. For example, if you want to view all users in two different sections, or in two different recitations.

Another question. Are the "Section: ⟨blank⟩" and "Recitation: ⟨blank⟩" filters useful, or just clutter?

I think that if you are a course coordinator working on assigning sections to all students, then it might be useful to filter by students with sections not yet set so you can clearly see which students you have left to assign sections to. So there are probably use cases that justify this filter.

drgrice1 commented 9 months ago

I also noticed that none of the scrolling record list texts are translated. Don't do that in this pull request though. Lets defer that for another one. I think there is enough going on with this for now. Note that this pull request does set up much of the structure needed to do the translations though, since the controller is now passed into the FormatRecords.pm and FilterRecords.pm methods.

drgrice1 commented 9 months ago

By the way, to test the inefficiency, you can create a course and import the attached class list file which contains 5000 users. With this pull request as is, I am seeing that it takes more than one second for a request that uses a permission filter to complete, while with my changes in my current pull request to this branch it decreases to less than a fifth of a second.

longDemoCourse.lst.txt

Alex-Jordan commented 9 months ago

I also noticed that none of the scrolling record list texts are translated.

I noticed that too. At that level, maketext() is not available and I opted not to investigate what is needed to get it for now. Sounds like you know what must be done.

Alex-Jordan commented 9 months ago

perltidy fixed.

I have one loose thread with this that I'm working on. If I finish it this morning I will push. Otherwise I'll sit on it for a small future PR.

Alex-Jordan commented 9 months ago

I added radio buttons to choose if you would like to intersect or union the filter results. In my testing, it is functional. Perhaps improvements can be made with the layout.

somiaj commented 9 months ago

The layout of the buttons needs improvement here.

Each button should be on its own row, and some padding is needed imo.

Alex-Jordan commented 9 months ago

Thanks @somiaj , that is in there now.

The word "Intersect" is long, and even on a mildly narrowed screen, will break onto the next line. It also happens with "Union". I'm OK with it but we could also use union and intersect symbols here as icons with a title that has the full word. Open to that or any better ideas.

openwebwork / webwork2

improvements to Instructor Tools #2266