Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Load user personnel data into portal #1088

Merged
merged 19 commits into from
Sep 6, 2023

Conversation

abbyoung
Copy link
Contributor

@abbyoung abbyoung commented Aug 28, 2023

SC-2264

To Do

  • Add env var to docker/gh as needed
  • Improve error handling

Proposed changes

  • Adds Personnel API data source to GraphQL server
  • Adds resolver to GraphQL server to return personnel data from Personnel API
  • Queries personnel data in authContext and stores in user of type SessionUser
  • Updates unit tests for authContext

Reviewer notes

A number of test files got auto-updated with prettier -- whoops! But also, yay? Sorry for the clutter -- the changes have no effect or bearing on the story. Just lots of added commas.

To test loading the personnel data locally, make sure you are running the ussf-personnel-api locally and have pulled down the latest changes on main. You can then console.log out the user (SessionUser) fetched by useAuthContext and see the personnelData object added to the user.

For example, in ThemeToggle.tsx --

[line 25] const { user: sessionUser, portalUser } = useUser()
[line 26] console.log(sessionUser)

Expected output in console when loading home page:

{
    "userId": "ETHEL.NEAL.643097412@testusers.cce.af.mil",
    "issuer": "http://localhost:8080/simplesaml/saml2/idp/metadata.php",
     ...
    },
    "personnelData": {
        "First_name": "ETHEL",
        "Last_Name": "NEAL",
        "DOD_ID": "643097412",
        "Grade": "2",
        "MAJCOM": "SPACE SYSTEMS COMMAND (6S)",
        "DUTYTITLE": "SPACE COMBAT",
        "Country": "COLORADO",
        "BASE_LOC": "SCHRIEVER",
        "Org_type": null,
        "EOPDate": "1716595200000",
        "userType": "Officer",
        "lastModifiedAt": "2023-08-28T16:01:35.065-07:00"
    }
}

Setup

Start the system

yarn services:up
yarn dev
cd ../ussf-portal-cms
yarn dev

Login to the portal http://localhost:3000

Start storybook

yarn storybook

Login to storybook http://localhost:6006, though the command above should open it for you


Code review steps

As the original developer, I have

  • Met the acceptance criteria
  • Created new stories in Storybook if applicable
  • Created/modified automated unit tests in Jest
  • Created/modified automated E2E tests
  • Followed guidelines for zero-downtime deploys, if applicable
  • Use ANDI to check for basic a11y issues

As a reviewer, I have

Check out our How to review a pull request document.


Screenshots

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #2264: Load portal user's personnel data.

portalUser: null,
loading: true
portalUser: null,
loading: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what is happening with all of these formatted commas in files I did not even touch 🤨

@@ -41,8 +47,10 @@ export const ObjectIdScalar = new GraphQLScalarType({

type PortalUserContext = {
db: typeof MongoClient
user: PortalUser
keystoneUrl: string
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

realized that since we use data sources now, we do not need to access the keystone URL in the resolvers 🎉

@@ -110,7 +112,8 @@ export const apolloServer = new ApolloServer({
}

// Set db connection, keystone api, and user in context
return { db, keystoneUrl, user: loggedInUser }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

realized that since we use data sources now, we do not need to access the keystone URL in the resolvers 🎉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

loading: true,
}
})
jest.spyOn(useUserHooks, 'useUser').mockImplementation(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more formatting weirdness in test files 🥲

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So many, I wonder how all these were missed? Maybe we got a new version of prettier that does a better job?

@abbyoung abbyoung marked this pull request as ready for review August 28, 2023 23:03
@abbyoung abbyoung requested a review from a team as a code owner August 28, 2023 23:03
Copy link
Contributor

@gidjin gidjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a console log in ThemeToggle but the personnelData seems to be empty even though the personnel api is up and manually querying it returns data. Not sure what I've missed maybe we can pair next week. Also left a note about the console.logs.

.eslintrc.json Outdated
@@ -51,6 +51,7 @@
"groupCollapsed",
"groupEnd",
"info",
"log",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: I would like to avoid log if we can. Since the lint recommendation is to avoid it and something like error or warn or debug is more clear about the purpose. Can you remove this and change the one's you added to console.debug or remove them if applicable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't worry -- this is just for debugging in gh actions, since i can't replicate the issue locally. will remove once the PR is ready (again) :)

loading: true,
}
})
jest.spyOn(useUserHooks, 'useUser').mockImplementation(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So many, I wonder how all these were missed? Maybe we got a new version of prettier that does a better job?

@@ -47,7 +47,7 @@ const UserModel = {
WIDGETS.FEATUREDSHORTCUTS,
WIDGETS.GUARDIANIDEAL,
]

console.log('New user to Create 🔥 in MongoDB: ', newUser)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd change this to info or debug unless we don't need it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are just my debugging logs, will for sure change or remove :)

@@ -95,7 +97,8 @@ export const apolloServer = new ApolloServer({

// Check if user exists. If not, create new user
const foundUser = await User.findOne(userId, { db })

console.log('💚foundUser', foundUser)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here for the console log, info, debug, or remove.

@@ -110,7 +112,8 @@ export const apolloServer = new ApolloServer({
}

// Set db connection, keystone api, and user in context
return { db, keystoneUrl, user: loggedInUser }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

}
} else {
// This (probably) means they aren't logged in
console.log('🦄Test: No user in response, redirecting to login')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think leaving this as info would be fine, but we could switch it to debug

@@ -17,6 +17,7 @@ const requireVars = [
'SESSION_DOMAIN',
'KEYSTONE_URL',
'LAUNCHDARKLY_SDK_CLIENT_SIDE_ID',
'PERSONNEL_API_URL',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought: Should we add this to the sysinfo handler in src/pages/api/sysinfo.ts? I think it might be helpful since we already have keystoneUrl there too.

@abbyoung abbyoung changed the title feat: Load user personnel data into portal feat: Load user personnel data into portal [wip] Sep 1, 2023
@abbyoung abbyoung changed the title feat: Load user personnel data into portal [wip] feat: Load user personnel data into portal Sep 4, 2023
@jcbcapps jcbcapps merged commit fe0a1d8 into main Sep 6, 2023
@jcbcapps jcbcapps deleted the sc-2264-load-user-personnel-data branch September 6, 2023 16:41
gidjin pushed a commit that referenced this pull request Sep 11, 2023
## [4.23.0](4.22.1...4.23.0) (2023-09-11)


### Features

* Add resolvers and mutations for managing weather widget ([#1075](#1075)) ([a652766](a652766))
* Add support for CTA to link a document ([#1082](#1082)) ([fa0a6bb](fa0a6bb))
* Change h3 from book weight to bold weight ([#1076](#1076)) ([c92ae65](c92ae65))
* Create weather widget ([#1079](#1079)) ([3e6b91d](3e6b91d))
* Load user personnel data into portal ([#1088](#1088)) ([fe0a1d8](fe0a1d8))
* new loader animation ([#1085](#1085)) ([b099904](b099904))


### Bug Fixes

* **deps:** update dependencies ([#1059](#1059)) ([441a35c](441a35c))
* design: dark mode adjustments ([#1091](#1091)) ([9c67a4a](9c67a4a))
* design: Footer styling refactor for accessibility and responsive design improvements ([#1081](#1081)) ([18dd17f](18dd17f))
* local personnel-api service targets builder stage only ([#1084](#1084)) ([94950eb](94950eb))
* storybook dark theme ([#1065](#1065)) ([c5deda3](c5deda3))
* storybook deploy ([#1072](#1072)) ([ee21044](ee21044))


### Security Improvements

* **deps:** update auto-instrumentations-web and auto-instrumentations-node ([#1087](#1087)) ([71236b8](71236b8))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants