Skip to content

Add newPassword page #230

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add newPassword page #230

wants to merge 7 commits into from

Conversation

NadezhdaShosheva
Copy link

Summary

A brief description of the pull request.

Resolves: #138

Screenshots in case of UI changes

Screenshot 2022-01-10 at 10 59 48

Review notes

While reviewing pull-request (especially when it's your pull-request), please make sure that:

  • you named PR close to issue name and linked that issue
  • you understand what problem is solved by PR and how is it solved
  • new tests are in place, no redundant tests
  • required ENV variables added and described in .env.example and added to Heroku
  • associated Heroku review app works correctly with introduced changes

};

const useUpdatePasswordMutation = (
options?: MutationHookOptions<UpdatePasswordResponseData, UpdatePasswordRequestVariables>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the options be optional for mutation?

Copy link
Author

@NadezhdaShosheva NadezhdaShosheva Jan 11, 2022

Choose a reason for hiding this comment

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

In this case it is required


NewPasswordPage.getInitialProps = ({ res, accessTokenManager }) => {
if (accessTokenManager.accessToken) {
if (res) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why, but in other files we use the following conditions if (!!req && !!res) {

Copy link
Contributor

@Regina-Gimazova Regina-Gimazova Jun 8, 2022

Choose a reason for hiding this comment

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

Now for all pages we check only res, so I think we can leave it

return {};
};

export default withApolloClient(WithAuth(NewPasswordPage));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you use WithAuthSecurity HOC?

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 we can leave as is, cause almost all pages do not use this HOC. I'll take a look at this in next prs


import { FieldWrapper, FormContentWrapper, SubmitButtonWrapper } from './styled';

const passwordRegularExp = /^(?=.*[0-9])(?=.*[A-Z])(?=.*[a-z])([0-9A-Za-z#$@&!?.*^{}<>;,)(~'"=_%+-]+)$/;
Copy link
Contributor

Choose a reason for hiding this comment

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

better create new file with all regexp

.required('This field is required'),
});

type ValuesFromFormik = {
Copy link
Contributor

Choose a reason for hiding this comment

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

rename ValuesFromFormik to FormValues. If we replace formik with another form, type of values should not change, as well as name of type


const passwordRegularExp = /^(?=.*[0-9])(?=.*[A-Z])(?=.*[a-z])([0-9A-Za-z#$@&!?.*^{}<>;,)(~'"=_%+-]+)$/;

const initialValues = {
Copy link
Contributor

Choose a reason for hiding this comment

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

add type

@@ -0,0 +1,5 @@
mutation UpdatePassword($input: UpdatePasswordInput!) {
updatePassword(input: $input) {
accessToken
Copy link
Contributor

Choose a reason for hiding this comment

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

refreshToken

@@ -0,0 +1,5 @@
mutation UpdatePassword($input: UpdatePasswordInput!) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oyyy I didn't know what type for $input existed
Maybe better to rename all input type for mutation

Copy link
Contributor

@Regina-Gimazova Regina-Gimazova Jun 8, 2022

Choose a reason for hiding this comment

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

I'm sorry, but I don't understand your point. I think we can leave as is for now

onCompleted,
});

const mutate = async ({ password, resetToken }: UpdatePasswordProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

UpdatePasswordProps should be named UpdatePasswordVariables from 'api/types...'

Copy link
Contributor

Choose a reason for hiding this comment

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

I fixed it


import useUpdatePasswordMutation from 'api/mutations/update/useUpdatePasswordMutation';

export type UpdatePasswordProps = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this type

Copy link
Contributor

Choose a reason for hiding this comment

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

I fixed it

const NewPasswordPage = ({ query }) => {
return (
<NotifierProvider>
<DefaultTemplate testId="recovery-password-page">
Copy link
Contributor

Choose a reason for hiding this comment

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

it is not recovery-password-page


import UpdatePasswordMutation from 'graphql/mutations/updatePassword.graphql';

import type { UpdatePasswordVariables, UpdatePasswordData } from '../../types/user/updatePasswordApiType';
Copy link
Contributor

Choose a reason for hiding this comment

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

should be the absolute path

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.

Reset password form
4 participants