Add group lookup functionality #88
Merged
r389li
merged 12 commits from 60-group-lookup
into master
3 months ago
Loading…
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
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 possibletui 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?
7ea8452e61
to0655d4ceec
5 months agodone! should I also write some tests as well?
3c87f5b1b3
tod3cad58a8e
4 months agod3cad58a8e
tod4e724c964
4 months agoa324c4ddd7
tod4e724c964
4 months agod4e724c964
toe99ef5203f
4 months agoI 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.
@bp.route('/search/<query>/<count>')
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_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 😄
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.
596d1ef4eb
tod3f23cf31d
3 months agod3f23cf31d
to3ff156e6ce
3 months agoDone! 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.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.
010937ea17
into master 3 months agoReviewers
010937ea17
.