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

Fix: exclude img from search #300

Merged
merged 25 commits into from
Sep 16, 2022
Merged

Fix: exclude img from search #300

merged 25 commits into from
Sep 16, 2022

Conversation

Yaminyam
Copy link
Member

바뀐점

검색할때 img 태그에 포함된 내용을 제외

바꾼이유

#299
img 태그에 포함된 검색어를 검색시 이미지가 포함된 모든 게시글이 검색됨

설명

Skyrich2000 and others added 16 commits January 16, 2022 22:59
🔥 Main Merge 1주차 🏃‍♀️🏃‍♂️🏃
🔥 Main Merge 2주차 🏃‍♀️🏃‍♂️🏃
🔥 Main Merge 3주차 🏃‍♀️🏃‍♂️🏃
🔥 Main Merge 🏃‍♀️🏃‍♂️🏃
Setting: 자동으로 assignee 할당해주는 액션 추가
Revert "Setting: 자동으로 assignee 할당해주는 액션 추가"
🔥 Main Merge 🏃‍♀️🏃‍♂️🏃
🔥 Main Merge 🏃‍♀️🏃‍♂️🏃
@Yaminyam Yaminyam changed the title Fix/exclude img from search Fix: exclude img from search Aug 12, 2022
article.content.replace(/![\S*](\S+)/g, '').indexOf(options.q) !== -1 ||
article.title.indexOf(options.q) !== -1,
);
return { articles: filteredArticles, totalCount };
Copy link
Member

Choose a reason for hiding this comment

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

생각해보니까 totalCount 개수가 안맞으면 pagenation 이 잘 안될 수 있겠네유......
이거는 db query를 손봐야할거같은데 query에서 정규식을 써야할수도 있겠네유....

Copy link
Member Author

Choose a reason for hiding this comment

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

아..그러게요 토탈카운트도 같이 변경해야할까요

Copy link
Member

Choose a reason for hiding this comment

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

articles는 페이지네이션된 배열이 나온거고, totalCount 는 전체 검색결과 개수라서...
db 단에서 처리해야할거같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

Db에서 정규식을 먹일수 있으려나 좀 알아봐야겠네요

Copy link
Member

Choose a reason for hiding this comment

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

@Skyrich2000 Skyrich2000 added skip:ci github action 작동을 skip and removed skip:ci github action 작동을 skip labels Aug 27, 2022
@Yaminyam Yaminyam added the bug Something isn't working label Sep 6, 2022
@Yaminyam Yaminyam linked an issue Sep 6, 2022 that may be closed by this pull request
q: `%${options.q}%`,
});
qb.where('article.title like :q', { q: `%${options.q}%` }).orWhere(
'regexp_replace(`article`.`content`, "!\\[[[:print:]]+\\]\\([[:print:]]+\\)", "") like :q',
Copy link
Member

Choose a reason for hiding this comment

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

정규식고수시네요

Copy link
Member

Choose a reason for hiding this comment

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

아 이거 게시글 검색할때 이미지 날려주는 그 정규식인가요
[:print:] 는 무엇인가요

Copy link
Member Author

Choose a reason for hiding this comment

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

저희가 javascript에서 사용하는 정규식은 좋은 정규식인데 mysql은 구려서 posix라는 정규식을 사용해서
[:print:]+가 s+ 라고 생각하시면 됩니다...ㅋㅋ

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

헐 특이하네요 ㅋㅋㅋ

@@ -61,9 +61,8 @@ services:
order: start-first
#-----------------------------------------------------------------------------------
db:
image: mysql:5.7
image: mysql:8.0
Copy link
Member

Choose a reason for hiding this comment

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

mysql의 버전을 올리셨군요 멋집니다

Copy link
Member Author

Choose a reason for hiding this comment

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

mysql 5버전에서는 정규식 지원이 안되고
8버전이 쿼리처리 속도도 훨씬 빠르다고해서 저번에 같이 날잡고 업글했습니다! ㅎㅎ

Copy link
Member

Choose a reason for hiding this comment

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

그리고보니 요거는 저번에 버전 올리던 PR이 따로 있지 않았나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Skyrich2000 체리픽 입니다 ㅋㅋ

Comment on lines 677 to 678
const contentWithoutSearchWord =
'bbbbbb![image.png](https://42world-image.s3.ap-northeast-2.amazonaws.com/111111111.png)';
Copy link
Member

Choose a reason for hiding this comment

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

기존 테스트를 수정하기보다는, 이미지 링크에 있는 숫자는 제외하는 테스트를 따로 추가하는편이 좋을거같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

테스트한다고 수정했었던것 같은데 잊고있었네요 ㅋㅋ

@Skyrich2000
Copy link
Member

이런식응로 db 와 직접적으로 소통하는 repository는 unit test 를 어떻게 짜야할지 고민되긴 하네요 @rockpell 님 이런건 어떻게 mocking 하나요?

@rockpell
Copy link
Member

rockpell commented Sep 7, 2022

이런식응로 db 와 직접적으로 소통하는 repository는 unit test 를 어떻게 짜야할지 고민되긴 하네요 @rockpell 님 이런건 어떻게 mocking 하나요?

레포지토리 유닛 테스트는 데이터베이스 붙여서 테스트하면 될거 같습니다

Copy link
Member

@blingblin-g blingblin-g left a comment

Choose a reason for hiding this comment

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

정규식 고수네요 👍

@Yaminyam Yaminyam merged commit d4d0c6d into develop Sep 16, 2022
@Yaminyam Yaminyam deleted the fix/exclude-img-from-search branch September 16, 2022 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] 검색에서 이미지 링크 제외
6 participants