Refactor other entity API functions and hooks to use new pattern #74

jhodapp · 2025-03-08T03:28:06Z

Description

This PR refactors all of the other entity API functions and hooks to use new pattern established with Organization.

Note: this is a big PR but I'm going to leave some comments showing where to really hone your review with much of the rest of the changes just being quite repetitive and uninteresting.

Note 2: this PR requires this small backend PR to function correctly for logout.

GitHub Issue: None

Changes

Refactor the coaching relationship functions to use new hook/API pattern
Refactor user session login/logout functions to use new hook/API pattern
Refactor the coaching session functions to use new hook/API pattern
Refactor the action functions to use new hook/API pattern
Refactor the agreement functions to use new hook/API pattern
Refactor the overarching goal functions to use new hook/API pattern
Update entity-api to include an optional response data parsing/transformation function parameter to useEntityList() and listFn()

Screenshots / Videos Showing UI Changes (if applicable)

Testing Strategy

Run through the entire application to ensure expected behavior, including…
Login/Logout
Select organization, relationship and join a coaching session
Create a new coaching session
Create, update and delete actions and agreements on the coaching session page
Set an overarching goal
Update an overarching goal
Change coaching sessions from the selector box on the coaching session page

Concerns

None

jhodapp · 2025-03-15T18:59:28Z

src/lib/api/entity-api.ts

This is an important file to check over since it's now used by all entities.

jhodapp · 2025-03-15T19:01:06Z

src/lib/api/actions.ts

Repetitious but still important to check over the use of the new entity-api hook/Api pattern.

jhodapp · 2025-03-15T19:01:15Z

src/lib/api/agreements.ts

Repetitious but still important to check over the use of the new entity-api hook/Api pattern.

jhodapp · 2025-03-15T19:01:23Z

src/lib/api/coaching-relationships.ts

Repetitious but still important to check over the use of the new entity-api hook/Api pattern.

jhodapp · 2025-03-15T19:01:30Z

src/lib/api/coaching-sessions.ts

Repetitious but still important to check over the use of the new entity-api hook/Api pattern.

jhodapp · 2025-03-15T19:03:02Z

src/lib/api/organizations.ts

      organization
    ),

+  createNested: async (id: Id, entity: Organization): Promise<Organization> => {


Thoughts on having the same API for all entities, but some of them currently don't (or never will make sense to) implement certain functions? This is about having a standard and predictable set of entity verbs.

Yeah - because it's a means to achieve standardization this seems fine.

Left a comment above around making it optional but I agree with this as well. Either works for me.

jhodapp · 2025-03-15T19:03:19Z

src/lib/api/overarching-goals.ts

Repetitious but still important to check over the use of the new entity-api hook/Api pattern.

jhodapp · 2025-03-15T19:03:44Z

src/lib/api/user-sessions.ts

Repetitious but still important to check over the use of the new entity-api hook/Api pattern.

jhodapp · 2025-03-15T19:05:02Z

src/types/general.ts

+// Type-safe transformation function with runtime validation that ensures
+// that raw ISO date time stamps are transformed into valid ts-luxon DateTime
+// instances.
+export const transformEntityDates = (data: any): any => {


This transformation function is new and is used by some of the entity *List hooks/API functions. Since this is new, definitely worth checking over to see if it's easy to use and implemented well.

jhodapp · 2025-03-15T19:06:30Z

src/components/ui/coaching-session-selector.tsx

        const sessionIds = coachingSessions?.map((session) => session.id) || [];
        const goalsPromises = sessionIds.map((id) =>
-          fetchOverarchingGoalsByCoachingSessionId(id)
+          OverarchingGoalApi.list(id)


This component and others should ideally be updated to use hooks instead of the direct function calls, but I decided it was out of the scope of this PR to refactor to this level since it would be a pretty major component code restructuring change.

The main reason being, this will give us automatic updates of list of entities when they're changed locally by the current user without having to refresh the page. For example, in the CoachingSessionList component.

…pattern. Also add a new entity-api method, createNested which creates a new entity nested under the ID of another entity.

…ormation function parameter to useEntityList() and listFn().

…mAgreement function

…ransformAgreement() function into general.ts as a generic, reusable function called transformEntityDates()

…d use it everywhere in the entity's Api

…ern.

… type since it's no longer used

…-auth-error-handling

…ndling Set error state with message rather than with error object

calebbourg

@jhodapp this is super cool and cleans things up a lot! Thank you for putting it together <3

I left a few thoughts and questions.
The main thing I'd just like to talk through is making config argument required vs returning an empty Array

src/components/ui/coaching-sessions/coaching-notes.tsx

src/components/ui/coaching-sessions/overarching-goal-container.tsx

src/lib/api/actions.ts

calebbourg · 2025-03-19T12:00:33Z

src/lib/api/agreements.ts

+  create: async (agreement: Agreement): Promise<Agreement> =>
+    EntityApi.createFn<Agreement, Agreement>(AGREEMENTS_BASEURL, agreement),
+
+  createNested: async (_id: Id, _entity: Agreement): Promise<Agreement> => {


I'm curious what these might be used for.

calebbourg · 2025-03-19T12:07:09Z

src/lib/api/entity-api.ts

+    transform?: (data: T) => U
+  ): Promise<U> => {
+    if (!config) {
+      return [] as unknown as U; // Return empty array if config is null


Would it make sense to make the config argument required? I'm worried that this might start returning [] because of this and we think it's an issue on the BE and spend an unneeded amount of time troubleshooting before finding this line of code.
If we require config would the type system protect us here? I'm not 100% on how typescript works in that situation. If it's at compilation we should be protected I would think.

An alternative would be to log something to the console here so we would at least be able to get a hint at what happened.

+1 to Caleb's idea. Definitely feels more sound to use the type system to enforce/require a config arg than an early return in the function body.

Also a good catch, it was an easy change to make.

calebbourg · 2025-03-19T12:07:44Z

src/lib/api/entity-api.ts

+    });
+
+    const rawData = response.data.data;
+    return transform ? transform(rawData) : (rawData as unknown as U);


I like this option a lot

Me too, it works super well. There if one needs it, otherwise ignore.

src/lib/api/entity-api.ts

calebbourg · 2025-03-19T12:15:14Z

src/lib/api/entity-api.ts

    baseUrl: string,
    api: {
      create: (entity: T) => Promise<T>;
+      createNested: (id: Id, entity: T) => Promise<T>;


One option that I think might exist, though I don't know how to do this in typescript, would be to extract the api object into it's own type or interface and then have createNested be optional.
I don't have a strong opinion on that.

The way useEntityMutation is set up, there's no easy way of not having the same functions across all entity types unfortunately.

calebbourg · 2025-03-19T12:15:50Z

src/lib/api/organizations.ts

      organization
    ),

+  createNested: async (id: Id, entity: Organization): Promise<Organization> => {


Left a comment above around making it optional but I agree with this as well. Either works for me.

…erSession from CoachingNotes' return value

calebbourg

Looks great!

jhodapp added the enhancement Improves existing functionality or feature label Mar 8, 2025

jhodapp added this to the 1.0-beta1 milestone Mar 8, 2025

jhodapp self-assigned this Mar 8, 2025

jhodapp added this to Refactor Coaching Platform Mar 8, 2025

jhodapp moved this to 🏗 In progress in Refactor Coaching Platform Mar 8, 2025

jhodapp changed the title ~~Refactor other entity apis and hooks~~ Refactor other entity apis and hooks to use new pattern Mar 8, 2025

jhodapp changed the title ~~Refactor other entity apis and hooks to use new pattern~~ Refactor other entity API methods and hooks to use new pattern Mar 8, 2025

jhodapp requested review from calebbourg, qafui and zgavin1 March 15, 2025 18:57