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.

Sign up for GitHub

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

비밀번호 재설정 기능 버그 수정 #93

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

비밀번호 재설정 기능 버그 수정 #93

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8fa18b0
fix: 비밀번호 재설정 성공 시 로그인 후, home으로 이동
live-small Aug 16, 2022
a3a2ecf
fix: 비밀번호 재설정 페이지 만료기간이 지났을 때 error 페이지로 이동
live-small Aug 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Routes.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ const AuthedContainer = () => {
<Route exact path="/" component={Home} />
{/* Direct */}
<Route path="/direct" component={Direct} />
<Redirect to="/" />
{/* 404 페이지 필요*/}
</>
);
Expand Down
4 changes: 0 additions & 4 deletions src/app/store/ducks/auth/authSlice.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import {
getUserInfo,
signIn,
resetPassword,
checkCurrentURL,
signInUseCode,
logout,
} from "./authThunk";
Expand Down Expand Up @@ -125,9 +124,6 @@ const authSlice = createSlice({
.addCase(resetPassword.rejected, (state) => {
state.errorMessage = `전에 사용한 적 없는 새로운 비밀번호를 만드세요.`;
})
.addCase(checkCurrentURL.rejected, (state) => {
// 유효하지 않은 url **
})
.addCase(logout.fulfilled, (state) => {
state.isLogin = false;
});
Expand Down
24 changes: 8 additions & 16 deletions src/app/store/ducks/auth/authThunk.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,22 +63,14 @@ export const getUserInfo = createAsyncThunk<AuthType.UserInfo>(
export const checkCurrentURL = createAsyncThunk<
void,
{ code: string; username: string }
>("auth/checkResetPassword", async (payload, ThunkOptions) => {
try {
const config = {
params: {
code: payload.code,
username: payload.username,
},
};
const { data } = await customAxios.get(
`/accounts/password/reset`,
config,
);
console.log(data);
} catch (error) {
throw ThunkOptions.rejectWithValue(error);
}
>("auth/checkResetPassword", async (payload) => {
const config = {
params: {
code: payload.code,
username: payload.username,
},
};
await customAxios.get(`/accounts/password/reset`, config);
Comment on lines 64 to +73
Copy link
Collaborator

Choose a reason for hiding this comment

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

스크린샷 2022-09-07 오후 4 40 22
에러가 나지 않고 200번 대 response가 오더라도 "올바르지�않은 비밀번호 재설정 코드입니다." 인 경우에는 에러 처리를 해줘야 하지 않을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

dispatch(checkCurrentURL({ code, username }))
            .unwrap()
            .catch(() => {
                history.push("/error");
            });
    }, []);

여기 catch문에서 처리될 거라고 생각했는데, 200번대였군요..! 이 부분도 에러처리 반영하겠습니다. 감사합니다

});

export const resetPassword = createAsyncThunk<
Expand Down
9 changes: 7 additions & 2 deletions src/components/Auth/ResetPassword/ResetPasswordForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useLocation } from "react-router-dom";
import { useHistory, useLocation } from "react-router-dom";
import queryString from "query-string";
import HeaderBeforeLogin from "./HeaderBeforeLogin";
import ContentBox from "components/Common/ContentBox";
Expand Down Expand Up @@ -77,6 +77,7 @@ export default function ResetPasswordForm() {
search,
) as AuthType.resetPasswordQuery;
const dispatch = useAppDispatch();
const history = useHistory();
const { errorMessage } = useAppSelector((state) => state.auth);

const [newPasswordInputProps, newPasswordIsValid] = useInput(
Expand All @@ -92,7 +93,11 @@ export default function ResetPasswordForm() {
);

useEffect(() => {
dispatch(checkCurrentURL({ code, username }));
dispatch(checkCurrentURL({ code, username }))
.unwrap()
.catch(() => {
history.push("/error");
});
}, []);
Comment on lines +96 to 101
Copy link
Collaborator

@kimyoungyin kimyoungyin Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

  • thunk나 slice에서 라우트 이동이 불가능해서 이렇게 컴포넌트 내부에서 처리하도록 한 걸까요?(unwrap())
    -> 내부에서 useEffect로 위 동작을 처리한 경우 컴포넌트가 마운트 된 후에 코드 유효성 검사를 하게 되는데, 이 때 코드가 유효하지 않을 때 의미 없는 렌더링이 발생할 것 같아요. 굳이 그럴 필요 없이 상위 컴포넌트에서 로딩 처리를 위임하여 재설정 코드 유효성이 통과된다면 하위 컴포넌트인 ResetPasswordForm을 렌더링하도록 하는 건 어떨까요?
  • deps에 dispatch, code, username을 넣지 않은 이유가 있을까요?
  • thunk별 수행하는 동작이: checkCurrentURL: 재설정 하기 위해 주어진 코드가 유효한지 확인, resetPassword: 비밀번호 재 설정 맞을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

하나씩 답변드릴게용!

deps에 dispatch, code, username을 넣지 않은 이유가 있을까요?

code, username은 백엔드가 넘겨준 url에서 추출해오기 때문에, 최초 렌더링 이후 변경될 가능성이 없기때문에 deps에 넣을 필요를 못 느꼈습니다.

dispatch는 다른 곳에서도 계속 안넣어왔던 거 같은데, 영인님은 dispatch를 넣는 이유가 있으신가요?


thunk별 수행하는 동작이: checkCurrentURL: 재설정 하기 위해 주어진 코드가 유효한지 확인, resetPassword: 비밀번호 재 설정 맞을까요?

넵 맞습니다!


thunk나 slice에서 라우트 이동이 불가능해서 이렇게 컴포넌트 내부에서 처리하도록 한 걸까요?(unwrap())

네 맞아요. 이 부분 꽤나 고민했었는데..! 꼼꼼히 봐주셔서 감사합니다!

내부에서 useEffect로 위 동작을 처리한 경우 컴포넌트가 마운트 된 후에 코드 유효성 검사를 하게 되는데, 이 때 코드가 유효하지 않을 때 의미 없는 렌더링이 발생할 것 같아요. 굳이 그럴 필요 없이 상위 컴포넌트에서 로딩 처리를 위임하여 재설정 코드 유효성이 통과된다면 하위 컴포넌트인 ResetPasswordForm을 렌더링하도록 하는 건 어떨까요?

  • 아 그렇네요. 의미없는 렌더링..! useEffect가 렌더링 전에 실행되는 줄 알았는데, 렌더링 이후에 실행되는 거더라구요. 영인님 의견이 좋은 거 같습니다. 그렇게 수정해서 커밋해놓을 게요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • https://okky.kr/articles/957064: dispatch를 deps에 넣을지에 대한 내용인데, 전 불변성이 유지되긴 하더라도 넣어주는 편이었습니다!
  • unwrap(): 확인해보니 authSlice에서 checkCurrentURL가 Return하는 promise에 대한 state 변화는 없네요! 저는 얼마 전까지도 몰랐던 건데, unwrap() 처리를 하더라도 addCase에 등록된 해당 thunk extraReducer는 호출되더라구요.. 참고하세요!

렌더링 관련 커밋 사항 발생하면 리뷰 하도록 하겠습니다!


const resetPasswordClickHandler = (
Expand Down