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

관리자 모드 개선 및 https 변환 메뉴 추가, workflow 추가 #239

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

Conversation

sooobee
Copy link

@sooobee sooobee commented Jun 15, 2023

안녕하세요. 전북대학교 컴퓨터인공지능학부 4학년 공리, 권시은, 박진용, 김수비 입니다.

오픈소스에 기여해야하는 학교 과제로 인해 그누보드를 직접 사용해보고 다음과 같은 부분을 개선하면 사용자에게 더욱 편리할 것 같아 부족한 걸 알지만 최대한 수정을 진행해 보았습니다.

  1. 관리자 모드 1:1 문의 설정(qa_config.php) 창에서 확인 버튼을 저장과 목록 버튼으로 분리

  2. 관리자 모드 메뉴 설정에서 메뉴 다중 선택 가능하도록 변경

  3. http 보안 강화를 위한 설명 메뉴 추가

  4. CI를 위한 workflow 추가 (push시 설정 php 버전과의 호환성 체크)

감사합니다.

@kkigomi
Copy link
Contributor

kkigomi commented Jun 19, 2023

.DS_Store, .bashrc, .htpasswd, php.ini 등의 시스템 파일이 포함되어 있습니다.
.htpasswd 파일에는 s00bee2 계정의 패스워드도 포함되어 있습니다(hash 된 패스워드지만요).
동작에 불필요한 파일은 제거하시는게 좋을 것 같네요.

참고로 @sooobee 님의 4p 브랜치에 추가 커밋을 하면 이 PR에 계속해서 반영되므로 다시 PR을 등록하실 필요가 없습니다.

@kkigomi
Copy link
Contributor

kkigomi commented Jun 19, 2023

PHPUnit 테스트 코드가 /adm에 위치한 것 및 adm/menu_listTest.php 파일은 예시 수준의 무의미한 테스트를 실행하고 있는 등 과제 목적이라지만 특정 목적을 이루기 위한 정상적인 변경이 아닌 수업 과제 해결만을 위한 PR로 보여집니다.

  • .htpasswd 파일 등 불필요한 파일이 포함된 문제
  • 하나의 PR에 너무 다양하고 연관성이 없는 변경사항이 포함된 문제
  • 나열한 것 이상으로 불필요한 변경사항이 포함된 문제
  • JS 코드 일부의 주석이 제거되어 오류가 발생할 수 있는 문제가 포함
  • 추가한 기능의 테스트가 아닌 의미 없는 더미용 테스트 코드
  • 코드를 제출한 구성원끼리 코드리뷰를 하신 것으로 알고 있는데 이런 다양한 문제를 발견하지 못한 ‘짜놓은 각본’으로만 리뷰를 주고받기만한 것 등

과제를 내 준 사람이라면 좋은 점수를 줄 필요를 느끼지 못할 것 같습니다.

@smaker
Copy link

smaker commented Jun 19, 2023

@kkigomi 님이 제기한 문제 이외에 좀 더 더하자면,

.htaccess 파일의 내용을 변경하고 있는데, 해당 파일에 쓰기 권한이 없어서 해당 동작이 실패할 수도 있습니다.
쓰기 권한이 없다면, 계속 "코드가 성공적으로 추가되었습니다."라고 나오겠네요.
이 부분에 대한 보완도 필요할 것 같습니다.

여담으로, http -> https로 리다이렉트 하는 기능을 추가하기 위해 .htaccess 파일을 임의로 변경하는 것은 적절하지 못해보입니다. Apache 환경뿐 아니라 nginx도 환경도 있을테고, 이미 해당 소스 코드가 .htaccess에 추가되어 있는 상태에서 "https 적용하기" 버튼을 클릭하면 중복되는 부분만 계속 무의미하게 추가될 뿐입니다.

Comment on lines +21 to +23
<h1>
http 를 https로 바꾸기 위해서는 SSL인증서가 필요합니다.
</h1>

Choose a reason for hiding this comment

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

Suggested change
<h1>
http 를 https로 바꾸기 위해서는 SSL인증서가 필요합니다.
</h1>
<h1>HTTPHTTPS로 바꾸기 위해서는 SSL인증서가 필요합니다.</h1>

@@ -9,7 +9,7 @@ callback function can be used for any purpose. With minor modifications, the
callback function can be used to create CSV logs, post results to databases,
etc.

Please review the test.php script for the example.
Please review the script for the example.

Choose a reason for hiding this comment

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

Suggested change
Please review the script for the example.
Please review the script for the example.

@thisgun
Copy link
Contributor

thisgun commented Dec 18, 2023

안녕하세요. SIR 입니다.

코드를 남겨주셔서 감사합니다.

앞으로 개발 하는데 참고하겠습니다.

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.

None yet

8 participants