Use LDAP API to fetch executive members' name (Closes #376) #396
Merged
b72zhou
merged 42 commits from b72zhou-ldap-exec into main 11 months ago
j285he
a258wang
n3parikh
Loading…
Reference in new issue
There is no content yet.
Delete Branch 'b72zhou-ldap-exec'
Deleting a branch is permanent. It CANNOT be undone. Continue?
WIP: Use LDAP API to fetch executive members' nameto WIP: Use LDAP API to fetch executive members' name (Closes #376) 1 year agoWIP: Use LDAP API to fetch executive members' name (Closes #376)to Use LDAP API to fetch executive members' name (Closes #376) 12 months agoUse LDAP API to fetch executive members' name (Closes #376)to WIP: Use LDAP API to fetch executive members' name (Closes #376) 12 months agoWIP: Use LDAP API to fetch executive members' name (Closes #376)to Use LDAP API to fetch executive members' name (Closes #376) 12 months agohttps://csclub.uwaterloo.ca/~a3thakra/csc/b72zhou-ldap-exec/about/team/
This is how it looks like now. The executive names are fetched by LDAP. After getting the names and positions successfully. It will look for the .md file of the exec to get their blurb. If the file is not found, the exec name and position would still be there, only the blurb becomes "coming soon!". (placeholder has been tested and worked as described here)
Note: Since only this term our president has two positions (i.e. "cro" and "president"), her position is stored as a string array in the database, but others' execs positions are just strings. To make sure it can function well each term, I only fetch the first element if the data is an array(i.e. "president").
I have a few chnages I'd like, but great job overall!
sysadmin: 5,};const positionNames: string[] = [Turn this into a object that maps from the LDAP short name (ex.
sysadmin) and then index by that instead. We shouldn't index by the position order, since that could change.ie.
"System Administrator",];export interface ExecMembers {Should this be is
ExecMember(singular), since it represents only one exec?? "": typeof item.position === "string"? item.position: (item.position[0] as string),We shouldn't rely on the position we want to be first. Also, this is unlikely, but it is possible that one person has mutiple postions we care about.
Here is what I propose instead (in psuedocode):
You can get rid of the sort afterwards and also the
execPositionsobject, since the positions array will be in the correct order.The logic has been fixed as suggested above. Could you please confirm it is what we need? @n3parikh Thank you so much :))
One more thing (hopefully last round of reviews!)
searchEntries.forEach((item) => {if (typeof item.position === "string" && item.position in execPositions) {execMembers[item.position] = item.cn as string;} else if (typeof item.position === "object") {Why are you checking if it is an object, rather than an array?
Whoops, that was because when I use typeof to get the type of an array, it returned "object". But now I found that instanceof would return "array" type direcly. Let me change that. :)
const lastName =fileName.split("-")[2][0].toUpperCase() + fileName.split("-")[2].slice(1);const posOrder = fileName.split("-")[0][1];I think it's not ideal that we use a position number, since it means another thing we need to keep in sync.
What do you think of returning a list of file-name, position pairs in
getExecNames, and then dropping the number from the file names? Ie. instead of["01-juthika-hoque.md", "02-eric-huang.md", ...], we return[["juthika-hoque.md", "president"], ...]., and then pass that pair togetExec. Then on line 111, you can index by the second entry in the pair, instead ofpositionNames[Number(posOrder) - 1].Great idea I think! I've commited the code that implement this and we can discuss it tmr :))
@j285he please take a look as well!
The test for placeholder also goes well. The following link shows how it looks like when there is no markdown file for the exec.
https://csclub.uwaterloo.ca/~a3thakra/csc/ldap-exec-test/about/team/
I get 404
import { getCurrentTerm } from "@/lib/events";const EXECS_PATH = path.join("content", "team", "execs");const fileType = ".md";Can we consider changing this to
FILETYPEsince this constant is not supposed to change?};const positionNames: string[] = ["president",Could we call this array orderedExecPositions or something? I was a little confused by why we needed this array at first until I realized that we use it to enforce the order of execs.
const client = new Client({ url });// position: nameconst execMembers: { [name: string]: string } = {};If I'm understanding the code correctly, could we change the type to
{ [position: string]: string }so that it's inline with the documentation above? If you like this change, also change line 14const firstName =name.split("-")[0][0].toUpperCase() + name.split("-")[0].slice(1);const lastName =name.split("-")[1][0].toUpperCase() + name.split("-")[1].slice(1);Please leverage the capitalize function in
utils.tsLooks good to me! I think I just had some stylistic nitpicks. Let me know if you disagree
Just found that there were only 3 onstaging branches left. I have no idea what happened to onstaging.
@b72zhou Aditya was cleaning up the staging directory and deleted all the branches that didn't have a PR with them. You can push an empty commit to the branch and it'll be regenerated.
bb073136b0into main 11 months agoReviewers
bb073136b0.