public
/
pyceo
26
1
Fork 2
Code Issues 14 Pull Requests 4 Projects Releases 1 Wiki Activity
Labels Milestones

Add group lookup functionality #88

Merged
r389li merged 12 commits from 60-group-lookup into master 3 months ago
Conversation 7 Commits 12 Files Changed 16
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
continuous-integration/drone/pr Build is failing
Details
718588d29a update openapi.yaml
r389li commented 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
continuous-integration/drone/pr Build is failing
Details
7ea8452e61 fix ci
d6sun force-pushed 60-group-lookup from 7ea8452e61 to 0655d4ceec 5 months ago
d6sun commented 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
continuous-integration/drone/pr Build is failing
Details
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)
continuous-integration/drone/pr Build is passing
Details
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
d6sun commented 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
ceod/api/groups.py
@bp.route('/search/<query>/<count>')
def search_group(query, count):
# compute levenshtein edit distance, adapted from rosetta code
def _fuzzy_match(s1, s2):
merenber commented 4 months ago
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.
ceod/api/groups.py Outdated
query = str(query)
count = int(count)
ldap_srv = component.getUtility(ILDAPService)
clubs = ldap_srv.get_clubs()
merenber commented 4 months ago
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:
merenber commented 4 months ago
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
d6sun commented 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
merenber commented 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.
merenber commented 3 months ago
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
continuous-integration/drone/pr Build is passing
Details
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
Details
The pull request has been merged as 010937ea17.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
merenber
Labels
Apply labels
Clear labels
No items
No Label
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: public/pyceo#88
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch '60-group-lookup'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes