[env/github] map SAML SSO data from Okta into user's profile data #1667

mike-lvov · 2021-06-25T21:59:00Z

closes https://github.com/sparkletown/internal-sparkle-issues/issues/767

Get data from Okta and apply it to the app.
Adds new user fields.

~~Will need to base it off env/github, once it's updated with the latest staging~~ This has been done now

src/pages/Account/Profile.tsx

codeclimate · 2021-06-25T21:59:42Z

Code Climate has analyzed commit 15dfaa4 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1
Duplication	2

View more on Code Climate.

github-actions · 2021-06-25T22:02:56Z

Visit the preview URL for this PR (updated for commit 15dfaa4):

https://co-reality-staging--preview-pr-1667-zq61mbfg.web.app

_{(expires Mon, 12 Jul 2021 00:11:41 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

src/hooks/useSAMLSignIn.ts

src/pages/Account/Profile.tsx

src/pages/VenuePage/VenuePage.tsx

src/types/User.ts

0xdevalias

A bunch of comments/changes/etc. I'll probably try and fix these up today to save time since I assume you're offline at the moment.

src/types/User.ts

src/utils/localStorage.ts

src/utils/profile.ts

src/components/molecules/ProfilePictureInput/ProfilePictureInput.tsx

Co-authored-by: Glenn 'devalias' Grant <glenn@devalias.net>

…sparkle into feature/apply-SAML-mappings

0xdevalias

Few more changes needed here.

src/components/molecules/ProfilePictureInput/ProfilePictureInput.tsx

0xdevalias · 2021-06-28T10:11:45Z

src/components/molecules/ProfilePictureInput/ProfilePictureInput.tsx

+  const getRenderedAvatarImageComponent = useCallback(
+    (avatarSrc: string, key: string | number) => (
      <div
-        key={`${avatar}-${index}`}
+        key={`${avatarSrc}-${key}`}
        className="profile-picture-preview-container"
-        onClick={() => uploadDefaultAvatar(avatar)}
+        onClick={() => uploadDefaultAvatar(avatarSrc)}
      >
        <img
-          src={avatar}
+          src={avatarSrc}
          className="profile-icon profile-picture-preview"
-          alt={`default avatar ${index}`}
+          alt={`default avatar ${key}`}
        />
      </div>
-    ));
-  }, [defaultAvatars, uploadDefaultAvatar]);
+    ),
+    [uploadDefaultAvatar]
+  );
+
+  const avatarImages = useMemo(() => {
+    const renderedAvatarImages = defaultAvatars.map(
+      getRenderedAvatarImageComponent
+    );


I still don't see why we would bother keeping getRenderedAvatarImageComponent instead of just inlining it within avatarImages.

defaultAvatars is a string[]

githubImageSrc is a string | undefined

So just do something like:

[githubAvatarImage, ...defaultAvatars].filter(isDefined).map((avatarSrc) => <div key={avatarSrc} // ..snip..

Done in 36f9ee2

Why did you use the index as part of the key?

@0xdevalias legacy logic

src/components/organisms/UserProfileModal/UserProfileModal.scss

0xdevalias · 2021-06-28T10:16:55Z

src/components/organisms/UserProfileModal/UserProfileModal.tsx

            return (
              <React.Fragment key={question.text}>
-                <p className="light question">{question.text}</p>
+                <p className="light no-margin">{question.text}</p>
                <h6>{questionAnswer}</h6>
              </React.Fragment>
            );


[OOS] But the usage of h6 here is SUPER janky. Header tags shouldn't be used for styling like this. This breaks all sorts of accessibility rules.

Actually.. bootstrap has classes that mirror the header styling. Can you change this to a div or something and use the h6 class at the very least? Should be a quick fix for it i'm guessing.

Why are they junky though? Note that this is for env/github only

Not sure which bootstrap you are referring to, but it looks like here the h1 tags are also used.
https://mdbootstrap.com/docs/standard/navigation/headers/

'Bootstrap' refers to https://getbootstrap.com/

https://getbootstrap.com/docs/4.0/content/typography/#headings

.h1 through .h6 classes are also available, for when you want to match the font styling of a heading but cannot use the associated HTML element.

Done in 41dc9b2

Out of curiousity, hoes does it effect the all sorts of accessibility rules?

Thanks :)

I explained this way back in the past. But basically headings have a very specific and defined semantic meaning in HTML structure, that is used by screen readers/etc.

This was from a quick google, there may be other/better resources:

https://www.w3.org/WAI/tutorials/page-structure/headings/

https://accessibility.psu.edu/headingshtml/

src/components/organisms/UserProfileModal/UserProfileModal.tsx

src/pages/Account/Profile.tsx

src/utils/profile.ts

Co-authored-by: Glenn 'devalias' Grant <glenn@devalias.net>

mike-lvov · 2021-06-28T11:06:58Z

Done the editing

src/components/organisms/ProfileModal/EditProfileForm/EditProfileForm.tsx

0xdevalias

I believe this comment still needs addressing: #1667 (comment)

But aside from that, I think this looks good to go.

Note: I've only code reviewed, not clickthrough/etc. But if you're happy with how it looks/functions, LGTM!

mike-lvov added 3 commits June 25, 2021 18:44

Adds mapping application to the profile

5247fa5

Adds functional prototype

9ab49a0

Adds comments + explanations

00d313a

mike-lvov added the 💡 new-feature New idea/feature request label Jun 25, 2021

mike-lvov requested a review from a team June 25, 2021 21:59

mike-lvov self-assigned this Jun 25, 2021

mike-lvov temporarily deployed to feature-preview June 25, 2021 21:59 Inactive

codeclimate bot reviewed Jun 25, 2021

View reviewed changes

src/pages/Account/Profile.tsx Outdated Show resolved Hide resolved

src/pages/Account/Profile.tsx Outdated Show resolved Hide resolved

0xdevalias reviewed Jun 25, 2021

View reviewed changes

src/hooks/useSAMLSignIn.ts Outdated Show resolved Hide resolved

0xdevalias reviewed Jun 25, 2021

View reviewed changes

src/pages/Account/Profile.tsx Outdated Show resolved Hide resolved

0xdevalias reviewed Jun 25, 2021

View reviewed changes

src/pages/VenuePage/VenuePage.tsx Outdated Show resolved Hide resolved

0xdevalias reviewed Jun 25, 2021

View reviewed changes

src/types/User.ts Outdated Show resolved Hide resolved

utils for localStorage

1835ebd

mike-lvov changed the title ~~Feature/apply saml mappings~~ apply saml mappings Jun 27, 2021

Adds SSO saml data integration

15dfaa4

mike-lvov temporarily deployed to feature-preview June 28, 2021 00:07 Inactive

github-actions bot added the 💎 styles For (S)CSS style related issues/changes/improvements/etc. label Jun 28, 2021

mike-lvov changed the base branch from staging to env/github June 28, 2021 00:11

mike-lvov changed the base branch from env/github to staging June 28, 2021 00:11

mike-lvov marked this pull request as ready for review June 28, 2021 00:12

mike-lvov changed the base branch from staging to env/github June 28, 2021 00:23

mike-lvov and others added 3 commits June 28, 2021 03:29

Merge branch 'env/github' into feature/apply-SAML-mappings

40c2047

Removes hardcoded text

471e08b

Merge branch 'env/github' into feature/apply-SAML-mappings

5b2e692

0xdevalias changed the title ~~apply saml mappings~~ [env/github] map SAML SSO data from Okta into user's profile data Jun 28, 2021

0xdevalias added P1 - Critical Priority 1 - Critical 🔒 security For issues/PRs related to security features (including addressing security vulnerabilities) labels Jun 28, 2021

0xdevalias suggested changes Jun 28, 2021

View reviewed changes

mike-lvov and others added 8 commits June 28, 2021 09:54

Update src/utils/profile.ts

9e85c8b

Co-authored-by: Glenn 'devalias' Grant <glenn@devalias.net>

Removes Mapping type; Replaces mapping fn

2d4f261

Refactors avatar images

8c56ac8

Replaces localStorage with firebase + useAsync

c77cd25

Merge branch 'env/github' into feature/apply-SAML-mappings

e3eee37

Fixes data display + Adds loading

3da671c

Merge branch 'feature/apply-SAML-mappings' of github.com:sparkletown/…

906213a

…sparkle into feature/apply-SAML-mappings

Removes comment

d4d5ad9

mike-lvov requested a review from 0xdevalias June 28, 2021 09:24

Fixes the incorrectly displayed value

124ffac

0xdevalias suggested changes Jun 28, 2021

View reviewed changes

mike-lvov and others added 3 commits June 28, 2021 13:41

Apply suggestions from code review

a27e97c

Co-authored-by: Glenn 'devalias' Grant <glenn@devalias.net>

Update src/components/organisms/UserProfileModal/UserProfileModal.tsx

c90d0f1

Co-authored-by: Glenn 'devalias' Grant <glenn@devalias.net>

Apply review suggestions

36f9ee2

mike-lvov added 3 commits June 28, 2021 14:08

Adds editing functionality

fc47408

Changes the icon to GitHub

28aeb73

Final polishes

4ce9fa3

mike-lvov requested a review from 0xdevalias June 28, 2021 11:25

0xdevalias reviewed Jun 28, 2021

View reviewed changes

src/components/organisms/ProfileModal/EditProfileForm/EditProfileForm.tsx Outdated Show resolved Hide resolved

mike-lvov added 2 commits June 28, 2021 14:36

Wrapps inputs into label

b1a4961

Replaces h6 with p

41dc9b2

0xdevalias approved these changes Jun 28, 2021

View reviewed changes

Returns a fn

b294918

mike-lvov enabled auto-merge June 28, 2021 12:01

mike-lvov disabled auto-merge June 28, 2021 12:01

mike-lvov merged commit 534bbd1 into env/github Jun 28, 2021

mike-lvov deleted the feature/apply-SAML-mappings branch June 28, 2021 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[env/github] map SAML SSO data from Okta into user's profile data #1667

[env/github] map SAML SSO data from Okta into user's profile data #1667

mike-lvov commented Jun 25, 2021 •

edited by 0xdevalias

codeclimate bot commented Jun 25, 2021 •

edited

github-actions bot commented Jun 25, 2021 •

edited

0xdevalias left a comment

0xdevalias left a comment

0xdevalias Jun 28, 2021

mike-lvov Jun 28, 2021

0xdevalias Jun 28, 2021

mike-lvov Jun 28, 2021

0xdevalias Jun 28, 2021

0xdevalias Jun 28, 2021

mike-lvov Jun 28, 2021

mike-lvov Jun 28, 2021

0xdevalias Jun 28, 2021

mike-lvov Jun 28, 2021

0xdevalias Jun 28, 2021 •

edited

mike-lvov commented Jun 28, 2021

0xdevalias left a comment

[env/github] map SAML SSO data from Okta into user's profile data #1667

[env/github] map SAML SSO data from Okta into user's profile data #1667

Conversation

mike-lvov commented Jun 25, 2021 • edited by 0xdevalias

codeclimate bot commented Jun 25, 2021 • edited

github-actions bot commented Jun 25, 2021 • edited

0xdevalias left a comment

Choose a reason for hiding this comment

0xdevalias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xdevalias Jun 28, 2021 • edited

Choose a reason for hiding this comment

mike-lvov commented Jun 28, 2021

0xdevalias left a comment

Choose a reason for hiding this comment

mike-lvov commented Jun 25, 2021 •

edited by 0xdevalias

codeclimate bot commented Jun 25, 2021 •

edited

github-actions bot commented Jun 25, 2021 •

edited

0xdevalias Jun 28, 2021 •

edited