Add group lookup functionality
Mergedr389li merged 12 commits from
master4 weeks ago
Reference in new issue
There is no content yet.
Delete Branch '60-group-lookup'
Deleting a branch is permanent. It CANNOT be undone. Continue?
note: I am unaware of best practices but I tried my best to keep changes consistent with the codebase
feedback would be much appreciated
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 thanks for getting on this! Would you mind fixing the CI tests?
@r345liu could you help review?
0655d4ceec3 months ago
done! should I also write some tests as well?
d3cad58a8e1 month ago
d4e724c9641 month ago
d4e724c9641 month ago
e99ef5203f1 month ago
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.
def search_group(query, count):
# compute levenshtein edit distance, adapted from rosetta code
def _fuzzy_match(s1, s2):
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()
So this isn't related to your PR, but I believe there is currently a bug in the
LDAPService.get_clubsimplementation 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_clubsimplementation? Feel free to ask on IRC if you have any questions.
Please and thank you 😄
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.
d3f23cf31d1 month ago
3ff156e6ce1 month ago
Done! For the LDAP it seemed sufficient to replace the search in the
_get_club_uidfunction 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.
Looks great! Thank you for taking this on!
Let's fix those lint errors, and then this should be ready to get merged.
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.
010937ea17into master 4 weeks ago