public
/
pyceo
25
1
Fork 3
Code Issues 26 Pull Requests 5 Projects Releases 1 Wiki Activity

Add group lookup functionality #88

Merged
r389li merged 12 commits from 60-group-lookup into master 2023-03-04 01:21:06 -05:00
Conversation 7 Commits 12 Files Changed 16 +439 -1
d6sun commented 2022-12-23 12:32:33 -05:00
Contributor
Copy Link

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 2022-12-23 12:32:34 -05:00
continuous-integration/drone/pr Build is failing Details
718588d29a update openapi.yaml
r389li commented 2022-12-31 17:20:58 -05:00
Owner
Copy Link

@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 2023-01-09 14:33:34 -05:00
continuous-integration/drone/pr Build is failing Details
7ea8452e61 fix ci
d6sun force-pushed 60-group-lookup from 7ea8452e61 to 0655d4ceec 2023-01-09 14:45:38 -05:00 Compare
d6sun commented 2023-01-09 15:00:25 -05:00
Author
Contributor
Copy Link

done! should I also write some tests as well?

done! should I also write some tests as well?
d6sun added 1 commit 2023-01-25 23:46:00 -05:00
d6sun added 1 commit 2023-02-15 17:52:44 -05:00
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 2023-02-15 18:19:49 -05:00 Compare
d6sun force-pushed 60-group-lookup from d3cad58a8e to d4e724c964 2023-02-15 20:53:27 -05:00 Compare
d6sun added 9 commits 2023-02-15 21:37:43 -05:00
b25e371329 use bullseye for base container (#91) 
All of the machines running ceod are on bullseye, so we don't need to support buster anymore.

Reviewed-on: #91
Co-authored-by: Max Erenberg <merenber@csclub.uwaterloo.ca>
Co-committed-by: Max Erenberg <merenber@csclub.uwaterloo.ca>
a5a6bff4e6 reload all NGINX servers after adding a vhost (#90) 
Currently, only the NGINX server on biloba is reloaded after adding a new vhost or renewing an SSL certificate. The NGINX server on chamomile should also be reloaded, since chamomile is a warm standby for biloba.

This PR adds a new config option in ceod.ini to specify the shell command to reload the web servers.

Reviewed-on: #90
Co-authored-by: Max Erenberg <merenber@csclub.uwaterloo.ca>
Co-committed-by: Max Erenberg <merenber@csclub.uwaterloo.ca>
8b6eb60cb7 Implement TUI support for multiple users in each position (#80) 
Co-authored-by: Justin Chung <20733699+justin13888@users.noreply.github.com>
Co-authored-by: Max Erenberg <merenber@csclub.uwaterloo.ca>
Reviewed-on: #80
Co-authored-by: Justin Chung <j24chung@csclub.uwaterloo.ca>
Co-committed-by: Justin Chung <j24chung@csclub.uwaterloo.ca>
continuous-integration/drone/pr Build is passing Details
a324c4ddd7 release 1.0.26
d6sun force-pushed 60-group-lookup from a324c4ddd7 to d4e724c964 2023-02-15 21:40:45 -05:00 Compare
d6sun force-pushed 60-group-lookup from d4e724c964 to e99ef5203f 2023-02-15 22:10:44 -05:00 Compare
d6sun commented 2023-02-15 22:25:29 -05:00
Author
Contributor
Copy Link

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 2023-02-16 17:14:54 -05:00
ceod/api/groups.py
@ -35,0 +38,4 @@
@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 2023-02-16 17:07:33 -05:00
Owner
Copy Link

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
@ -35,0 +84,4 @@
query = str(query)
count = int(count)
ldap_srv = component.getUtility(ILDAPService)
clubs = ldap_srv.get_clubs()
merenber commented 2023-02-16 17:14:36 -05:00
Owner
Copy Link

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 2023-02-16 17:16:05 -05:00
Owner
Copy Link

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 2023-02-19 22:53:10 -05:00
d6sun force-pushed 60-group-lookup from 596d1ef4eb to d3f23cf31d 2023-02-19 23:04:32 -05:00 Compare
d6sun force-pushed 60-group-lookup from d3f23cf31d to 3ff156e6ce 2023-02-20 14:26:10 -05:00 Compare
d6sun commented 2023-02-20 14:59:11 -05:00
Author
Contributor
Copy Link

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 2023-02-20 14:59:25 -05:00
merenber commented 2023-02-25 17:33:55 -05:00
Owner
Copy Link

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 2023-02-25 17:42:34 -05:00
Owner
Copy Link

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 2023-02-25 17:42:52 -05:00
d6sun added 1 commit 2023-03-02 10:06:54 -05:00
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 2023-03-02 13:08:28 -05:00
merenber approved these changes 2023-03-03 17:27:16 -05:00
r389li merged commit 010937ea17 into master 2023-03-04 01:21:06 -05:00
Sign in to join this conversation.
Reviewers
No reviewers
merenber
Labels
Clear labels   
priority
high
High priority

  
priority
low
Low priority

  
priority
medium
Medium priority

  
priority
very high
Very high priority

  
BUG

   
Feature

   
High Priority

   
Low Priority

   
Medium Priority
No Label
priority
high
priority
low
priority
medium
priority
very high
BUG
Feature
High Priority
Low Priority
Medium Priority
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: public/pyceo#88
No description provided.
Delete Branch "60-group-lookup"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?