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

Merged
b72zhou merged 42 commits from b72zhou-ldap-exec into main 2022-03-16 02:17:34 -04:00
Contributor
No description provided.
b72zhou added 3 commits 2022-02-02 04:08:34 -05:00
continuous-integration/drone/push Build is failing Details
b06189f5f3
Test Ldap
continuous-integration/drone/push Build is failing Details
6e8f7bba94
Test using console log
continuous-integration/drone/push Build is failing Details
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) 2022-02-02 04:08:51 -05:00
b72zhou added 1 commit 2022-02-02 08:48:34 -05:00
continuous-integration/drone/push Build is failing Details
8f144983ef
Test Ldap
b72zhou added 1 commit 2022-02-09 08:22:53 -05:00
continuous-integration/drone/push Build is passing Details
79c5005652
add placeholder
b72zhou requested review from j285he 2022-02-09 08:23:21 -05:00
b72zhou requested review from n3parikh 2022-02-09 08:23:21 -05:00
b72zhou requested review from a258wang 2022-02-09 08:23:21 -05:00
b72zhou added 1 commit 2022-02-09 08:25:10 -05:00
continuous-integration/drone/push Build is passing Details
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) 2022-02-09 08:25:45 -05:00
b72zhou added 2 commits 2022-02-09 08:31:26 -05:00
b72zhou added 1 commit 2022-02-09 11:45:10 -05:00
continuous-integration/drone/push Build is passing Details
608f6342da
Debug placeholder
b72zhou added 1 commit 2022-02-09 12:01:55 -05:00
continuous-integration/drone/push Build is failing Details
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) 2022-02-09 12:02:09 -05:00
b72zhou added 1 commit 2022-02-09 12:22:21 -05:00
continuous-integration/drone/push Build is passing Details
c7947d9054
Test new getExecNames
b72zhou added 1 commit 2022-02-09 12:35:35 -05:00
continuous-integration/drone/push Build is failing Details
ae0b81c4cc
debug getExecNames
b72zhou added 1 commit 2022-02-13 10:48:43 -05:00
continuous-integration/drone/push Build is failing Details
3a7ec04f4e
Debug ldap data fetch function
b72zhou added 1 commit 2022-02-13 11:22:39 -05:00
continuous-integration/drone/push Build is failing Details
c729bf40ee
Fix name order
b72zhou added 1 commit 2022-02-13 11:32:13 -05:00
continuous-integration/drone/push Build is failing Details
2aeaeaccde
Fix Line Format
b72zhou added 2 commits 2022-02-13 20:33:12 -05:00
b72zhou added 1 commit 2022-02-13 21:00:07 -05:00
continuous-integration/drone/push Build is failing Details
ae66794a54
add codey
b72zhou added 1 commit 2022-02-13 21:09:20 -05:00
continuous-integration/drone/push Build is passing Details
57f67bc8d1
edit codey file name
b72zhou added 1 commit 2022-02-13 22:45:38 -05:00
continuous-integration/drone/push Build is passing Details
340dfa67b5
Add placeholder
b72zhou added 1 commit 2022-02-16 06:35:32 -05:00
continuous-integration/drone/push Build is passing Details
fe3358d9ae
Test position field
b72zhou added 1 commit 2022-02-16 07:01:59 -05:00
continuous-integration/drone/push Build is passing Details
f96d776259
Test position field 2
b72zhou added 1 commit 2022-02-16 07:15:47 -05:00
continuous-integration/drone/push Build is passing Details
9a1ecf82bd
edit president to presidentcro
b72zhou added 1 commit 2022-02-16 07:29:49 -05:00
continuous-integration/drone/push Build is passing Details
6dc69fbe14
get position test
b72zhou added 1 commit 2022-02-16 07:51:08 -05:00
continuous-integration/drone/push Build is passing Details
615a8ddbf9
test filter function
b72zhou added 1 commit 2022-02-16 08:07:39 -05:00
continuous-integration/drone/push Build is passing Details
3506b6d00c
test sort function
b72zhou added 1 commit 2022-02-16 08:45:57 -05:00
continuous-integration/drone/push Build is passing Details
ca1a9f3185
debug sort function
b72zhou added 1 commit 2022-02-16 10:31:31 -05:00
continuous-integration/drone/push Build is failing Details
bb743f4ca6
test president
b72zhou added 1 commit 2022-02-16 10:43:20 -05:00
continuous-integration/drone/push Build is passing Details
f09580151f
test position array
b72zhou added 1 commit 2022-02-16 11:06:25 -05:00
continuous-integration/drone/push Build is passing Details
3dbc1efe39
test data type for position
b72zhou added 1 commit 2022-02-16 21:58:57 -05:00
continuous-integration/drone/push Build is passing Details
0c0515a3c6
fetch array at index 0 from ldap
b72zhou added 1 commit 2022-02-16 22:42:18 -05:00
continuous-integration/drone/push Build is passing Details
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) 2022-02-16 22:53:12 -05:00
b72zhou added 1 commit 2022-02-27 09:19:34 -05:00
continuous-integration/drone/push Build is passing Details
83b68a3ce3
Merge branch 'main' into b72zhou-ldap-exec
Author
Contributor

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 2022-03-02 15:17:33 -05:00
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
@ -9,0 +19,4 @@
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
@ -9,0 +27,4 @@
"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
@ -20,0 +71,4 @@
? ""
: 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 2022-03-04 03:15:28 -05:00
continuous-integration/drone/push Build is failing Details
2d9176256d
Fix logic for fetching array
b72zhou added 1 commit 2022-03-04 04:09:05 -05:00
continuous-integration/drone/push Build is passing Details
51bb491277
fix undefined bug
Author
Contributor

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 2022-03-09 02:09:20 -05:00
n3parikh left a comment
Owner

One more thing (hopefully last round of reviews!)

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") {
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?
Author
Contributor

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
@ -33,0 +107,4 @@
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]`.
Author
Contributor

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 2022-03-09 11:39:03 -05:00
continuous-integration/drone/push Build is failing Details
4c087b4af4
eliminate position name to keep in sync
b72zhou added 1 commit 2022-03-09 11:52:18 -05:00
continuous-integration/drone/push Build is passing Details
67afba862a
fix type bug
b72zhou added 1 commit 2022-03-09 12:10:02 -05:00
continuous-integration/drone/push Build is passing Details
ccd075fa84
resolve conflicts with main
b72zhou added 1 commit 2022-03-09 21:32:06 -05:00
continuous-integration/drone/push Build is passing Details
00caa70143
fix data type
b72zhou added 1 commit 2022-03-09 21:43:01 -05:00
continuous-integration/drone/push Build is passing Details
cafdf334fe
final fix
n3parikh approved these changes 2022-03-09 21:44:39 -05:00
n3parikh left a comment
Owner

@j285he please take a look as well!

@j285he please take a look as well!
Author
Contributor

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

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 2022-03-11 01:58:57 -05:00
lib/team.ts Outdated
@ -7,2 +8,4 @@
import { getCurrentTerm } from "@/lib/events";
const EXECS_PATH = path.join("content", "team", "execs");
const fileType = ".md";
Member

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
@ -9,0 +20,4 @@
};
const positionNames: string[] = [
"president",
Member

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
@ -20,0 +43,4 @@
const client = new Client({ url });
// position: name
const execMembers: { [name: string]: string } = {};
Member

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
@ -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);
Member

Please leverage the capitalize function in utils.ts

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

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
Author
Contributor

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 2022-03-16 01:58:38 -04:00
continuous-integration/drone/push Build is failing Details
caf0613118
improve code style and readability
b72zhou added 1 commit 2022-03-16 01:59:08 -04:00
continuous-integration/drone/push Build is passing Details
6d2d3cea3f
Merge branch 'main' into b72zhou-ldap-exec
b72zhou merged commit bb073136b0 into main 2022-03-16 02:17:34 -04:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
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: www/www-new#396
No description provided.