for recreate PR by Klay24-Huang · Pull Request #14 · NTUCourse-Neo/ncn-backend-v2

Klay24-Huang · 2022-10-18T06:06:20Z

先前的PR 誤按merge
很抱歉

swh00tw · 2022-10-18T06:09:43Z

標不到 Max，@max 記得來看 xd

prisma/new/schema.prisma

…Foreign to not null

swh00tw

Overall looks great. I listed some issues below.

typo
every field should use camelCase
improper naming, ex: ableAddRollNumber (Ambiguous.)
+s when it's 1-to-n or m-to-n mapping (I saw you do this, but sometimes you don't. It's inconsistent)

What you should do are correct all the problems I found & check again each line. About improper naming, feel free to let us know on Slack if you don't know how to translate~

prisma/new/schema.prisma

maxchou415 · 2022-11-08T00:39:53Z

src/routes/api/v2/users.js

      return;
    }
-    const course = await prisma.courses.findUnique({ where: { id: course_id } });
+    const course = await prisma.courses.findUnique({


We should handle SQL execution with a try-catch wrapping

這邊是neo的api
是排版的時候動到了
是否就保持原樣?

Just leave it. It's okay since you are going to create a new repo in the future.
But remember to wrap SQL execution in try-catch blocks in your new repo.

My question was "wrapping the SQL execution with try-catch" instead of the change itself. No matter if that is from the Neo API, if there is anything that can be improved, just refactor it right away. Please wrap the SQL execution in the next PR.

maxchou415 · 2022-11-08T00:40:59Z

src/routes/api/v2/users.js

+      where: { id: course_id },
+    });
    if (!course) {
      res.status(400).send({ message: "Course not found." });


HTTP status code 400 cannot indicate the Course not found, it should be either 200 or 404 depending on the exact behaviour.

This change is probably caused by eslint & prettier. @Klay24-Huang remember to use 404 in your new repo.

maxchou415 · 2022-11-08T00:43:12Z

prisma/new/schema.prisma

+/// 修課限制
+// ex: 限學士班二年級以上
+// 來自舊系統 NOL table `ifcrfsl`
+model Limit {


We need a better naming for this Limit table since the Limit is meaningful, we will need a specified name.

我只能想到叫CourseLimit，這樣會不會太直翻😂

maxchou415 · 2022-11-08T00:43:49Z

prisma/new/schema.prisma

+  createdAt DateTime @default(now())
+}
+
+model LimitTranslation {


We will need to update this name since the Limit will need to be changed.

maxchou415 · 2022-11-08T00:49:32Z

prisma/new/schema.prisma

+  /// 領域專長代碼
+  code  String  @id @db.VarChar(10)
+  /// 是否通過
+  apply Boolean


If this field is for marking status with a type boolean, we should use past tense for its naming, for this field, I will suggest isAdmitted as the naming.

Also, the reason for using admitted is - from my understanding, this field marks the status of the application submitted by the student, but please correct me if I am wrong.

我會改成isAdmitted
補充一下領域專長的話應該是各系所提出，最後被某個單位審核通過

maxchou415 · 2022-11-08T00:50:45Z

prisma/new/schema.prisma

+  // 原本於ceiba就有兩種形式
+  // table office_time_sw.sw
+  // 為0時，表示為須預約。 為1時，固定時
+  needReserved  Boolean?


What about isRequiredAppointment?

maxchou415 · 2022-11-08T00:57:35Z

prisma/new/schema.prisma

+  courseCollections         CourseCollection[]
+  userInfoTranslations      UserInfoTranslation[]
+  preseletectedCourseTables PreseletectedCourseTable[]
+}


We need these fields for better ACL -

1. createdAt - default value should be now(), and not nullable. 2. deletedAt - default value should be null, nullable. 3. isActive - default value can be true.

想問第三點
是指所有bool type的field都要有預設值嗎
但是這些資料都是從別的資料庫來的
通常都會有值
這樣還有需要default value嗎

maxchou415 · 2022-11-08T01:00:50Z

prisma/new/schema.prisma

+  name      String
+  /// 組別， 只有學生才有
+  // ex: 國際關係組
+  group     String?


Can the dataType be ending in a question mark?

這是prisma當field可以為nullable的寫法

maxchou415 · 2022-11-08T01:04:53Z

prisma/new/schema.prisma

+  name      String
+  /// 組別， 只有學生才有
+  // ex: 國際關係組
+  group     String?


As I remember, the department name is not separating the department name and group name, so I might be - { "name": "政治學系國際關係組" } instead of { "name": "政治學系", "group": "國際關係組" }. Please make sure the data payload before we create it.

這個部份我會再去確認
這邊需要申請學生資料的DB訪問權限
可能沒這麼快

剛剛有去確認學生學籍資料庫
我這邊的name是學生名稱
有的學生填基本資料時有填英文名字
或是外籍生有取中文名字

group的話會是政治系國際關係組 or DEPARTMENT OF POLITICAL SCIENCE, INTERNATIONAL RELATIONS DIVISION 的i18n

maxchou415 · 2022-11-08T01:07:55Z

prisma/new/schema.prisma

+  /// 教育學程
+  EducationProgram
+  /// 夜間部
+  NightSchool


ContinuingEducation is the common name for 夜間部. (推廣教育) XD

maxchou415

Request change

prisma/new/schema.prisma

…s' to 'intervals'

prisma/new/schema.prisma

swh00tw · 2022-11-10T03:54:07Z

prisma/new/schema.prisma

+  departmentId              String?                    @db.VarChar(4)
+  department                Department?                @relation(fields: [departmentId], references: [id])
+  /// 年級
+  grade                     Int?


Suggested change

grade Int?

grade String?

We need to consider "master" and "phD".

for recreate PR

2ee1142

Klay24-Huang requested a review from swh00tw October 18, 2022 06:06

swh00tw requested a review from jc-hiroto October 18, 2022 06:06

Klay added 2 commits October 26, 2022 09:18

與姿君討論後修改shema

17b30a7

adjust course collection model

89a4e90

swh00tw reviewed Oct 26, 2022

View reviewed changes

prisma/new/schema.prisma Outdated Show resolved Hide resolved

Klay added 3 commits October 27, 2022 16:23

*course collection增加主表 *add isCommon field in course table *adjust as…

b7a6c61

…Foreign to not null

add layer to classroom table

eb867e6

fix typo

92abcea

swh00tw requested changes Oct 28, 2022

View reviewed changes

swh00tw and others added 2 commits October 28, 2022 16:11

make linter happy

d11ef30

resolve PR reviews

3372f1c

swh00tw approved these changes Oct 31, 2022

View reviewed changes

maxchou415 reviewed Nov 8, 2022

View reviewed changes

maxchou415 suggested changes Nov 8, 2022

View reviewed changes

resolve PR reviews

880519e

Klay24-Huang commented Nov 9, 2022

View reviewed changes

prisma/new/schema.prisma Outdated Show resolved Hide resolved

swh00tw reviewed Nov 9, 2022

View reviewed changes

prisma/new/schema.prisma Outdated Show resolved Hide resolved

swh00tw reviewed Nov 9, 2022

View reviewed changes

prisma/new/schema.prisma Outdated Show resolved Hide resolved

Klay added 2 commits November 9, 2022 16:41

*change field name 'specialization' to 'specialized' * change 'lesson…

d73cf7d

…s' to 'intervals'

add enroll method to course table

43820ee

swh00tw reviewed Nov 10, 2022

View reviewed changes

prisma/new/schema.prisma Outdated Show resolved Hide resolved

swh00tw reviewed Nov 10, 2022

View reviewed changes

prisma/new/schema.prisma Outdated Show resolved Hide resolved

swh00tw reviewed Nov 10, 2022

View reviewed changes

resolve PR reviews

6f37f7f

Conversation

Klay24-Huang commented Oct 18, 2022

Uh oh!

swh00tw commented Oct 18, 2022

Uh oh!

Uh oh!

swh00tw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxchou415 Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maxchou415 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

maxchou415 Nov 9, 2022 •

edited

Loading