Use LDAP API to fetch executive members' name (Closes #376) #396
Labels
No Label
a11y
Backlog
Blocked
Bug
Content
Dependencies
Design
Feature Request
Good First Issue
In Progress
Performance
Priority - High
Priority - Low
Priority - Medium
Untriaged
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: www/www-new#396
Loading…
Reference in New Issue
No description provided.
Delete Branch "b72zhou-ldap-exec"
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?
WIP: Use LDAP API to fetch executive members' nameto WIP: Use LDAP API to fetch executive members' name (Closes #376)WIP: Use LDAP API to fetch executive members' name (Closes #376)to Use LDAP API to fetch executive members' name (Closes #376)Use LDAP API to fetch executive members' name (Closes #376)to WIP: Use LDAP API to fetch executive members' name (Closes #376)WIP: Use LDAP API to fetch executive members' name (Closes #376)to Use LDAP API to fetch executive members' name (Closes #376)https://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!
@ -9,0 +19,4 @@
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.
@ -9,0 +27,4 @@
"System Administrator",
];
export interface ExecMembers {
Should this be is
ExecMember
(singular), since it represents only one exec?@ -20,0 +71,4 @@
? ""
: 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
execPositions
object, 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!)
@ -20,0 +62,4 @@
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. :)
@ -33,0 +107,4 @@
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
@ -7,2 +8,4 @@
import { getCurrentTerm } from "@/lib/events";
const EXECS_PATH = path.join("content", "team", "execs");
const fileType = ".md";
Can we consider changing this to
FILETYPE
since this constant is not supposed to change?@ -9,0 +20,4 @@
};
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.
@ -20,0 +43,4 @@
const client = new Client({ url });
// position: name
const 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 14@ -33,0 +106,4 @@
const 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.ts
Looks 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.