Use LDAP API to fetch executive members' name (Closes #376) #396

Merged
b72zhou merged 42 commits from b72zhou-ldap-exec into main 7 months ago
Collaborator
There is no content yet.
b72zhou added 3 commits 8 months ago
b06189f5f3 Test Ldap
6e8f7bba94 Test using console log
fbcdec33eb Change returned data type
b72zhou changed title from WIP: Use LDAP API to fetch executive members' name to WIP: Use LDAP API to fetch executive members' name (Closes #376) 8 months ago
b72zhou added 1 commit 8 months ago
8f144983ef Test Ldap
b72zhou added 1 commit 8 months ago
79c5005652 add placeholder
b72zhou requested review from j285he 8 months ago
b72zhou requested review from n3parikh 8 months ago
b72zhou requested review from a258wang 8 months ago
b72zhou added 1 commit 8 months ago
cb17e15b22 Merge branch 'main' into b72zhou-ldap-exec
b72zhou changed title from WIP: Use LDAP API to fetch executive members' name (Closes #376) to Use LDAP API to fetch executive members' name (Closes #376) 8 months ago
b72zhou added 2 commits 8 months ago
b72zhou added 1 commit 8 months ago
608f6342da Debug placeholder
b72zhou added 1 commit 8 months ago
6ad38d1971 Test getExec function
b72zhou changed title from Use LDAP API to fetch executive members' name (Closes #376) to WIP: Use LDAP API to fetch executive members' name (Closes #376) 8 months ago
b72zhou added 1 commit 8 months ago
c7947d9054 Test new getExecNames
b72zhou added 1 commit 8 months ago
ae0b81c4cc debug getExecNames
b72zhou added 1 commit 8 months ago
3a7ec04f4e Debug ldap data fetch function
b72zhou added 1 commit 8 months ago
c729bf40ee Fix name order
b72zhou added 1 commit 8 months ago
2aeaeaccde Fix Line Format
b72zhou added 2 commits 8 months ago
b72zhou added 1 commit 8 months ago
ae66794a54 add codey
b72zhou added 1 commit 8 months ago
57f67bc8d1 edit codey file name
b72zhou added 1 commit 8 months ago
340dfa67b5 Add placeholder
b72zhou added 1 commit 8 months ago
fe3358d9ae Test position field
b72zhou added 1 commit 8 months ago
f96d776259 Test position field 2
b72zhou added 1 commit 8 months ago
9a1ecf82bd edit president to presidentcro
b72zhou added 1 commit 8 months ago
6dc69fbe14 get position test
b72zhou added 1 commit 8 months ago
615a8ddbf9 test filter function
b72zhou added 1 commit 8 months ago
3506b6d00c test sort function
b72zhou added 1 commit 8 months ago
ca1a9f3185 debug sort function
b72zhou added 1 commit 8 months ago
bb743f4ca6 test president
b72zhou added 1 commit 8 months ago
f09580151f test position array
b72zhou added 1 commit 8 months ago
3dbc1efe39 test data type for position
b72zhou added 1 commit 8 months ago
0c0515a3c6 fetch array at index 0 from ldap
b72zhou added 1 commit 8 months ago
856f849526 finalize ldap
b72zhou changed title from WIP: Use LDAP API to fetch executive members' name (Closes #376) to Use LDAP API to fetch executive members' name (Closes #376) 8 months ago
b72zhou added 1 commit 7 months ago
83b68a3ce3 Merge branch 'main' into b72zhou-ldap-exec
Poster
Collaborator

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").

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").
n3parikh requested changes 7 months ago
n3parikh left a comment
Owner

I have a few chnages I'd like, but great job overall!

I have a few chnages I'd like, but great job overall!
lib/team.ts Outdated
sysadmin: 5,
};
const positionNames: string[] = [
Owner

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.

{
	"president": "President",
    ...
}
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. ``` { "president": "President", ... } ```
n3parikh marked this conversation as resolved
lib/team.ts Outdated
"System Administrator",
];
export interface ExecMembers {
Owner

Should this be is ExecMember (singular), since it represents only one exec?

Should this be is `ExecMember` (singular), since it represents only one exec?
b72zhou marked this conversation as resolved
lib/team.ts Outdated
? ""
: typeof item.position === "string"
? item.position
: (item.position[0] as string),
Owner

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):

let positions = ["president", "vice-president", ...]
let execMembers = {}
// Write a comment here explaining that item.position may be an array
searchEntries.forEach item
	if item.position is string and item.position in positions:
    	execMembers[item.position] = { construct ExecMember object }
    else if item.position is array:
    	item.position.forEach p
        	if p in positions:
            	execMembers[p] = { construct ExecMember object }

execMembers = positions.map((position) => execMembers[position]);

You can get rid of the sort afterwards and also the execPositions object, since the positions array will be in the correct order.

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): ``` let positions = ["president", "vice-president", ...] let execMembers = {} // Write a comment here explaining that item.position may be an array searchEntries.forEach item if item.position is string and item.position in positions: execMembers[item.position] = { construct ExecMember object } else if item.position is array: item.position.forEach p if p in positions: execMembers[p] = { construct ExecMember object } execMembers = positions.map((position) => execMembers[position]); ``` You can get rid of the sort afterwards and also the `execPositions` object, since the positions array will be in the correct order.
b72zhou added 1 commit 7 months ago
2d9176256d Fix logic for fetching array
b72zhou added 1 commit 7 months ago
51bb491277 fix undefined bug
Poster
Collaborator

The logic has been fixed as suggested above. Could you please confirm it is what we need? @n3parikh Thank you so much :))

The logic has been fixed as suggested above. Could you please confirm it is what we need? @n3parikh Thank you so much :))
n3parikh reviewed 7 months ago
n3parikh left a comment
Owner

One more thing (hopefully last round of reviews!)

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") {
Owner

Why are you checking if it is an object, rather than an array?

Why are you checking if it is an object, rather than an array?
Poster
Collaborator

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. :)

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. :)
b72zhou marked this conversation as resolved
lib/team.ts Outdated
const lastName =
fileName.split("-")[2][0].toUpperCase() + fileName.split("-")[2].slice(1);
const posOrder = fileName.split("-")[0][1];
Owner

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 to getExec. Then on line 111, you can index by the second entry in the pair, instead of positionNames[Number(posOrder) - 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 to `getExec`. Then on line 111, you can index by the second entry in the pair, instead of `positionNames[Number(posOrder) - 1]`.
Poster
Collaborator

Great idea I think! I've commited the code that implement this and we can discuss it tmr :))

Great idea I think! I've commited the code that implement this and we can discuss it tmr :))
b72zhou added 1 commit 7 months ago
4c087b4af4 eliminate position name to keep in sync
b72zhou added 1 commit 7 months ago
67afba862a fix type bug
b72zhou added 1 commit 7 months ago
ccd075fa84 resolve conflicts with main
b72zhou added 1 commit 7 months ago
00caa70143 fix data type
b72zhou added 1 commit 7 months ago
cafdf334fe final fix
n3parikh approved these changes 7 months ago
n3parikh left a comment
Owner

@j285he please take a look as well!

@j285he please take a look as well!
Poster
Collaborator

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/

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/
Collaborator

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

> 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
j285he reviewed 7 months ago
lib/team.ts Outdated
import { getCurrentTerm } from "@/lib/events";
const EXECS_PATH = path.join("content", "team", "execs");
const fileType = ".md";
Collaborator

Can we consider changing this to FILETYPE since this constant is not supposed to change?

Can we consider changing this to `FILETYPE` since this constant is not supposed to change?
b72zhou marked this conversation as resolved
lib/team.ts Outdated
};
const positionNames: string[] = [
"president",
Collaborator

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.

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.
b72zhou marked this conversation as resolved
lib/team.ts Outdated
const client = new Client({ url });
// position: name
const execMembers: { [name: string]: string } = {};
Collaborator

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

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
b72zhou marked this conversation as resolved
lib/team.ts Outdated
const firstName =
name.split("-")[0][0].toUpperCase() + name.split("-")[0].slice(1);
const lastName =
name.split("-")[1][0].toUpperCase() + name.split("-")[1].slice(1);
Collaborator

Please leverage the capitalize function in utils.ts

Please leverage the capitalize function in `utils.ts`
Collaborator

Looks good to me! I think I just had some stylistic nitpicks. Let me know if you disagree

Looks good to me! I think I just had some stylistic nitpicks. Let me know if you disagree
Poster
Collaborator

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

Just found that there were only 3 onstaging branches left. I have no idea what happened to onstaging.

> > 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 Just found that there were only 3 onstaging branches left. I have no idea what happened to onstaging.
Owner

@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.

@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.
b72zhou added 1 commit 7 months ago
caf0613118 improve code style and readability
b72zhou added 1 commit 7 months ago
6d2d3cea3f Merge branch 'main' into b72zhou-ldap-exec
b72zhou merged commit bb073136b0 into main 7 months ago

Reviewers

j285he was requested for review 8 months ago
a258wang was requested for review 8 months ago
n3parikh approved these changes 7 months ago
continuous-integration/drone/push Build is passing
The pull request has been merged as bb073136b0.
Sign in to join this conversation.
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…
There is no content yet.