Top events updated

[daniel-buta]

Implemented a dynamic Top Events section on the homepage that displays the 5 most popular events based on ticket sales.

Backend:

• Added /api/events/top endpoint returning top events sorted by ticket count.

Frontend:

• Created responsive card grid displaying event details (image, title, description, location, price, tickets sold).
• Added Buy Ticket and Save Event buttons with authentication checks.
• Integrated with existing saved events functionality.

closes #139

[Copilot]

Pull Request Overview

This PR implements a dynamic "Top Events" section on the homepage that displays the 5 most popular events based on ticket sales, enhancing user engagement by showcasing trending campus events.

Key Changes:

• Added backend /api/events/top endpoint that returns events sorted by ticket count
• Created responsive card grid UI with event cards showing image, title, description, location, price, and tickets sold
• Integrated authentication-aware features for buying tickets and saving events with proper error handling

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.

File	Description
backend/springboot-app/src/main/java/com/linkt/controller/EventController.java	Added new /top GET endpoint to retrieve the 5 most popular events sorted by ticket sales
frontend/my-react-app/src/api/events.api.ts	Added getTopEvents() API function with data transformation to match frontend Event interface
frontend/my-react-app/src/App.tsx	Replaced static placeholder content with dynamic top events display, including loading states, authentication checks, and save/buy functionality

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The /events/top endpoint is not included in the Spring Security configuration. This endpoint should be added to the permitAll() list in SecurityConfig to allow unauthenticated access, similar to other GET event endpoints.

Add /api/events/top to line 62:

.requestMatchers(HttpMethod.GET, "/api/events", "/api/events/{id}", "/api/events/search", "/api/events/filter", "/api/events/top").permitAll()

[RenaudBernier]

Could you please look into this @daniel-buta ? Try accessing the homepage without being logged in and tell me if you can still see the top events please.

[daniel-buta]

Yes, I can see the top events when accessing the homepage without being logged in

[daniel-buta]

You can see the top events without being logged in but when you try to save an event or buy a ticket, it will redirect you to log in.

[Copilot]

Image URL handling is inconsistent with other functions in this file. The getTopEvents function includes logic to check if the URL starts with 'http' or 'https', but other functions like getAllEvents() (line 13) and getEventById() (line 33) simply prepend http://localhost:8080 without this check.

For consistency, either:

1. Apply the same URL handling logic across all functions, or
2. Simplify this to match the existing pattern if all image URLs are expected to be relative paths:

image: event.imageUrl ? [`http://localhost:8080${event.imageUrl}`] : [],

[Copilot]

The N+1 loop pattern here can cause performance issues when checking many events. Each call to checkIfSaved() makes a separate API request, resulting in up to 5 sequential requests when displaying top events.

Consider batching this operation by:

1. Creating a new backend endpoint that accepts multiple event IDs and returns which ones are saved, or
2. Making the API calls in parallel using Promise.all():

const savedIds = new Set<number>();
const results = await Promise.all(
    topEvents.map(event => 
        checkIfSaved(event.eventID).catch(() => false)
    )
);
topEvents.forEach((event, index) => {
    if (results[index]) savedIds.add(event.eventID);
});
setSavedEventIds(savedIds);

Suggested change

	for (const event of topEvents) {
	try {
	const isSaved = await checkIfSaved(event.eventID);
	if (isSaved) {
	savedIds.add(event.eventID);
	}
	} catch (error) {
	// Ignore errors for individual checks
	}
	}
	const results = await Promise.all(
	topEvents.map(event =>
	checkIfSaved(event.eventID).catch(() => false)
	)
	);
	topEvents.forEach((event, index) => {
	if (results[index]) savedIds.add(event.eventID);
	});

[Copilot]

Missing JavaDoc documentation for this new public endpoint. Other endpoints in this controller (e.g., lines 82-85) have documentation describing their purpose and behavior.

Add documentation following the existing pattern:

/**
 * Get the top 5 events sorted by ticket sales
 * Returns a list of events with their basic information and ticket count
 * @return List of top events with ticketsSold field
 */
@GetMapping("/top")

Suggested change

	/**
	* Get the top 5 events sorted by ticket sales.
	* Returns a list of events with their basic information and ticket count.
	* @return List of top events with ticketsSold field
	*/

[Copilot]

Error handling silently fails without providing user feedback. When the API call fails, the user sees no indication that something went wrong, they just see an empty events list (or potentially old data).

Consider adding user-facing error feedback:

} catch (error) {
    console.error('Failed to fetch top events:', error);
    enqueueSnackbar('Failed to load top events', { variant: 'error' });
} finally {

Suggested change

	console.error('Failed to fetch top events:', error);
	console.error('Failed to fetch top events:', error);
	enqueueSnackbar('Failed to load top events', { variant: 'error' });

[Copilot]

[nitpick] The hardcoded limit of 5 events is a magic number that should be extracted as a named constant for better maintainability. If the requirement changes (e.g., to show top 10 events), the constant name makes it clear what needs to be updated.

Consider defining it at the class level:

private static final int TOP_EVENTS_LIMIT = 5;

Then use:

.limit(TOP_EVENTS_LIMIT)

[Copilot]

Potential N+1 query problem: Calling event.getTickets().size() for each event will trigger a separate query per event since tickets is lazily loaded. This could cause significant performance issues when there are many events.

Consider using a custom query with JOIN FETCH or a database-level COUNT to efficiently retrieve ticket counts:

@Query("SELECT e FROM Event e LEFT JOIN FETCH e.tickets")
List<Event> findAllWithTickets();

Or create a DTO projection query that counts tickets at the database level:

@Query("SELECT e.eventId as eventId, e.title as title, ..., COUNT(t) as ticketCount FROM Event e LEFT JOIN e.tickets t GROUP BY e.eventId")
List<EventWithTicketCount> findAllEventsWithTicketCount();

[Copilot]

The endpoint returns all events regardless of their approval status. Since this is a public endpoint (visible on the homepage), it should only display approved events to prevent showing pending or rejected events to users.

Add a filter to only include approved events:

List<Event> allEvents = eventRepository.findAll().stream()
    .filter(event -> "approved".equals(event.getStatus()))
    .collect(java.util.stream.Collectors.toList());

Suggested change

	List<Event> allEvents = eventRepository.findAll();
	List<Event> allEvents = eventRepository.findAll().stream()
	.filter(event -> "approved".equals(event.getStatus()))
	.collect(java.util.stream.Collectors.toList());

[Copilot]

The endpoint returns a List<Map<String, Object>> instead of using a proper DTO class. This inconsistent API design makes the response schema unclear and harder to maintain. Other endpoints in this controller use strongly-typed Event objects.

Consider creating a dedicated DTO (e.g., TopEventDTO) that explicitly defines the response structure:

public class TopEventDTO {
    private Long eventId;
    private String title;
    private String description;
    // ... other fields
    private Integer ticketsSold;
}

This provides type safety, better IDE support, and clearer API documentation.

[DudeNamedDarcy]

Looked great during the meeting today! Thanks for implementing it!

[RenaudBernier]

lgtm