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

Autocomplete usernames when inviting students and teachers #6032

Open
wants to merge 45 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
a723cf8
add search endpoint
jpelay Dec 10, 2024
d2d25fe
undo unwated change
jpelay Dec 10, 2024
b4f6348
improe ui a bit
jpelay Dec 10, 2024
7873c27
Merge branch 'main' into search_usernames
jpelay Dec 10, 2024
edda8d4
update events
jpelay Dec 12, 2024
0f3c014
focus on input field after htmx requets completes
jpelay Dec 12, 2024
789af85
add list of students
jpelay Dec 13, 2024
021fee9
send empty array on empty strng
jpelay Dec 13, 2024
7ffb480
use grid instead of flex make width bigger
jpelay Dec 20, 2024
c7230df
Merge branch 'main' into search_usernames
jpelay Dec 27, 2024
7e7c031
merge from main
jpelay Dec 27, 2024
ca1a443
add filter to check if a set is emtpy or is not within a record
jpelay Jan 10, 2025
95105aa
fix sort key in users table and add query
jpelay Jan 10, 2025
98a9c87
delete search students route from app.py
jpelay Feb 6, 2025
663d913
add new search modal
jpelay Feb 6, 2025
64c7570
add functions in teachers to search usernames
jpelay Feb 6, 2025
4583954
modify templates to include new modal
jpelay Feb 6, 2025
d6b97ed
add new dynamo condition
jpelay Feb 6, 2025
33f1af8
add function to search and add invitations on classes py
jpelay Feb 6, 2025
7c0de85
search functions
jpelay Feb 6, 2025
f59758d
automatically generated files
jpelay Feb 6, 2025
7699527
🤖 Automatically update generated files
jpelay Feb 6, 2025
f51a50a
Merge branch 'main' into search_usernames
jpelay Feb 6, 2025
6196fbe
🤖 Automatically update generated files
jpelay Feb 6, 2025
9b71abf
fixing prompt modal
jpelay Feb 7, 2025
e86309f
fix query that gets all users
jpelay Feb 7, 2025
266c548
🤖 Automatically update generated files
jpelay Feb 7, 2025
d43c232
restructure functions around using HTMX
jpelay Feb 13, 2025
fff9674
modify modal to clean up upon hitting ok or cancel
jpelay Feb 13, 2025
6a92120
use use htmx when obtaining inviting the list of usernames
jpelay Feb 13, 2025
6b4631d
Merge branch 'search_usernames' of https://github.com/hedyorg/hedy in…
jpelay Feb 13, 2025
600356f
🤖 Automatically update generated files
jpelay Feb 13, 2025
ad41ee1
add babel label
jpelay Feb 13, 2025
76aec99
Merge branch 'search_usernames' of https://github.com/hedyorg/hedy in…
jpelay Feb 13, 2025
c4d28c3
automatically generated files
jpelay Feb 13, 2025
a13c024
🤖 Automatically update generated files
jpelay Feb 13, 2025
1571862
Merge branch 'main' into search_usernames
jpelay Feb 13, 2025
416cf3c
fix merge
jpelay Feb 13, 2025
f7dc9fe
Merge branch 'search_usernames' of https://github.com/hedyorg/hedy in…
jpelay Feb 13, 2025
75c9df1
🤖 Automatically update generated files
jpelay Feb 13, 2025
af827cf
fix tests... again
jpelay Feb 14, 2025
7d046fd
Merge branch 'search_usernames' of https://github.com/hedyorg/hedy in…
jpelay Feb 14, 2025
5535cd6
add labels
jpelay Feb 14, 2025
3c7bdc4
Merge branch 'main' into search_usernames
jpelay Feb 14, 2025
3672693
update autogenerated files
jpelay Feb 14, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions app.py
Original file line number Diff line number Diff line change
Expand Up @@ -1853,6 +1853,14 @@ def tryit(level, program_id):
))


@app.route('/search_students', methods=['GET'])
def filter_usernames():
search = request.args.get('search', '')
results = DATABASE.get_users_that_starts_with(search)
Copy link
Collaborator

@rix0rrr rix0rrr Dec 10, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh for sure! I was thinking about that yesterday, I'm only going to look data if I there's more than one character.

usernames = [record['username'] for record in results]
return render_template('modal/results_reply.html', usernames=usernames)


@app.route('/hedy', methods=['GET'])
def index_level():
if current_user()['username']:
Expand Down
15 changes: 14 additions & 1 deletion templates/layout.html
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,20 @@ <h2>{{ _('levels')|capitalize }}</h2>
<div id="modal_prompt" class="hidden">
<div id="modal_prompt_text" class="text-white mb-8 text-xl"></div>
<div class="xm-auto">
<input id="modal_prompt_input" data-cy="modal_prompt_input" type="text" class="border border-green-400 rounded p-2 px-3 w-4/5 mt-1">
<input id="modal_prompt_input"
data-cy="modal_prompt_input"
type="search"
name="search"
class="border border-green-400 rounded p-2 px-3 w-4/5 mt-1"
hx-get="/search_students"
hx-trigger="input changed delay:500ms, search"
hx-target="#search_results"
hx-indicator=".htmx-indicator"
>
<ul class="list-none flex flex-col items-center align" id="search_results">

</ul>
</div>
<div class="mt-4 flex flex-row justify-center">
<button id="modal_ok_button" data-cy="modal_ok_button" class="green-btn block m-4 w-40 pb-4 pt-4">{{_('ok')}}</button>
<button id="modal_cancel_button" data-cy="modal_cancel_button" class="red-btn block m-4 w-40 pb-4 pt-4">{{_('cancel')}}</button>
Expand Down
3 changes: 3 additions & 0 deletions templates/modal/results_reply.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{% for username in usernames %}
<li class="p-4 m-1 bg-blue-100 rounded cursor-pointer w-4/5" >{{ username}}</li>
{% endfor %}
6 changes: 5 additions & 1 deletion website/database.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

@rix0rrr rix0rrr Dec 10, 2024

Choose a reason for hiding this comment

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

No please.

Suggested change
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.

Copy link
Collaborator

@rix0rrr rix0rrr Dec 10, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@rix0rrr rix0rrr Dec 10, 2024

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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!

]
)
self.tokens = dynamo.Table(storage, 'tokens', 'id',
Expand Down Expand Up @@ -1156,6 +1157,9 @@ def get_username_role(self, username):
role = "teacher" if self.users.get({"username": username}).get("is_teacher") == 1 else "student"
return role

def get_users_that_starts_with(self, search):
return self.users.get_many({'username': dynamo.BeginsWith(search)}, limit=10)


def batched(iterable, n):
"Batch data into tuples of length n. The last batch may be shorter."
Expand Down
Loading