Move to new CSH LDAP ORM #104
Conversation
stevenmirabito
left a comment
stevenmirabito
left a comment
There was a problem hiding this comment.
Looks really good, with some minor suggestions in a few places. Thanks for doing this!
conditional/__init__.py
Outdated
| free_the_zoo(app.config['ZOO_DATABASE_URI']) | ||
|
|
||
|
|
||
| app.run(host=app.config['IP'], port=app.config['PORT']) |
There was a problem hiding this comment.
This line should remain in app.py with the if __name__ == "__main__" check to keep compatibility with other web servers.
There was a problem hiding this comment.
Yup, had to move stuff around so that LDAP initialized before the app started and I never moved it back.
| def ldap_set_housingpoints(username, housing_points): | ||
| _ldap_set_field(username, 'housingPoints', housing_points) | ||
| def ldap_set_housingpoints(account, housing_points): | ||
| account.housingPoints = housing_points |
There was a problem hiding this comment.
We should actually check if config['LDAP_RO'] is set here (and for the rest of the write functions) instead of ignoring it.
conditional/blueprints/dashboard.py
Outdated
| if ldap_is_intromember(user_name): | ||
| data['freshman'] = get_freshman_data(user_name) | ||
| if ldap_is_intromember(member): | ||
| data['freshman'] = get_freshman_data(username) |
There was a problem hiding this comment.
In my opinion, we should do a ldap_get_member lookup based on whatever criteria (in this case, UID from Webauth) like you're doing above, then use that member object's attributes instead of continuing to use the header value. This will also aid in the transition away from Webauth.
There was a problem hiding this comment.
👍 I think this is one of the only places this is seen, but I agree nonetheless.
| import ldap.modlist | ||
| from ldap.ldapobject import ReconnectLDAPObject | ||
|
|
||
| # Global state is gross. I'm sorry. |
| @lru_cache(maxsize=1024) | ||
| def ldap_get_housing_points(username): | ||
| return int(_ldap_get_field(username, 'housingPoints')) | ||
| def _ldap_get_group_members(group): |
There was a problem hiding this comment.
Although it's a bit late to bring up architecture concerns, why not have a member object that wraps the csh_ldap object that has these functions as methods, instead of having these util functions that take the csh_ldap object as an argument?
conditional/util/ldap.py
Outdated
| def ldap_get_active_members(): | ||
| return [x for x in ldap_get_current_students() | ||
| if ldap_is_active(x['uid'][0].decode('utf-8'))] | ||
| def _ldap_is_member_of_committee(account, directorship): |
There was a problem hiding this comment.
committee == directorship, although this isn't the only occurrence. Not sure if it's worth fixing.
conditional/util/ldap.py
Outdated
| return [x for x in ldap_get_current_students() | ||
| if ldap_is_intromember(x['uid'][0].decode('utf-8'))] | ||
| def ldap_get_member(username): | ||
| print("Got Member: {}".format(username)) |
There was a problem hiding this comment.
Remove print statement
There was a problem hiding this comment.
I did... I just didn't save in Cloud9 before committing. 🙄
mbillow
force-pushed
the
new-ldap
branch
3 times, most recently
from
92055da to
aa2fa4d
Compare
|
Awaiting merge and integration of https://github.com/liam-middlebrook/csh_ldap/pull/2. |
|
Just pushed to PyPi. https://pypi.python.org/pypi/csh_ldap |
stevenmirabito
left a comment
stevenmirabito
left a comment
There was a problem hiding this comment.
Regression on housing queue. All other issues resolved.
| def get_queue_length(): | ||
| return len(get_housing_queue()) | ||
| # TODO: Actually implement queue length. | ||
| return 20 |
There was a problem hiding this comment.
Merging this will break the housing queue. This constitutes regression on a major feature and I'm not comfortable doing that. The UI for #103 can be done separately, but the business logic should be here instead of just commenting it out.
| except (IndexError, ValueError): | ||
| return "0" | ||
| # TODO: Actually implement queue position. | ||
| return len(username) |
|
Approving this with the understanding that |
Solves #89
pyldap => csh_ldap