Add group lookup functionality #88

Merged
r389li merged 12 commits from 60-group-lookup into master 3 months ago
d6sun commented 5 months ago
Collaborator

note: I am unaware of best practices but I tried my best to keep changes consistent with the codebase

feedback would be much appreciated

notable changes:
new api endpoint: /groups/search -- I moved searching into the api so it could be used in tui and cli, also seemed like a good idea to keep the json response as small as possible
tui searching -- at first I wanted to make this realtime interactable, but the work required seemed inappropriate to a feature I am assuming will only be used sparingly

note: **I am unaware of best practices** but I tried my best to keep changes consistent with the codebase feedback would be much appreciated notable changes: **new api endpoint**: `/groups/search` -- I moved searching into the api so it could be used in tui and cli, also seemed like a good idea to keep the json response as small as possible **tui searching** -- at first I wanted to make this realtime interactable, but the work required seemed inappropriate to a feature I am assuming will only be used sparingly
d6sun added 5 commits 5 months ago
Owner

@d6sun thanks for getting on this! Would you mind fixing the CI tests?

@r345liu could you help review?

@d6sun thanks for getting on this! Would you mind fixing the CI tests? @r345liu could you help review?
d6sun added 1 commit 5 months ago
7ea8452e61 fix ci
d6sun force-pushed 60-group-lookup from 7ea8452e61 to 0655d4ceec 5 months ago
Poster
Collaborator

done! should I also write some tests as well?

done! should I also write some tests as well?
d6sun added 1 commit 4 months ago
d6sun added 1 commit 4 months ago
3c87f5b1b3 add api tests and fix ci
d6sun force-pushed 60-group-lookup from 3c87f5b1b3 to d3cad58a8e 4 months ago
d6sun force-pushed 60-group-lookup from d3cad58a8e to d4e724c964 4 months ago
d6sun added 9 commits 4 months ago
b25e371329 use bullseye for base container (#91)
a5a6bff4e6 reload all NGINX servers after adding a vhost (#90)
8b6eb60cb7 Implement TUI support for multiple users in each position (#80)
a324c4ddd7 release 1.0.26
d6sun force-pushed 60-group-lookup from a324c4ddd7 to d4e724c964 4 months ago
d6sun force-pushed 60-group-lookup from d4e724c964 to e99ef5203f 4 months ago
Poster
Collaborator

I wrote some tests for the API endpoint. As far as I can see tests don't seem to be required for the cli or tui. I think the only thing left is for the pull request to be reviewed.

I wrote some tests for the API endpoint. As far as I can see tests don't seem to be required for the cli or tui. I think the only thing left is for the pull request to be reviewed.
merenber requested changes 4 months ago
@bp.route('/search/<query>/<count>')
def search_group(query, count):
# compute levenshtein edit distance, adapted from rosetta code
def _fuzzy_match(s1, s2):
Owner

The code related to fuzzy searching should be in a separate file, just so we can keep the API routes tidy.

The code related to fuzzy searching should be in a separate file, just so we can keep the API routes tidy.
query = str(query)
count = int(count)
ldap_srv = component.getUtility(ILDAPService)
clubs = ldap_srv.get_clubs()
Owner

So this isn't related to your PR, but I believe there is currently a bug in the LDAPService.get_clubs implementation which does not take into account the fact that our OpenLDAP server has a limit on the number of records returned 😅. We'll probably to need to implement a paginated search (a batch size of 50 should be safe).

Could you please update the get_clubs implementation? Feel free to ask on IRC if you have any questions.

Please and thank you 😄

So this isn't related to your PR, but I believe there is currently a bug in the `LDAPService.get_clubs` implementation which does not take into account the fact that our OpenLDAP server has a limit on the number of records returned :sweat_smile:. We'll probably to need to implement a paginated search (a batch size of 50 should be safe). Could you please update the `get_clubs` implementation? Feel free to ask on IRC if you have any questions. Please and thank you :smile:
Owner

As far as I can see tests don't seem to be required for the cli or tui

Could we add one unit test for the CLI? Just as a sanity check.

TUI tests are much harder so we don't need to worry about those.

> As far as I can see tests don't seem to be required for the cli or tui Could we add one unit test for the CLI? Just as a sanity check. TUI tests are much harder so we don't need to worry about those.
d6sun added 3 commits 3 months ago
d6sun force-pushed 60-group-lookup from 596d1ef4eb to d3f23cf31d 3 months ago
d6sun force-pushed 60-group-lookup from d3f23cf31d to 3ff156e6ce 3 months ago
Poster
Collaborator

Done! For the LDAP it seemed sufficient to replace the search in the _get_club_uid function with a paginated search, since the get_clubs function already did a batched search using the club_uids. I've also added a cli test as requested. I wasn't sure where to move the fuzzy searching code so I decided on ceo_common/utils.py since it looked empy.

Done! For the LDAP it seemed sufficient to replace the search in the `_get_club_uid` function with a paginated search, since the get_clubs function already did a batched search using the club_uids. I've also added a cli test as requested. I wasn't sure where to move the fuzzy searching code so I decided on ceo_common/utils.py since it looked empy.
d6sun requested review from merenber 3 months ago
Owner

Looks great! Thank you for taking this on!

Let's fix those lint errors, and then this should be ready to get merged.

Looks great! Thank you for taking this on! Let's fix those lint errors, and then this should be ready to get merged.
Owner

Never mind, it seems like the CI doesn't run when commits are force-pushed. I ran the tests locally and they're passing, so I'll approve this now.

I'll let you do the honours of merging.

Never mind, it seems like the CI doesn't run when commits are force-pushed. I ran the tests locally and they're passing, so I'll approve this now. I'll let you do the honours of merging.
merenber approved these changes 3 months ago
d6sun added 1 commit 3 months ago
67cf42d2bc Merge branch 'master' into 60-group-lookup
d6sun scheduled this pull request to auto merge when all checks succeed 3 months ago
merenber approved these changes 3 months ago
r389li merged commit 010937ea17 into master 3 months ago

Reviewers

merenber approved these changes 3 months ago
continuous-integration/drone/pr Build is passing
The pull request has been merged as 010937ea17.
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: public/pyceo#88
Loading…
There is no content yet.