Check favorite sessions in Schedule View #41

ewa1989 · 2024-03-18T11:57:14Z

Add check favorite session feature as below.

Only in ShceduleView, show star icons on the right inside of session row, toggle when the icons are tapped, and persist favorited states.

Ref: #40

Note

persist [String: [Session]] ~~Conference including favorited state~~ in Document ~~using UserDefaults~~ as Json files, in save methods of FileClient ~~DataClient~~
when app launch, try to load [String: [Session]] ~~Conference~~ from Document ~~UserDefaults, and if failed then load from files in Resources~~

…k-favorite-session-feature # Conflicts: # MyLibrary/Sources/ScheduleFeature/Schedule.swift

d-date · 2024-03-19T14:37:50Z

MyLibrary/Sources/DataClient/Client.swift

+  public var saveDay1: @Sendable (Conference) throws -> Void
+  public var saveDay2: @Sendable (Conference) throws -> Void
+  public var saveWorkshop: @Sendable (Conference) throws -> Void


It's better to create UserDefaultsClient instead of DataClient since this will be changed to ApiClient for example.
Also, favorite data is better to store in Document folder instead of User Default. What do you think?

Also, favorite data is better to store in Document folder instead of User Default. What do you think?

I agree for reading UserDefaults documentation saying

The defaults system allows an app to customize its behavior to match a user’s preferences.

Because favorite data are for saving app's state resulting in user operations, rather than customizing app's behavior.
So I will create FileClient or something... I'm thinking appropriate name that explains what it does, not how it does.
If you have any idea, please tell me.

@d-date I created FileClient in 23983b5 and changed save/load area to Document in 0e8b403.
I'll change failed test and create new test for star icon behavior from now.

d-date · 2024-03-19T14:51:06Z

MyLibrary/Sources/ScheduleFeature/Schedule.swift

+      case let .view(.favoriteIconTapped(session)):
+        switch state.selectedDay {
+        case .day1:
+          state.day1 = update(state.day1!, togglingFavoriteOf: session)


I think saving Conference object itself is not good approach. This leads such a whole updating object.
Instead, It might be better for creating [Days: Favorite], which having session hash (I know session.id is better but I don't believe id in handmade-JSON 😶‍🌫️ ).

Instead, It might be better for creating [Days: Favorite], which having session hash

Thank you.
I've chosen easier way came up with, but will try better approach of performance.

To create save method for [Days: Favorite], Days must be shown from DataClient module.
However Days is a type of LocalizedStringKey that is a frozen struct of SwiftUI.
Therefore I think it might be better to create save method for [Session: Bool], what do you think?

Further background...

If putting Days in SharedModels and save [Days: [Sesson]]

SharedModels depends on SwiftUI, I think SharedModels wants to depends only on Foundation.

If putting Days still in ScheduleFeature and make it public

DataClient uses Days for determining save method, and of course ScheduleFeature depends on DataClient already, so there's circular dependence.

Therefore, I resulted in it's better to create [Session: Bool] as Favorites in SharedModels, what do you think of this design idea?

Sorry, I realized that it's better to use Conference as key instead of enum Days!

@d-date In 846098f , changed save data type to [String: [Session]] which key is enforced to get from Conference.title.

d-date · 2024-03-19T14:52:50Z

@ewa1989 Also please add unit tests to ensure that the favorite functions are working properly.

d-date · 2024-03-20T06:07:01Z

FYI: Now Map is move to tab so may conflict your PR.

…tching methods which might change API Client in future

# Conflicts: # MyLibrary/Sources/ScheduleFeature/Schedule.swift

…se favorite data aren't customizing app's behavior

ewa1989 · 2024-03-20T10:36:19Z

@d-date

@ewa1989 Also please add unit tests to ensure that the favorite functions are working properly.

Added in 35b4d14.
Code fixing completed!

d-date · 2024-03-21T00:51:38Z

MyLibrary/Sources/DataClient/DataClient.swift

@@ -49,9 +49,3 @@ extension DataClient: DependencyKey {
    return data
  }
 }
-
-let jsonDecoder = {


Still we need to use.

d-date · 2024-03-21T00:52:29Z

MyLibrary/Sources/DataClient/FileClient.swift

I ask you create in another module. Define in Package.swift.

d-date · 2024-03-21T00:53:47Z

MyLibrary/Sources/DataClient/JSONHandler.swift

Not have to create in another file. Just put in under the client code even if duplicated.

d-date · 2024-03-21T00:56:30Z

MyLibrary/Sources/SharedModels/Favorites.swift

+public struct Favorites: Equatable, Codable {
+  private var eachConferenceFavorites: [String: [Session]]
+
+  public init(eachConferenceFavorites: [(conference: Conference, favoriteSessions: [Session])]) {


Since initialized in second line, first parameter should be removed.

Sorry for my code readability, but first parameter is necessary.

To create Favorites, give pairs which has conference and its favorites sessions.
Then convert given pairs to dictionary of [conference title: its favorites sessions], and set the dictionary to property.

I intentionally use conference as input to avoid using non-existent conference title as key.
I'll continue to think readable name.

It might be better to use reduce(into:)

self.favorites = favorites.reduce(into: [:]) { result, element in result[element.conference.title] = element.favoriteSessions }

d-date · 2024-03-21T00:57:37Z

MyLibrary/Tests/ScheduleFeatureTests/Mocks.swift

@@ -95,3 +97,28 @@ extension Speaker {
    ]
  )
 }
+
+extension Favorites {


You can add mock under the your test code.

I moved these specific mocks near related tests in 4e279e4.

d-date · 2024-03-21T00:58:13Z

MyLibrary/Tests/ScheduleFeatureTests/ScheduleTests.swift

@@ -13,12 +14,14 @@ final class ScheduleTests: XCTestCase {
      $0[DataClient.self].fetchDay1 = { @Sendable in .mock1 }
      $0[DataClient.self].fetchDay2 = { @Sendable in .mock2 }
      $0[DataClient.self].fetchWorkshop = { @Sendable in .mock3 }
+      $0[FileClient.self].loadFavorites = { @Sendable in .mock1 }


Should be separated your test.

I'm sorry that I can't image way to separate.
Could you give me more idea?

Current implementation, loading favorites using FileClient and fetching conferences using DataClient at the same time (onAppear).
So it's required to implement testValue of loadFavorites to test onAppear behavior.

I could only image to separate loadResponse(Result<Favorites, Error>) from fetchResponse(Result<SchedulesResponse, Error>), but this idea also requires testValue of loadFavorites, so I think it doesn't satisfy your review.

NO. This effects to current test case. Should be created another test case.

Since fetching data using DataClient and loading favorites using FileClient are must to be executed at the same time, onAppear, I renamed testFetchData to testOnAppear in 04cefc0.

d-date · 2024-03-21T00:59:37Z

MyLibrary/Sources/ScheduleFeature/Schedule.swift

+  }
+
+  @ViewBuilder
+  func favoriteIcon(for session: Session) -> some View {


These logic should put in Reducer so that we can write test easily.

I'm sorry that I can't image which part of logic to put in Reducer.
Does it mean create new Schedule.Action case and handle it in Reduce?
Or create computed property getting Conference in Schedule.State?

Now I implemented Favorites to use Conference.title as key for getting value of favorited [Session].

I moved these logic to Schedule.swift in d3b9901.

Please remove this method if you have moved.

Sorry for my lack of explanation.
I moved a part of logic as far as possible, so now State.favorites: [String: [Session]] is used in view whether each session is favorited or not, only.

This reverts commit f2622b8.

# Conflicts: # MyLibrary/Package.swift # MyLibrary/Sources/ScheduleFeature/Schedule.swift

d-date

I forget to comment you since stacked in pending 😅

d-date · 2024-03-22T15:18:45Z

MyLibrary/Sources/SharedModels/Favorites.swift

+public struct Favorites: Equatable, Codable {
+  private var eachConferenceFavorites: [String: [Session]]
+
+  public init(eachConferenceFavorites: [(conference: Conference, favoriteSessions: [Session])]) {


It might be better to use reduce(into:)

self.favorites = favorites.reduce(into: [:]) { result, element in result[element.conference.title] = element.favoriteSessions }

d-date · 2024-03-22T15:19:44Z

MyLibrary/Sources/SharedModels/Favorites.swift

+  public mutating func updateFavoriteState(of session: Session, in conference: Conference) {
+    guard var favorites = eachConferenceFavorites[conference.title] else {
+      eachConferenceFavorites[conference.title] = [session]
+      return
+    }
+    if favorites.contains(session) {
+      eachConferenceFavorites[conference.title] = favorites.filter { $0 != session }
+    } else {
+      favorites.append(session)
+      eachConferenceFavorites[conference.title] = favorites
+    }
+  }
+
+  public func isFavorited(_ session: Session, in conference: Conference) -> Bool {
+    guard let favorites = eachConferenceFavorites[conference.title] else {
+      return false
+    }
+    return favorites.contains(session)
+  }
+}


Move into reducer. Favorite should be keep as pure model.

d-date · 2024-03-22T15:20:38Z

MyLibrary/Tests/ScheduleFeatureTests/ScheduleTests.swift

@@ -13,12 +14,14 @@ final class ScheduleTests: XCTestCase {
      $0[DataClient.self].fetchDay1 = { @Sendable in .mock1 }
      $0[DataClient.self].fetchDay2 = { @Sendable in .mock2 }
      $0[DataClient.self].fetchWorkshop = { @Sendable in .mock3 }
+      $0[FileClient.self].loadFavorites = { @Sendable in .mock1 }


NO. This effects to current test case. Should be created another test case.

d-date · 2024-03-22T15:21:53Z

MyLibrary/Sources/FileClient/FileClient.swift

+    }
+  )
+
+  static func saveDataToFile(_ data : Data, named fileName: String) {


Suggested change

static func saveDataToFile(_ data : Data, named fileName: String) {

static func deserialize(data : Data, into fileName: String) {

d-date · 2024-03-22T15:23:34Z

MyLibrary/Sources/FileClient/FileClient.swift

+    try? data.write(to: fileURL)
+  }
+
+  static func loadDataFromFile(named fileName: String) -> Data? {


Suggested change

static func loadDataFromFile(named fileName: String) -> Data? {

static func serialize(from filePath: String) -> Data? {

I renamed them to explain exactly how they do in cfb2fa3.

d-date · 2024-03-26T07:29:20Z

Please prioritize what I said to you in-person. Thank you!

ewa1989 · 2024-03-26T13:58:59Z

I remember your advice at the conference, that was so thankful!
I will do my best to contribute.

…ely in tests.

…and View, and remove Favorites and use typealias instead because Favorites has one property only.

…y do.

ewa1989 · 2024-03-31T14:28:16Z

@d-date
Thank you for your kind review.
I updated the code to mainly focus on conversation with you at the conference, as you said.
As I told, I want to contribute this feature through to the end!

… does

d-date · 2024-04-10T04:19:39Z

MyLibrary/Sources/ScheduleFeature/Schedule.swift

+        return .run { send in
+          await send(
+            .fetchResponse(
+              Result {
+                let day1 = try dataClient.fetchDay1()
+                let day2 = try dataClient.fetchDay2()
+                let workshop = try dataClient.fetchWorkshop()
+                return .init(day1: day1, day2: day2, workshop: workshop)
+              }))
+          await send(
+            .loadResponse(
+              Result {
+                try fileClient.loadFavorites()
+              }))
+        }


It might be better to change response to Result<(SchedulesResponse, Favorites), Error> .

Thank you, fixed simpler in 0e3b9f8.

d-date · 2024-04-10T04:24:21Z

MyLibrary/Sources/ScheduleFeature/Schedule.swift

+  }
+
+  @ViewBuilder
+  func favoriteIcon(for session: Session) -> some View {


Please remove this method if you have moved.

…tate

ewa1989 added 5 commits March 17, 2024 22:48

add star icon in the right of session row

875b195

change icon according to the session is favorited

33888d9

create new action, view favoriteIconTapped

08a2b0d

change to toggle favorite condition of tapped session

c32c5bf

change to save and load conferences in UserDefaullts

cf49338

ewa1989 mentioned this pull request Mar 18, 2024

Check your favorite sessions feature #40

Open

Merge commit '1b5005da964abcaf5a5865b68347ab32d6e72aa2' into add-chec…

5a32df4

…k-favorite-session-feature # Conflicts: # MyLibrary/Sources/ScheduleFeature/Schedule.swift

d-date changed the title ~~Add check favorite session feature in ShceduleView~~ Check favorite sessions in Schedule View Mar 19, 2024

d-date reviewed Mar 19, 2024

View reviewed changes

ewa1989 added 7 commits March 20, 2024 17:34

change save data type as [String: [Session]] for performance

846098f

extract favorites load/save methods to FileClient, separating with fe…

23983b5

…tching methods which might change API Client in future

Merge branch 'main' into add-check-favorite-session-feature

a7a611b

# Conflicts: # MyLibrary/Sources/ScheduleFeature/Schedule.swift

change file save/load area to Document that is more appropriate becau…

0e8b403

…se favorite data aren't customizing app's behavior

fix failed test due to adding load Favorites when onAppear

daa7fa7

separate saving favorites and updating state for testability

1a5419a

add tests for adding/removing favorites

35b4d14

extract json encoder and decorder as JSONHandler

f2622b8

d-date reviewed Mar 21, 2024

View reviewed changes

ewa1989 added 4 commits March 21, 2024 14:29

Revert "extract json encoder and decorder as JSONHandler"

b648df2

This reverts commit f2622b8.

separate FileClient

a929586

move mocks that are intended for specific behavior near related tests

4e279e4

Merge branch 'main' into add-check-favorite-session-feature

1db51b5

# Conflicts: # MyLibrary/Package.swift # MyLibrary/Sources/ScheduleFeature/Schedule.swift

d-date reviewed Mar 26, 2024

View reviewed changes

ewa1989 added 2 commits March 31, 2024 23:09

remove unused mock

d3a49ab

fix to separate load favorites from fetch data, and to assert separat…

04cefc0

…ely in tests.

ewa1989 added 2 commits March 31, 2024 23:10

to keep models having no logics, move logics in Favorites to Reducer …

d3b9901

…and View, and remove Favorites and use typealias instead because Favorites has one property only.

rename saveDataToFile and loadDataFromFile to explain exactly how the…

cfb2fa3

…y do.

change variable name of Conference, day to conference, as other place…

77b7a09

… does

d-date reviewed Apr 11, 2024

View reviewed changes

ewa1989 added 2 commits April 11, 2024 22:16

fix to combine load response into fetch response for simplicity.

0e3b9f8

move a part of logic to Reducer for collecting logics relative with s…

6cca9bd

…tate

	static func saveDataToFile(_ data : Data, named fileName: String) {
	static func deserialize(data : Data, into fileName: String) {

	static func loadDataFromFile(named fileName: String) -> Data? {
	static func serialize(from filePath: String) -> Data? {

Check favorite sessions in Schedule View #41

Are you sure you want to change the base?

Check favorite sessions in Schedule View #41

Conversation

ewa1989 commented Mar 18, 2024 • edited Loading

Note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewa1989 Mar 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-date commented Mar 19, 2024 • edited Loading

d-date commented Mar 20, 2024

ewa1989 commented Mar 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewa1989 Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-date left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-date commented Mar 26, 2024

ewa1989 commented Mar 26, 2024

ewa1989 commented Mar 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewa1989 commented Mar 18, 2024 •

edited

Loading

ewa1989 Mar 20, 2024 •

edited

Loading

d-date commented Mar 19, 2024 •

edited

Loading

ewa1989 Mar 21, 2024 •

edited

Loading