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 |
| @@ -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.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.
Adds autocomplete functionality for searching usernames.
Fixes #4689
How to test
WIP