インポートページの改善 #673

kichi2004 · 2023-04-07T09:49:18Z

各授業ごとに，重複の確認のために registered-courses を呼んでいたので，1 回だけ呼ぶように修正
チェックボックスなどが機能していなかったので修正
重複のモーダルが閉じられない問題を修正
すでに存在している授業（科目番号一致）はデフォルトで unchecked になるように変更
- 同じ授業を重複扱いとせずに，差分のみ追加することができるようにしました

vercel · 2023-04-07T09:49:22Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
twinte-front-staging	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 7, 2023 11:39am

HosokawaR

ありがとうございます！その他のエラーも修正してもらい大助かりです！

こちらの共有不足が原因で申し訳ないのですが、いくつか現行のアーキテクチャと乖離していそうな箇所があったので見ていただけると嬉しいです

HosokawaR · 2023-04-07T10:26:56Z

src/application/usecases/course/checkScheduleDuplicate.ts

+export const checkScheduleDuplicate = (
+  { courseRepository }: Ports,
+  registered?: RegisteredCourse[]
+) => async (
  year: number,
  schedules: Schedule[]
 ): Promise<
  boolean | UnauthorizedError | NetworkError | InternalServerError
 > => {


先にアーキテクチャのドキュメントを共有しとくべきでした…！

registeredの引数は以下のようにずらしたほうが現行のアーキテクチャに沿っていると思うのですがいかがでしょうか。

registered はビジネスロジックに必要な引数ですが、一方で{ courseRepository } は DI に必要な引数です。
なので registered は { courseRepository } と並べるよりも year, schedules と並べたほうがいいなと思いました。

Suggested change

export const checkScheduleDuplicate = (

{ courseRepository }: Ports,

registered?: RegisteredCourse[]

) => async (

year: number,

schedules: Schedule[]

): Promise<

boolean | UnauthorizedError | NetworkError | InternalServerError

> => {

export const checkScheduleDuplicate = (

{ courseRepository }: Ports,

) => async (

year: number,

schedules: Schedule[],

registered?: RegisteredCourse[]

): Promise<

boolean | UnauthorizedError | NetworkError | InternalServerError

> => {

HosokawaR · 2023-04-07T10:43:01Z

src/ui/pages/import.vue

-  selected: boolean;
-  expanded: boolean;
-};
+const registered = await ports.courseRepository.getRegisteredCoursesByYear(


この方法ですと UI (Presentation 層) が Repository に依存してしまっており（ports. を介してはいるものの、courseRepository がコードの露出しているから）、現行のアーキテクチャと乖離してしまうため、以下のように UseCase を参照するのがいいと思いました。

Suggested change

const registered = await ports.courseRepository.getRegisteredCoursesByYear(

const registered = await Usecase.getRegisteredCoursesByYear(

src/ui/pages/import.vue

HosokawaR

ありがとうございます！

hayato24s · 2023-04-07T14:43:16Z

@kichi2004

apiを複数回呼び出さないようにする工夫はしてはいるのですが、実際には複数回呼び出されていますね。
下記の画像の再現方法ってわかりますか？
初回ロード時に1回目のapiが帰ってくる前に、2回目のapiを呼び出してしまっているとか？

twinte-front/src/repositories/production/CourseRepository.ts

Lines 188 to 216 in 391f56a

    
           async getRegisteredCoursesByYear( 
        
             year: number 
        
           ): Promise< 
        
             RegisteredCourse[] | UnauthorizedError | NetworkError | InternalServerError 
        
           > { 
        
             if (this.#years.has(year)) { 
        
               const storedCourses = this.#courses.filter( 
        
                 (course) => course.year === year 
        
               ); 
        
               return storedCourses; 
        
             } 
        
             return this.#api.call< 
        
               ApiType.RegisteredCourse[], 
        
               RegisteredCourse[], 
        
               200, 
        
               400 | 401 | 500 
        
             >( 
        
               (client) => client.registered_courses.get({ query: { year } }), 
        
               (body) => { 
        
                 const courses: RegisteredCourse[] = body.map(apiToRegisteredCourse); 
        
                 this.#courses = [...this.#courses, ...courses]; 
        
                 this.#years.add(year); 
        
                 return courses; 
        
               }, 
        
               [200], 
        
               [400, 401, 500] 
        
             ); 
        
           }

HosokawaR · 2023-04-07T14:48:44Z

（代わりに返信）
@hayato24s

https://app.twinte.net/import?codes=GA15211,GA15311,GA15121,GA18212,GA18312,6524102,6424102,6124101,GB13332,1227601,1118402,GA13401,GA12111,GA12301,GA10201,GB13024,GC24401,GB13704,GC13101,1226051,GB12017

をインポートすると再現しました。

Original: https://discord.com/channels/1008574300787322980/1009642761827385434/1093488235759542284

HosokawaR · 2023-04-07T14:52:01Z

うーんただ↑をクリックするだけでは再現しないな…

HosokawaR · 2023-04-07T14:53:57Z

（代わりに返信） @hayato24s

https://app.twinte.net/import?codes=GA15211,GA15311,GA15121,GA18212,GA18312,6524102,6424102,6124101,GB13332,1227601,1118402,GA13401,GA12111,GA12301,GA10201,GB13024,GC24401,GB13704,GC13101,1226051,GB12017

をインポートすると再現しました。

Original: https://discord.com/channels/1008574300787322980/1009642761827385434/1093488235759542284

すみません、~~嘘を付きました~~言葉足らずでした
上記をクリックした後に、追加する講義が表示されると思うのですが、その後に「選択した授業を追加」ボタンを押してください
そのタイミングで大量のリクエストが行われていそうです

hayato24s · 2023-04-07T15:21:12Z

https://app.twinte.net/import?codes=GA15211,GA15311,GA15121,GA18212,GA18312,6524102,6424102,6124101,GB13332,1227601,1118402,GA13401,GA12111,GA12301,GA10201,GB13024,GC24401,GB13704,GC13101,1226051,GB12017

下記の画像は上記のURLにアクセスした時のgetResitredCourseByYearとcheckScheduleDuplicateが呼び出されるタイミングです。

上記の結果を得るためのコード

  async getRegisteredCoursesByYear(
    year: number
  ): Promise<
    RegisteredCourse[] | UnauthorizedError | NetworkError | InternalServerError
  > {
    console.log("getResitredCourseByYear");
    if (this.#years.has(year)) {
      console.log("found year");
      const storedCourses = this.#courses.filter(
        (course) => course.year === year
      );
      return storedCourses;
    }
    console.log("not year");

    return this.#api.call<
      ApiType.RegisteredCourse[],
      RegisteredCourse[],
      200,
      400 | 401 | 500
    >(
      (client) => client.registered_courses.get({ query: { year } }),
      (body) => {
        const courses: RegisteredCourse[] = body.map(apiToRegisteredCourse);
        this.#courses = [...this.#courses, ...courses];
        this.#years.add(year);
        console.log("add year");
        return courses;
      },
      [200],
      [400, 401, 500]
    );
  }

hayato24s · 2023-04-07T15:25:01Z

やはり、1回目のgetResitredCourseByYear に呼び出されたapiの結果が帰ってくる前に2回目以降のgetResitredCourseByYear が呼び出されてしまっているようです。

hayato24s · 2023-04-07T15:32:40Z

src/ui/pages/import.vue

  result.map((course) => ({
    course: courseToDisplay(course),
    schedules: course.schedules,
-    selected: true,
+    selected: !registeredSet.has(`${course.year}_${course.code}`),


UIの方で「登録済み」のような表示をしたいですね。

ですね！
ただ UI の変更は Figma や CourseCard Component などいろいろ大変そうなので、一旦はユーザの不具合解消を優先して Merge でいいかなーと思っています。

Issue にしました → #676

HosokawaR · 2023-04-07T15:34:39Z

なるほど…

とすれば API インスタンスで Promise Object を保持するみたいな方法とかもいいかもしれませんね (& かつ State にも保存）

async getRegisteredCoursesByYear(
  year: number
): Promise<
  RegisteredCourse[] | UnauthorizedError | NetworkError | InternalServerError
> {
  if (this.#years.has(year)) {
    // キャッシュがある場合は、キャッシュからデータを返す
    const storedCourses = this.#courses.filter(
      (course) => course.year === year
    );
    return storedCourses;
  }

  // 初回の呼び出しの場合、API呼び出しの結果をPromiseで待機する
  if (!this.#apiCallPromise) {
    this.#apiCallPromise = this.#api.call<
      ApiType.RegisteredCourse[],
      RegisteredCourse[],
      200,
      400 | 401 | 500
    >(
      (client) => client.registered_courses.get({ query: { year } }),
      (body) => {
        const courses: RegisteredCourse[] = body.map(apiToRegisteredCourse);
        this.#courses = [...this.#courses, ...courses];
        this.#years.add(year);
        return courses;
      },
      [200],
      [400, 401, 500]
    );
  }

  // 初回の呼び出しの結果を待機する
  const response = await this.#apiCallPromise;

  // 初回の呼び出しの結果をキャッシュする
  if (response.status === "success") {
    const courses: RegisteredCourse[] = response.data;
    this.#courses = [...this.#courses, ...courses];
    this.#years.add(year);
    return courses;
  } else {
    throw new Error("API Error: " + response.error.message);
  }
}

hayato24s · 2023-04-07T15:47:39Z

なるほど…

とすれば API インスタンスで Promise Object を保持するみたいな方法とかもいいかもしれませんね (& かつ State にも保存）

this.#apiCallPromiseはyear毎に持つ必要があります。

hayato24s

@kichi2004

現状のコードでも正常に動作するためマージしてしまって大丈夫です。

もし、修正したいようであればお願いします！

hayato24s · 2023-04-07T15:45:15Z

src/application/usecases/course/checkScheduleDuplicate.ts

+  schedules: Schedule[],
+  registered?: RegisteredCourse[]


const registered = await Usecase.getRegisteredCoursesByYear(ports)(year.value);

上記の箇所でrepositoryに授業が保存されるのでregisteredはなくても大丈夫だと思います。

ただ、この方法は綺麗ではないので将来的には以下の方法がいいかなと思っています。

checkScheduleDuplicate を複数の授業に対応させる

もしくは、repositoryでapiの呼び出しの記録を持っておいて、同様のapiを複数回呼び出そうとしたときに初回のapiの結果が帰っているまでawaitして、初回のapiの結果が帰ってきたら、それを返す。 <- 上記の細川くんのやつです

インポートページの改善

63bdd30

kichi2004 requested a review from HosokawaR April 7, 2023 09:49

HosokawaR requested a review from hayato24s April 7, 2023 09:50

HosokawaR reviewed Apr 7, 2023

View reviewed changes

HosokawaR mentioned this pull request Apr 7, 2023

Sentry にユーザ情報を追加 #674

Merged

kichi2004 requested a review from HosokawaR April 7, 2023 11:36

設計に関する問題を修正

0b35a14

vercel bot deployed to Preview April 7, 2023 11:39 View deployment

HosokawaR approved these changes Apr 7, 2023

View reviewed changes

hayato24s reviewed Apr 7, 2023

View reviewed changes

hayato24s approved these changes Apr 7, 2023

View reviewed changes

kichi2004 merged commit 10e28d2 into main Apr 9, 2023

kichi2004 deleted the feature/672-fix-import branch April 9, 2023 08:02

HosokawaR mentioned this pull request May 5, 2023

インポート画面の問題の修正 #672

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

インポートページの改善 #673

インポートページの改善 #673

kichi2004 commented Apr 7, 2023

vercel bot commented Apr 7, 2023 •

edited

Loading

HosokawaR left a comment

HosokawaR Apr 7, 2023

HosokawaR Apr 7, 2023 •

edited

Loading

HosokawaR left a comment

hayato24s commented Apr 7, 2023 •

edited

Loading

HosokawaR commented Apr 7, 2023 •

edited

Loading

HosokawaR commented Apr 7, 2023

HosokawaR commented Apr 7, 2023 •

edited

Loading

hayato24s commented Apr 7, 2023 •

edited

Loading

hayato24s commented Apr 7, 2023

hayato24s Apr 7, 2023

HosokawaR Apr 7, 2023

HosokawaR commented Apr 7, 2023 •

edited

Loading

hayato24s commented Apr 7, 2023

hayato24s left a comment

hayato24s Apr 7, 2023 •

edited

Loading

	const registered = await ports.courseRepository.getRegisteredCoursesByYear(
	const registered = await Usecase.getRegisteredCoursesByYear(

インポートページの改善 #673

インポートページの改善 #673

Conversation

kichi2004 commented Apr 7, 2023

vercel bot commented Apr 7, 2023 • edited Loading

HosokawaR left a comment

Choose a reason for hiding this comment

HosokawaR Apr 7, 2023

Choose a reason for hiding this comment

HosokawaR Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

HosokawaR left a comment

Choose a reason for hiding this comment

hayato24s commented Apr 7, 2023 • edited Loading

HosokawaR commented Apr 7, 2023 • edited Loading

HosokawaR commented Apr 7, 2023

HosokawaR commented Apr 7, 2023 • edited Loading

hayato24s commented Apr 7, 2023 • edited Loading

hayato24s commented Apr 7, 2023

hayato24s Apr 7, 2023

Choose a reason for hiding this comment

HosokawaR Apr 7, 2023

Choose a reason for hiding this comment

HosokawaR commented Apr 7, 2023 • edited Loading

hayato24s commented Apr 7, 2023

hayato24s left a comment

Choose a reason for hiding this comment

hayato24s Apr 7, 2023 • edited Loading

Choose a reason for hiding this comment

vercel bot commented Apr 7, 2023 •

edited

Loading

HosokawaR Apr 7, 2023 •

edited

Loading

hayato24s commented Apr 7, 2023 •

edited

Loading

HosokawaR commented Apr 7, 2023 •

edited

Loading

HosokawaR commented Apr 7, 2023 •

edited

Loading

hayato24s commented Apr 7, 2023 •

edited

Loading

HosokawaR commented Apr 7, 2023 •

edited

Loading

hayato24s Apr 7, 2023 •

edited

Loading