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 level filters #711

Merged
merged 17 commits into from
Feb 4, 2020
Merged

Fix level filters #711

merged 17 commits into from
Feb 4, 2020

Conversation

clockvoid
Copy link
Contributor

Issue

Overview (Required)

  • rename AudienceCategory to Level
  • create member level to Session model
  • create LevelEntity
  • fix FilterTest
  • rename AudienceCategory to Level

日本語でこの変更を行った理由と私の思いについて書かせてください.
まず,AudienceCategoryという名前は去年のものだと思いますが,今年はLevelということで,視聴者のレベルではなく,セッションそのもののレベルであり,これは,セッションそのものの特性を指していることから,すべてのAudienceCategoryと名のついたものをLevelという名前に変えさせていただきました.
その際,iOS側のクラスの名前も変わってしまうことになります(なるはずですよね?)が,iOSではまだフィルターは実装されておらず,変更の効く範囲だと思いますので,このようにさせていただきました.
また,セッションのエンティティにlevelsという要素が加わったことにより,キャッシュのデータベースが変更されました.この事により,アプリのストレージをクリアしないとアップデート時に機能しなくなりますが,マイグレーションの処理は書いていません.理由としては,このアプリはまだPlayStoreに上がっておらず,開発段階であり,開発者の皆さまがアプリを再インストールすることで使えるようになるならば,スピード感を優先してマイグレーションはつけなくて良いと判断したためです.
最後に,テストについてです.今回私がいじったのはmodelモジュールですが,このモジュールのjvmTestモジュールはなぜかテストとして認識されていませんでした.model/build.gradleを見ると,jvm()の記述がなかったため,それを追加することによりjvmTestが動くようにしました.また,FilterTestに関しましては,今回の変更ですべてのパターンを調べると18通りとなり,非常に記述が長くなってしまうため,最小限ですべてのパターンを調べることができているように記述したつもりです.

Links

Screenshot

Before After

import androidx.room.ColumnInfo
import io.github.droidkaigi.confsched2020.data.db.entity.LevelEntity

internal data class LevelEntityImpl (
Copy link
Collaborator

Choose a reason for hiding this comment

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

🚫 Unexpected spacing before “(“

@@ -0,0 +1,13 @@
package io.github.droidkaigi.confsched2020.data.db.internal.entity.mapper

import io.github.droidkaigi.confsched2020.data.db.entity.LevelEntity
Copy link
Collaborator

Choose a reason for hiding this comment

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

🚫 Unused import

@@ -97,7 +99,7 @@ data class SpeechSession(
override val isFavorited: Boolean,
val speakers: List<Speaker>,
val message: LocaledString?
) : Session(id, title, desc, dayNumber, startTime, endTime, room, isFavorited), AndroidParcel {
) : Session(id, title, desc, dayNumber, startTime, endTime, room, isFavorited, levels), AndroidParcel {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🚫 Exceeded max line length (100) (cannot be auto-corrected)

val sessionType: SessionType,
override val isFavorited: Boolean
) : Session(id, title, desc, dayNumber, startTime, endTime, room, isFavorited), AndroidParcel {
) : Session(id, title, desc, dayNumber, startTime, endTime, room, isFavorited, levels), AndroidParcel {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🚫 Exceeded max line length (100) (cannot be auto-corrected)

filterItem = setOf(AudienceCategory.BEGINNERS, AudienceCategory.UNSPECIFIED),
isForBeginners = false,
Param.forLevels(
title = "filter has Beginners and Intermediate passes Intermediate and Advanced session",
Copy link
Collaborator

Choose a reason for hiding this comment

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

🚫 Exceeded max line length (100) (cannot be auto-corrected)

@takahirom
Copy link
Member

アプリ内で実態にあった名前を変えるのはいいと思います!!👍👍

jvmの記述がなかったため,それを追加することによりjvmTestが動くようにしました

👍👍👍👍

マイグレーションについてですが、現在マイグレーションが必要になると自動的にDBが破棄される設定になっています!スキーマのバージョンだけ上がっていれば大丈夫です!

@clockvoid
Copy link
Contributor Author

スキーマのバージョンを上げておりませんので,変更します

@nashcft
Copy link
Contributor

nashcft commented Feb 4, 2020

👍👍👍

jvmTest についてですが、現状CIではテストが実行されていないので、.circleci/config.yml.github/workflows/on_release_branch.yml のテストを実行している箇所に jvmTest タスクを追加すると良さそうに思いました 👀

@clockvoid
Copy link
Contributor Author

@nashcft
I added jvmTest to .circleci/config.yml like this, but CircleCI's Gradle said No cached version of com.soywiz.korlibs.klock:klock-jvm:1.8.6 available for offline mode..
In my environment, ./gradlew cleanAllTests testDebugUnitTest jvmTest --offline is run and pass correctly. (Of course, I cleaned in advance.)
I am short on experience with CI so, I don't understand why it happens.
Please suggest me some solutions 🙏

@clockvoid
Copy link
Contributor Author

clockvoid commented Feb 4, 2020

I think we have to create a cache with jvmTest dependencies when caching all of the other structures.
However, I can't find the function of caching dependencies for now...

package io.github.droidkaigi.confsched2020.model

enum class Level(val rawValue: LocaledString) {
BEGINNER(LocaledString("初級", "Beginner")),
Copy link
Member

@takahirom takahirom Feb 4, 2020

Choose a reason for hiding this comment

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

I think it is better to have a string such as "BEGINNER" as a property. Level(val id :String, val rawValue: LocaledString) This is because this class is caused by the change of API response. The property name can be id. 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see.
I agree with you. I will fix it!

@takahirom
Copy link
Member

Thanks! Almost LGTM!

@nashcft
Copy link
Contributor

nashcft commented Feb 4, 2020

@clockvoid
You're right, we should have cached dependencies for jvmTest but it seems we don't.
Resolving and downloading dependencies are executed here. I'm not sure but it seems that dependencies task doesn't download the listed dependencies unlike androidDependencies (and androidDependenciesExtra, which is extended version defined in this project).

@takahirom
Copy link
Member

It's better to have a cache, but after the merge. 👍

@jmatsu-bot
Copy link
Collaborator

No issue was reported. Cool!

Generated by 🚫 Danger

@jmatsu-bot
Copy link
Collaborator

Your apk has been deployed to https://deploygate.com/distributions/a6778c80b527cb2ca5b5bdc7db3a9bdd2418dacc. Anyone can try your changes via the link.

Generated by 🚫 Danger

@nashcft
Copy link
Contributor

nashcft commented Feb 4, 2020

@takahirom
I think it's OK but we have to remove jvmTest task from CircleCI workflow before merge because no test is executed in this branch now.

@takahirom
Copy link
Member

It's a problem that the test doesn't run, but on another branch, we seem to have a problem with CI testing. 👀 ( #671 )
#714 (comment)

@nashcft
Copy link
Contributor

nashcft commented Feb 4, 2020

I see. Thanks for the information 🙏

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.

Level filter does not work correctly?
4 participants