Remove action scope and add action form validation.

[somiaj]

On the UserList, ProblemSetList, and AchievementList managers, remove the scope option that helps determine which items to act on, instead users will always select which items to act on and can use filters to change the list of items to select from. This address #1991.

In addition add javascript form validation that will create a modal popup if the form is missing information, such as no items are selected, a text string is not provided, a valid file is not selected, and so on.

Last, make the import tabs not create a form if no valid files are found to import from.

[drgrice1]

Here are a few things that I noticed with an initial look at the javascript files.

There is an issue with this pull request in general though. Basically, this is a reason that issue #1991 is not entirely valid. At least for the Accounts Manager page in part. I have been meaning to add a comment to that issue about this.

If there are more than 200 users in a course, then no users are shown on that page by default. So when you select the "Edit" tab, there is initially no way to select users to edit (for example) with this pull request. You now have to go to the filter tab and get some users to be shown on the page before you can do anything. That is not ideal. Previously when none of the users were selected, you could still edit "all users".

[drgrice1]

This was supposed to be in the last review, but I must not have clicked the button to add it.

[drgrice1]

Also note that using a modal dialog for form validation messages is a rather intrusive and ugly way of doing so. You should take a look at https://getbootstrap.com/docs/5.3/forms/validation/ for a better way of doing this sort of thing.

[somiaj]

Updated the javascript and moved the validation from a modal to in-form messaging and toggling the is-invalid style.

@drgrice1 I noticed the extra step when I was testing a class with 500+ users of having to first filter before acting on all users. I wasn't sure how big of an issue it would be, with a large class is it common to act on all users or first filter? Another option could be to leave the scope menus in the few places they are beneficial, such as the example stated.

[somiaj]

I also moved to disabling the import tab if no valid files are found to import from vs show a different form.

[drgrice1]

Updated the javascript and moved the validation from a modal to in-form messaging and toggling the is-invalid style.

I like this better. It is less intrusive, and it is good that you clear the validation message when the tabs change. Perhaps this could be taken further though and this could be used for the other validation messages also. For example, if the "Filter" tab is used with field matching and no text is entered that message could also be shown in this hidden div (and cleared when the tabs change).

@drgrice1 I noticed the extra step when I was testing a class with 500+ users of having to first filter before acting on all users. I wasn't sure how big of an issue it would be, with a large class is it common to act on all users or first filter? Another option could be to leave the scope menus in the few places they are beneficial, such as the example stated.

You generally don't want to edit all users if the course is large enough (say 5000 or more) as the page takes to long to load, and who would seriously want to manually edit that many users at one time anyway. So filtering is almost a necessity. I think that pagination should be used for dealing with large numbers of users instead of just not showing any. With proper pagination, all users (to which the current filter applies) could be available in pages and you could select users from multiple pages.

[drgrice1]

If you know how to do this, could you apply prettier to the javascript files that you have edited. I have started doing this when I change javascript files, and eventually I plan to add a workflow to check this (much like we use perltidy for perl files).

The contents of the .prettierrc file that I use for this (and plan to use for the workflow) are:

{
	"arrowParens": "always",
	"bracketSpacing": true,
	"printWidth": 120,
	"semi": true,
	"singleQuote": true,
	"tabWidth": 4,
	"trailingComma": "none",
	"useTabs": true
}

[somiaj]

I updated the javascript to remove all the error messages when switching tabs.

[somiaj]

Last commit removes none from the list of options to filter from. Since you need some users/sets to act on, I don't think this option is useful. I can revert if others think we should keep it.

[drgrice1]

There is another case on the UserList page (the accounts manager) that the removal of the scope selector is particularly bad with for large courses. That is the "Export" tab. Now if you have a large class and you want to export all users, the only way to do this is to first show all users and then select them all. For really large classes that can take a really long time for the browser to load all of that. Previously you could just choose the export "all users" option.

I think in general removing these scope selectors for the accounts manager is not a good idea, and will need to be added back. For the sets manager and the achievements manager it is fine, since those lists are usually not that large.

[somiaj]

I added the action scope back to the Edit, Password, and Export actions on the UserList. In this case it only has two options, either all or selected, since there is a select all checkbox for visible users. Though in practice it will probably only be the export option this is most likely used, since manually editing or changing the password of all users in a class of >200 isn't something that is probably done that often.

[drgrice1]

I think this is okay now. I agree that the "scope" selector is most important for the export form of the user list page. Although I think it should be there for the edit and password forms as well due to the fact that there is the case where all users are hidden by default for large courses.

[somiaj]

I did consider only showing the scope drop downs to Edit, Password, and Export users when less than the full list of students is shown, though unsure if that is useful.

[drgrice1]

At least for the export page I think that the option to select all users for export via the dropdown should always be there. Even if the class is not large. this makes sense as it allows not needing to deal with the checkboxes at all.

[drgrice1]

To be frank, I think that should be added back for the set manager export form as well. In fact, that is how I have always done it. It is just clear and concise, and really is better than needing to select all users. It should be there for the export form for achievements also.

[somiaj]

That logic could be extended to any place that scope use to be used, though export is the place it is most likely used.

As an alternative to removing scope completely, would be just reduce it to all and selected. Having none there isn't useful, and the checkboxes take care of the visible option. This would also be useful if you have done a filter, but want to quickly act on the full class without having to reset the filter. I will work on adding back this limited scope approach.

@dlglin do you have any input from your intent behind #1991?

[pstaabp]

I'm good with this. @drgrice1 I know you approved, but okay with merging now or any other changes?

[drgrice1]

Lets wait a bit on this. There are still open questions.

[somiaj]

I have updated this so that both the Accounts and Sets manager still have action scope, but it is limited to two options, either 'all' or 'selected' and 'selected' is always default. In essence only 'visible' and 'none' was removed (since this can be done via the checkboxes). My thoughts here are that on the Accounts and Sets manager can filter the options, so it could be useful to have a quick way to act on all users without having to update the filter (along with large classes not showing any users without filtering).

I have a patch to also add this back to the achievements manager too, but didn't add it here. Logic being there is no filter on the achievements page, so the checkboxes can be used to act on all achievements at any time. If others prefer to have the scope added back here too, let me know and I'll add that patch.

It would be nice if @dlglin weighs in on the original intent behind #1991

[drgrice1]

I think that this is better now. Allowing acting on all sets or users regardless of selection for the accounts and sets manager pages is good.

There is one detail that needs attention, and then this can be merged.

[dlglin]

When I posted #1991 I wasn't thinking about large classes where all students are not automatically displayed, so I see @drgrice1's point on it being extra work to have to show the whole list before exporting. My thought was to simplify the interface, since right now the user has to mess with two things: the dropdown and the checkboxes to get the subset of students/sets that they want.

There is one thing that I find counterintuitive with this setup: if you filter the list and then perform an action (e.g. export), you are given the option of "all users", which leads to exporting users that are not currently being shown.

I'm not sure if the following suggestion is too complicated, but would it make sense to have the options be "all" and "selected" when no filter is applied, but replace "all" with "displayed" if the list is filtered (with appropriate logic to only apply the action to the shown items)? Maybe that's beyond the scope of this PR.

[drgrice1]

I personally find that counterintuitive. To me it seems clear that "all" users means all users from the course, and not all visible users. Furthermore, I think the "all" option should always be there. If you do as you suggest, and make that switch to "displayed" when a filter is applied, then you are back to the how this initially was and you add steps needed to export all users again if you have previously applied a filter. That also seems counter to your initial issue that led to this pull request. What you can do with the "displayed" option can always be done with the check boxes.

[dlglin]

I'm having trouble envisioning a scenario where you would apply a filter, and then subsequently perform an action on all users. My intuition is that once I've filtered I'm only working on that subset. If I want to do something to all users I would expect that I would remove the filter.

[drgrice1]

I can. You might want to edit some users and so apply a filter to find those users in your large class. After doing so you might want to export all users.

There is also the issue that there really isn't a way to remove a filter other than to start from scratch by reloading the page.

[Alex-Jordan]

Can we do this with more clear labeling? Instead of "all" or "all users", something like "all course users" would (to me) clearly imply everyone, not just all the ones being displayed.

[drgrice1]

I am fine with that.

[Alex-Jordan]

I just tried this and it looks good. I suggest changing "all users" to "all course users" as mentioned in the thread, but mark for approval either way.

[somiaj]

@Alex-Jordan Should All sets be kept as is, or do you suggest updating that as well?

[somiaj]

I just noticed an issue with the validation javascript. It requires a user to be selected even when 'all course users' is selected. Fixing that now, so don't merge.

[Alex-Jordan]

@Alex-Jordan Should All sets be kept as is, or do you suggest updating that as well?

Oh, I realize now I was only looking in the Accounts Manager, not several other places this impacts. But sure, "all course sets" seems fine. Just looking for clarity between literally all of them versus all that are presently displayed.

Does "all course achievements" come up too?

[somiaj]

Because achievements don't have a filter action, there is no option to select between all course achievements and selected achievements, since the checkboxes already deal with this case. I have a patch to add that option to the achievements page if desired for consistency, but currently is not there.

[Alex-Jordan]

Because achievements don't have a filter action, there is no option to select between all course achievements and selected achievements, since the checkboxes already deal with this case. I have a patch to add that option to the achievements page if desired for consistency, but currently is not there.

I'd like to have that. Of course, it can be separate from this PR. The reason I'd like it is that I often load the default collection of achievements, but then go and delete the Challenge achievements, plus a few more. Filtering would just make that a little more smooth. (I should just export the sets that I actually use into an achievement collection file, but then I still have colleagues to mentor, etc.)

[drgrice1]

Since a filter on the achievements list page is new, that is something that I think should be in a separate pull request.

[somiaj]

I can not remove the action scope from achievements here in anticipation of such a change, but agree it should be in a different PR.

[somiaj]

Might be worth looking over one last time. I noticed I overlooked the javascript interacting with the all/selected toggle, so I improved that. I also added the all/selected scope back to the achievements page for consistency and if a filter ever does get added in a future PR.

[dlglin]

Another observation: when I first looked at this I was confused as to why the Import tab was greyed out. Is there a good way to let the user know that in order to import Sets/Users that they need to first upload a file via the file manager?

It would be nice if the instructor could upload a .def/.lst file here directly, but that's beyond the scope of this PR, so it's captured in #2325.

[drgrice1]

I looked this over another time to be sure, but I think this pull request is ready.

As to @dlglin's comment about the import tab being grayed out, perhaps a comment could be added to the help file about that. I don't think that is a particularly serious issue though, as the typical course will have .def files in the templates directory, since some are copied from the model course in the default course creation process.

I personally think that import/export should ONLY be done the way that @dlglin suggested. Files are imported from a local client side file, and files are exported for download. There is no need for the clutter in the courses templates directory of set definition and classlist files (or even achievement .axp files). For exporting multiple set definition files, a zip file could be offered.

[drgrice1]

We are going to merge this. If someone wants to add a comment in the help regarding the grayed out import tabs, that might be nice.