-
Notifications
You must be signed in to change notification settings - Fork 290
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: oauth landing page [SQSERVICES-1775] #14793
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #14793 +/- ##
==========================================
- Coverage 42.94% 42.88% -0.06%
==========================================
Files 626 630 +4
Lines 21320 21439 +119
Branches 4897 4920 +23
==========================================
+ Hits 9155 9194 +39
- Misses 10998 11078 +80
Partials 1167 1167 |
04c2dd2
to
0f4fc94
Compare
a4b3183
to
de0172c
Compare
21b0748
to
fe862aa
Compare
const mockedActions = { | ||
clientAction: { | ||
generateClientPayload: spies.generateClientPayload, | ||
generateClientPayload: jasmine.createSpy().and.returnValue({}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use jest
instead of jasmine
if possible: jest.fn().mockReturnValue({})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense
return async (dispatch, getState, {apiClient}) => { | ||
dispatch(AuthActionCreator.startFetchTeam()); | ||
try { | ||
const teamData = await apiClient.api.teams.team.getTeam(teamId ?? getState().selfState.self.team ?? ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty string as teamId
doesn't make too much sense to me. Should teamId
be a required prop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea
const oauthParams = decodeURIComponent(window.location.search.slice(1)) | ||
.split('&') | ||
.reduce((acc, param) => { | ||
const [key, value] = param.split('='); | ||
if (key === 'scope') { | ||
return {...acc, [key]: value.replaceAll('+', ' ')}; | ||
} | ||
return {...acc, [key]: value}; | ||
}, {} as OAuthBody); | ||
const [oAuthApp, setOAuthApp] = useState<OAuthClient | null>(null); | ||
const oAuthScope = oauthParams.scope | ||
.split(/\+|%20|\s/) | ||
.filter(scope => Object.values(Scope).includes(scope as Scope)) as Scope[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about moving the logic of these two oauthParams
and oAuthScope
to some kind of OAuthUtil
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
> | ||
{chunks} | ||
</a> | ||
)) as any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This as any
is not needed, TS seems to be happy without it 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same thing with this eslint ignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
if (event.key === KEY.ENTER) { | ||
onContinue(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can use handleKeyDown
from KeyboardUtil
here (unless we dont want it to react on space key also)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
ccf4387
to
0dce794
Compare
0dce794
to
a8b48c4
Compare
@@ -264,8 +264,7 @@ const AccountFormComponent = ({account, ...props}: Props & ConnectedProps & Disp | |||
<FormattedMessage | |||
{...accountFormStrings.termsAndPrivacyPolicy} | |||
values={{ | |||
// eslint-disable-next-line react/display-name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was this one for 🤔 ?
Good that we could get rid of it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no clue, i guess our linter changed a while ago. maybe we turned off this rule where components need to be named?
@@ -78,7 +78,7 @@ export class ClientAction { | |||
}; | |||
}; | |||
|
|||
generateClientPayload = (clientType: ClientType): ClientInfo | undefined => { | |||
private generateClientPayload = (clientType: ClientType): ClientInfo | undefined => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 🙏
style={{ | ||
fontSize: '12px', | ||
lineHeight: '16px', | ||
display: 'block', | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's extract all those styles to a OAuthPermissions.styles.ts
and put it to the css={}
attributes (not style
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/script/auth/util/oauthUtil.ts
Outdated
*/ | ||
export const oAuthParams = (location: Location) => | ||
decodeURIComponent(location.search.slice(1)) | ||
.split('&') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we use URLSearchParam instead of doing the parsing ourselves?
https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tried this as well but it didn't seem to work very well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont recall the specifics but ill test it again to see what the issue is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe i found it less than ideal because it became a special object that I would have to then turn into a regular object. Anyway I have implemented that solution now.
the internet is split on the best way to handle this: https://stackoverflow.com/questions/8648892/how-to-convert-url-parameters-to-a-javascript-object
for our use-case, URLSearchParams should be fine.
|
||
import {COLOR_V2} from '@wireapp/react-ui-kit'; | ||
|
||
export const getTeamImageCSS = (): CSSObject => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any particular motivation in using functions instead of plain objects here.
There doesn't seem to be any dynamic values in none of those. Just wondering if you see a particular benefit in having functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah no sorry i just looked at another file and copied the syntax since i haven't done this in a while. lets make them objects
return ( | ||
<Page> | ||
<ContainerXS centerText verticalCenter css={containerCSS}> | ||
{!oAuthApp ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to use a isLoading variable? When we fail to get the oAuthApp for whatever reason, we show the loader indefinitely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tried just now with the redux isFetching
state for the auth Selector. Unfortunately because multiple fetching calls are going off in sequence it creates a bit of a jump.
Since the oAuthApp is the last thing we are loading this works slightly nicer.
i have added a logout if the function errors to prevent an infinite loading case.
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764
The PR Description
What's new in this PR?
Issues
oauth landing page inside of the authorization flow for webapp
Solutions
Briefly describe the solutions you have implemented for the issues explained above.
Dependencies (Optional)
oauth backend needs to be released first.
Needs releases with:
wireapp/wire-server#2901
Testing
Test Coverage (Optional)
How to Test
Briefly describe how this change was tested and if applicable the exact steps taken to verify that it works as expected.
Notes (Optional)
Specify here any other facts that you think are important for this issue.
Attachments (Optional)
Attachments like images, videos, etc. (drag and drop in the text box)
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764
.