Angular v17 Upgrade: Events Feature Refactor

ajaygandecha commented 4 months ago

This major pull request refactors the entire events feature to the new standards of Angular v17. This pull request also rewrites most of the functionality to be more concise and readable, following the best conventions for future students in COMP 423 and 393, using everything we have learned so far.

This refactor also fully utilizes the new frontend pagination abstraction created in #456.

Once again, I am adding @jadekeegan and @atoneyd to this PR as reviewers, since this was the feature we worked on the most! @atoneyd, you can also see the first glimpses of the new age of pagination in our site - thank you for helping to get the first version working, which was the stepping stone that allowed the abstraction to be made!!✨ @jadekeegan, this PR also iterates on event registrations.

Managing State of Paginated Data

Intro

This refactor was the first one that featured pagination as the main form of data retrieval, marking the first usage of the Paginator<T> abstraction introduced in #456. This is a refactor using Angular v17. We can combine the signals functionality with the abstracted paginator to make our code much more readable and performant.

The Event Service

To use our abstraction, the new event service creates an encapsulated event paginator object, as shown below:

https://github.com/unc-csxl/csxl.unc.edu/blob/fcf9e16ced9da94d81ac41cefe3e622456117dbc/frontend/src/app/event/event.service.ts#L34-L36

The TimeRangePaginator is an implementation of the PaginatorAbstraction abstract class which specifically works with paginated data on a time range, where a start and end time are used as parameters.

Loading our new pages is extremely easy and is done in the getEvents() method, shown below:

https://github.com/unc-csxl/csxl.unc.edu/blob/fcf9e16ced9da94d81ac41cefe3e622456117dbc/frontend/src/app/event/event.service.ts#L47-L49

This method takes in an optional params field for the pagination's parameters. In the case that this is not supplied, a default time range parameter constant is used. This saves us a lot of repeated code elsewhere in the feature.

As mentioned in #456, .loadPage() takes in both params and an operator function in the case that the model returned by the API is not of the same type expected. Since our API returns EventJson, we can pass in parseEventJson into this parameter, resulting in paginated data of type Event.

Notice that, unlike the organization refactor ( #455 ), the service does not expose any signals. This is because, unlike with organizations, events does not have a single collection of events shared by every component. We only want to load the events we need for a page (the entire point of pagination), so sharing this data does not really make sense here. Not to mention, pagination parameters are component specific and based on query parameters. So, this management is left up to the components.

The rest of the service methods follow normal conventions and return observables.

The Event Page Component

Pagination truly shines in the event page component, which got the heaviest refactor - this component shrunk nearly in half after the refactor.

First, our component has a reactive page signal that stores the current paginated event data. This signal will update every time a new page is loaded to store the new page, allowing our page to dynamically update.

In addition, we previously needed to use a function to group event data by date. This functionality was moved into a new pipe called GroupEventsPipe. Now, the component also adds a computed signal which, when called (and the page updates), the signal uses the new pipe to expose grouped event data. This is done as shown below:

https://github.com/unc-csxl/csxl.unc.edu/blob/fcf9e16ced9da94d81ac41cefe3e622456117dbc/frontend/src/app/event/event-page/event-page.component.ts#L51-L55

The best part is that we can make our specific pagination parameters as signals. There are three parameters - one for the start date, end date, and any filter query. This is shown below:

https://github.com/unc-csxl/csxl.unc.edu/blob/fcf9e16ced9da94d81ac41cefe3e622456117dbc/frontend/src/app/event/event-page/event-page.component.ts#L56-L62

This functionality is great paired with an effect. The new component has an effect such that, when any of the pagination signals are updated, the page automatically refreshes with new data and the query parameters in the URL is updated:

https://github.com/unc-csxl/csxl.unc.edu/blob/fcf9e16ced9da94d81ac41cefe3e622456117dbc/frontend/src/app/event/event-page/event-page.component.ts#L81-L100

This is great, since our page now updates whenever a query parameter is changed - which is done when the search bar is edited, or when a user presses on the arrows to switch between pages.

The Event Detail Component

Nothing major changed in the event detail component's refactor, although some code has been cleaned up. This component still uses the resolver to load a single event. This is required for the titleResolver to run properly.

The Event Editor Component

The event editor has also been refactored to make the code cleaner and more concise.

In this refactor, the authentication functionality was moved to a new guard. Since this guard uses two different conditions to ensure that the user can access the page, I had to use the combineLatest RxJS method:

https://github.com/unc-csxl/csxl.unc.edu/blob/fcf9e16ced9da94d81ac41cefe3e622456117dbc/frontend/src/app/event/event-editor/event-editor.guard.ts#L45-L50

Notice though that this guard has to call the getEvent() function from the service - which repeats functionality in the event resolver. This means that when we open the editor, two GET API calls are performed. This is really unfortunate, and I tried many different approaches, none of them work.

The problem is that resolvers run before canActivate guards in Angular, so there was no way to use the resolver's event data in the guard. The only other idea I saw online was to perform a reroute in the resolver if the user is not an organizer, however I felt that this overloaded the resolver with functionality that it should not have - thoughts @KrisJordan?

Widgets

All widget HTML code was also updated to the new conventions!

Other Changes

Removal of the Event Admin Page and Gears

Throughout the refactoring process, I found the event admin component and related functionality merged in #402 needs to be reworked - so, I removed the admin page entirely. We will revisit this page once we get to the Angular Material 3 upgrade. No functionality is lost to the user with this removed, and the code becomes much cleaner.

Reversion to a Single Column Format

I have temporarily disabled the dual-column arrangement on the events page, since you recommended this @KrisJordan (@jadekeegan is very sad 😭)

I mainly disabled it because the code became much more concise - even if we add it back, I think the way it was implemented needs reworking.

File Structure Changes

There are not extremely major change to the file structure in the events page, however a new /pipe directory has been added to follow the conventions established in #455.

Future Considerations

There are a few things I noticed in the events frontend that I think could warrant some refinements to our backend. These are noted in #459.
We might be able to try to figure out a better construction to replace the existing titleResolver and the child route added to get it configured.
As @KrisJordan mentioned, it might be better for all admin features to be moved to a separate admin folder rather than individual feature folders. Since services are not declared in modules and are injected from root, feature services would be available to the Admin module. This might allow for our file organization to be cleaner.

ajaygandecha commented 4 months ago

Thank you for the review @KrisJordan! I fixed most of the comments, but I had some additional comments on the rest + some other things that I have noticed.

Yes, the date conversions here were extremely painstaking - in the first iteration of COMP 393, we ended up having to use the GB version to get everything working. We had issues with getting the native date selectors to work, as they reverted back to random / arbitrary times. I think, however, that the solution separating the Json model and the regular model may have solved the date problems we had - so, I will revisit this now and see if I can get things working standardized on ISO format!
For the resolver / guard issue, my first attempt actually was to use route.data - however, the result was undefined. Since resolver data is not loaded before the guard runs, the route's data is not set by the time the guard runs. Now, I think that this problem also kind of ties into the comment you made about profile()!, something I also seems off. In this case too, even if we protect the admin page with a guard, it would still be hard to pass this data directly in. We could have a profile resolver which loads a profile if logged in, or redirects the user if they are not logged in, but it also seems really anti-Angular to me. We could create some sort of guard that is loaded in the app module and then pass it around, but not sure what is best here.
This is not related to the CR comments, but something I wanted to ask about as we did this refactor. There does not yet seem to be a convention marking signal variables like we have $ for observables. In Swift state variables, we also use $, but preceding the name (as in $flashcards). Do you think we need to standardize on some convention to denote signals vs. non-signals? I can see, for example, organizations being more easily confused as a non-signal variable.

ajaygandecha commented 4 months ago

Well, it looks like the conversion to ISO strings was easier than expected!

unc-csxl / csxl.unc.edu