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(agreement): change to manage agreements dynamically #747

Merged
merged 15 commits into from
May 19, 2024

Conversation

woowahan-pjs
Copy link
Contributor

Resolves #738

해결하려는 문제가 무엇인가요?

  • 개인정보 수집 및 이용 동의서가 클라이언트 코드에 포함되어 있어 수정이 필요할 때마다 코드를 수정하고 배포해야 했습니다.

어떻게 해결했나요?

  • 동의서를 데이터베이스에서 관리하고, 클라이언트가 서버에서 받을 수 있도록 변경하였습니다.

어떤 부분에 집중하여 리뷰해야 할까요?

참고 자료


RCA 룰

r: 꼭 반영해 주세요. 적극적으로 고려해 주세요. (Request changes)
c: 웬만하면 반영해 주세요. (Comment)
a: 반영해도 좋고 넘어가도 좋습니다. 그냥 사소한 의견입니다. (Approve)

Copy link

@woowahan-neo woowahan-neo left a comment

Choose a reason for hiding this comment

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

따봉

create table agreement
(
id bigint not null auto_increment,
content varchar(5000) not null,

Choose a reason for hiding this comment

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

a: 만약 5000 넘어갈 일이 있다면 있을 것 같으면 미리 TEXT로 만들어둬도 좋겠네요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#738 (comment) 에서 실제로 사용되는 동의서는 636자, 1,180바이트이네요. 1,000자를 넘을 가능성은 거의 없어 보이지만, 레코드가 약 8,000바이트 정도를 넘어야 길이가 긴 컬럼을 off-page에 저장한다고 하니 지금은 걱정하지 않아도 될 것 같아요.

Copy link

@woowabrie woowabrie left a comment

Choose a reason for hiding this comment

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

로컬에서 잘 동작하는 것 확인했습니다. 👍👍
�230527 버전의 동의서는 DB insert로 넣는 것이 맞겠죠?

import io.kotest.matchers.shouldBe
import io.kotest.matchers.string.shouldContain

class IndentTest : StringSpec({

Choose a reason for hiding this comment

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

테스트 👍👍

@@ -92,7 +103,7 @@ const Header = () => {
<>
<Link to={PATH.LOGIN}>로그인</Link>
<div className={styles.bar} />
<Link to={PATH.SIGN_UP}>회원가입</Link>
<a onClick={goToSignUp}>회원가입</a>

Choose a reason for hiding this comment

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

c: href 없는 a tag로 바꾸면서 cursor가 pointer가 아닌 text 가 되었네요.
css 설정을 하거나 href="#" 넣고 이벤트를 막는 처리가 들어가면 좋을 것 같습니다! (어떤게 정석이죠?ㅋㅋ)

Choose a reason for hiding this comment

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

오! 좋습니다. 제가 놓친 부분이에요 ㅎㅎ
여러 부분 고민했는데 Link에도 href="#"를 넣고 사용할 수 있네요 ㅎㅎ
확인해 보니 브라우저 접근성 측면에서도 href="#" 처리가 좀 더 나아 보입니다!

일관성을 맞추기 위해서 이 부분 코드 수정해서 반영해둘게요!

@woowahan-pjs
Copy link
Contributor Author

@woowabrie
배포 후 실행해야 하는 SQL을 아래에 정리해 두었으며, 관리 화면에서 동의서를 관리할 수 있도록 후속 이슈도 만들었습니다.
#738 (comment)

@woowahan-pjs woowahan-pjs merged commit a377c1b into develop May 19, 2024
@woowahan-pjs woowahan-pjs deleted the feature/agreement branch May 19, 2024 08:15
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.

5 participants