Autocomplete usernames when inviting students and teachers #6032
Conversation
|
This is how we are going so far, still a bit of work to be done, but the search is already there 2024-12-09.20-18-46.mp4 |
website/database.py
Outdated
| @@ -109,7 +109,8 @@ def __init__(self): | |||
| }), | |||
| indexes=[ | |||
| dynamo.Index('email'), | |||
| dynamo.Index('epoch', sort_key='created') | |||
| dynamo.Index('epoch', sort_key='created'), | |||
| dynamo.Index('username', sort_key='epoch', keys_only=True) | |||
There was a problem hiding this comment.
No please.
| dynamo.Index('username', sort_key='epoch', keys_only=True) | |
| dynamo.Index('epoch', sort_key='username', keys_only=True) |
Also, putting a Condition on a PK doesn't make sense. A partition must always be matched with full key equality. If the DynamoDB layer allowed you to formulate the query below, something is wrong
EDIT: There was a bug that would only manifest for tables with only a partition key and not a sort key. In that case, the protection that should give you an error when trying to put a condition onto the PK wouldn't work. I fixed it in the linked PR.
There was a problem hiding this comment.
Oh I see, the abstraction layer complains if you use the same partition key multiple times so you were forced to reverse the keys to get it to do anything at all.
That restriction is not strictly necessary. It's not enforced by Dynamo, but by our abstraction layer for the convenience of identifying which index to query. Let me see what I can do about that.
There was a problem hiding this comment.
Here is a PR you will need to merge before you can set up the indexes the way you need them: #6034. Unfortunately, this will also require you to annotate all existing epoch queries to indicate what index to use. I know at least one of them in the admin interface, and there aren't any tests on those, so do have a good look around.
The motivation for the design decisions is in the linked PR.
(BTW I created the necessary (epoch, username) indexes in the actual DDB tables already, so you don't need to do anything there anymore)
There was a problem hiding this comment.
Oh I see, the abstraction layer complains if you use the same partition key multiple times so you were forced to reverse the keys to get it to do anything at all.
That restriction is not strictly necessary. It's not enforced by Dynamo, but by our abstraction layer for the convenience of identifying which index to query. Let me see what I can do about that.
Yes, so I thought I was making a mistake. Thanks for pointing it out and fixing it.
(BTW I created the necessary (epoch, username) indexes in the actual DDB tables already, so you don't need to do anything there anymore)
Neat!
app.py
Outdated
| @app.route('/search_students', methods=['GET']) | ||
| def filter_usernames(): | ||
| search = request.args.get('search', '') | ||
| results = DATABASE.get_users_that_starts_with(search) |
There was a problem hiding this comment.
Probably you want to only do an actual search once you've gotten at least one character.
Otherwise the first query will always return the same first 10 items from the users table ["000_test", "4242 i like kittens", "aaron a. aaronson", ...]. Not useful, and people will get tired of it.
Just return an empty list otherwise.
There was a problem hiding this comment.
Oh for sure! I was thinking about that yesterday, I'm only going to look data if I there's more than one character.
|
I have to deal with the conflicts, but this is done :) Sorry it took so long! |
Adds autocomplete functionality for searching usernames when adding a student or a second teacher.
It does so by matching the usernames in the database with the user's query, so if search for: user, it'll show me: user1, user2, user3, etc.
In the process of doing this I wanted to use a bit more of the power of Dynamo, instead of relying on filtering the data trhough python. To this end I added 2 extra Dynamo conditions: one for checking if a set exits, and another for checking if an element is not in a container.
I also updated HTMX and Tailwind, but maybe I can update those in a different PR?
I tried to improve the code a little bit also by avoiding duplication where I could, so instead of having 2 function for adding teachers and students I merged them together since they were mostly the same.
Fixes #4689
How to test