Add filter action to achievements manager.

[somiaj]

This adds a action to filter achievements as suggested by @Alex-Jordan.

[drgrice1]

There are a few details that need attention.

If a filter happens to filter out all achievements, then the message shown is "No achievements shown. Create an achievement!" For example, in my test course all achievements are enabled. So when I selected the "disabled achievements" filter, that gave me none. That message is not appropriate anymore if there are achievements, but none are shown because of the filter.

[drgrice1]

This looks good now. Everything seems to be working in my tests.

[pstaabp]

Overall, looks good. Seems like the filter by category should be a dropdown/select list of categories, instead of requiring it to be typed in exactly. The default list only has 6 categories, and this should be easy to pull out of the list.

[somiaj]

@pstaabp I thought about that, but the only way I could think to get the list of all categories is to first pull all achievements from the database and cycle through them getting all categories. But once a filter is applied not all achievements are pulled from the database, only the filtered ones.

[drgrice1]

There are some real problems with the javascript that you are using. This goes back to what you did in #2294.

You are repeatedly adding event handlers to various elements each time the form is submitted. Those event handlers pile up. Note that the once: true option makes the event handler go away if it is called, but each time the form is submitted while that has not been called another event handler is added. Eventually all of those fire at the same time if the event does occur.

Also you getElementById in many cases where you have already obtained a reference to that element. That is an expensive query. Each time it is called, the entire DOM has to be searched until that element is found. For example on line 24 of achievementlist.js you find the filter_select element (and even save a reference in a variable), and then on line 36 you search the DOM again, instead of just using the already found reference. Several places you just blindly search the DOM each time without saving a reference. For example, on line 43 you find the filter_text_err_msg, and then on line 49 you search for it again.

I mentioned the redundancy in the code before, but this is just a total kludge really. The achievementlist.js file is 233 lines long, merely to validate a few form fields. That is really excessive.

[somiaj]

I have been thinking about ways to clean up the code and remove the redundancy, though unsure on the best approach here. My thought was to have helper functions do the work.

As for the change event methods, the reason I was calling getElementById again is I didn't realize that inside the event function I had access to the previously defined variables. Now that I know that I do have access, I'll update things to avoid calling getElementById multiple times when not needed.

I'm not quite sure the what to do about multiple events, it sounds like I should also be removing these events to avoid them piling up when one of the companion events fires?

[somiaj]

I will have time to look at this and see what I can come up with this weekend.

[drgrice1]

I did a little with the multiple event thing for userlist.js in #2335. Look at what I did there for a start.

[somiaj]

@drgrice1 There is my approach at reducing the code duplication and adding/removing event listeners, so they don't pile up. If you prefer this, and any code cleanup you may see, I can apply this to the other two pages as well (though maybe I should leave userlist.js alone since you have gotten it in your PR).

[drgrice1]

Yeah, that looks considerably better. Go ahead and do that for the other two pages as well. I will work my changes to userlist.js into your scheme.

[somiaj]

I have updated the other two action form validation location to match what was done here.

[somiaj]

There was some achievement transition code that was suggested to remove around 2017....I can open another PR for that if preferred.

[drgrice1]

Deleting that code is fine.

The updated javascript is considerably better. Perhaps in the future it would be nice to do this more uniformly for all pages that use action tabs (and the actiontabs.js file). But this is good for now.

[drgrice1]

I missed a few things before.

I see that you removed the { once: true } option to the error hiding event listeners. That is needed. Otherwise the handler will continue to be called even after the form validation messages have been cleared any time a checkbox is checked or unchecked.

[drgrice1]

This is better, and works to prevent handlers for events from piling up.

That stack exchange post you linked to is certainly not accurate. You have to read the comments responding to it to really see what is needed.

In the code I was using in userlist.js I was only dealing with one handler being added, and so didn't need something more complicated. Although, as mentioned in that #2335, there was more work needed. I saw that there were other event listeners that would need similar handling.

[Alex-Jordan]

Thanks for this. I was testing it, and I thought I would make the category selection be a dropdown. I opened a PR to your branch (@somiaj ):

somiaj#13

[somiaj]

@Alex-Jordan updated this so the category filter is now a drop down menu instead of a text field.

[pstaabp]

Already 2 approvals, but this looks better to me now.

[drgrice1]

I will merge this with 2+1 approvals.

[drgrice1]

@Alex-Jordan: I realized I didn't look closely at your commit at the end of this. I just saw this. This is a nice. A good new database method.