Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change achievement categrory selection to dropdown #13

Merged
merged 1 commit into from
Mar 1, 2024

Conversation

Alex-Jordan
Copy link

This makes the category selection offer a dropdown with only the available categories.

@Alex-Jordan
Copy link
Author

Although, this is not ideal. After selecting a category and submitting, then only the displayed category is available in the dropdown list. Not sure yet how to recover the list of all available categories.

@somiaj
Copy link
Owner

somiaj commented Feb 29, 2024

The only way I can think of was to just load all records at the start, then just loop/filter them with grep. The old transitional code I removed was already looping through all achievement records and there probably won't be too many achievements, so just loading them all is probably okay. I just wasn't sure how necessary that is if someone could just type out the category (even though a drop down would be nicer).

@Alex-Jordan
Copy link
Author

I'll update this soon.

@Alex-Jordan
Copy link
Author

OK, now there is a DB utility to list all the achievement categories and it puts them in the select using that utility.

@somiaj
Copy link
Owner

somiaj commented Feb 29, 2024

Works well, only thing I see is since the drop down always contains a valid category, the javascript validation for the filter can be removed for this case.

My thoughts were a bit more extensive, since I think we are doubling up on database calls here. Since we are getting all achievement records anyways when calling getAchievementCategories, would it be worthwhile just doing this in the initialize function, and then we have all the records, and can use them (vs making more database calls) in AchievementsList.pm when showing/filtering achievements, etc.

But your method lead me to another thought, what about let the database do the filtering (so we aren't really getting every possible record), instead have a listAllAchievementCategories method which does a database call like SELECT DISTINCT category FROM achievements_table, which gives us the list we want.

Anyways, once the javascript validation for this filter category is removed I'll merge, unless you think it is wort while addressing my other comments.

@somiaj somiaj closed this Feb 29, 2024
@somiaj somiaj reopened this Feb 29, 2024
@somiaj
Copy link
Owner

somiaj commented Feb 29, 2024

Opps, clicked the wrong button.

@somiaj
Copy link
Owner

somiaj commented Feb 29, 2024

To remove the javascript validation just delete lines 68 and 75-78 in achievementlist.js, seems you have already taken out the error message div.

@Alex-Jordan
Copy link
Author

I'd like to query like

SELECT DISTINCT category FROM coursename_achievement;

but I don't find any existing use of a DISTINCT query in the database code. There is a select method that is used, and maybe it has an option that supports distinct, or maybe I could extend it to support distinct, but I can't find where it is defined.

@somiaj
Copy link
Owner

somiaj commented Feb 29, 2024

I also couldn't figure it out. The only place I see DISTINCT is in lib/WeBWorK/Utils/ListingDB.pm, but these are doing raw sql queries vs using the database object. I didn't see any clear way using the database object and don't know it well enough.

@Alex-Jordan
Copy link
Author

I think I see how to hack it in. Maybe the hack can be developed further once more eyes are on it.

@Alex-Jordan
Copy link
Author

OK, it comes from SQL::Abstract::Classic...

@Alex-Jordan
Copy link
Author

Alright I got it working so the database does the "distinct" part. And I don't think it's hackish.

Copy link
Owner

@somiaj somiaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted the wrong line here (my bad, I guess I read the line number incorrectly). Delete the line above this, 66, const filter_category = ....

Comment on lines 66 to 68
const filter_category = document.getElementById('filter_category');
if (filter === 'selected' && !is_achievement_selected()) {
e.preventDefault();
e.stopPropagation();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the lines, unsure why they didn't show up in my last review.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I added them (er deleted them) after your comment and then force pushed.

@Alex-Jordan
Copy link
Author

And now sorted on the database end, not the perl end.

@Alex-Jordan
Copy link
Author

OK, updated the js lines.

@somiaj
Copy link
Owner

somiaj commented Mar 1, 2024

Looks good now.

@somiaj somiaj merged commit 674c724 into somiaj:achievement-filter Mar 1, 2024
2 checks passed
somiaj pushed a commit that referenced this pull request Jun 24, 2024
Help updates to InstructorIndex and InstructorProblemGrader.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants