Skip to content

fix:#681 Added cancel buttons on Edit Group page #702

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.

Sign up for GitHub

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

Open
tonyb650 wants to merge 1 commit into
base: develop
Choose a base branch
from
+113 −33
Open

fix:#681 Added cancel buttons on Edit Group page #702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 88 additions & 25 deletions app/gift-exchanges/[id]/edit/page.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,99 @@
import { Calendar } from '@/components/Calendar/calendar';
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

Missing copyright header. Test files in this codebase should include:

// Copyright (c) Gridiron Survivor.
// Licensed under the MIT License.

This can be seen in other test files like app/create-group-page/page.test.tsx.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator

@shashilo shashilo Dec 2, 2025

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

import { render, screen } from '@testing-library/react';
import EditGroupPage from './page';

describe('Calendar component in create group page', () => {
const currentDate = new Date('2025-10-15T00:00:00Z');
jest.mock('next/navigation', () => ({
useRouter: () => ({
push: jest.fn(),
refresh: jest.fn(),
}),
useParams: () => ({
id: 'mock-group-id',
}),
}));

beforeEach(() => {
jest.useFakeTimers();
jest.setSystemTime(currentDate);
class MockResizeObserver {
observe() {}
unobserve() {}
disconnect() {}
}

global.ResizeObserver = MockResizeObserver;
global.fetch = jest
.fn()
.mockResolvedValue({
ok: true,
json: () => Promise.resolve({ owner_id: '1', name: '' }),
});
Comment on lines +5 to +27
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Nov 25, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Add Supabase auth mock and complete the fetch mock data.

The component uses supabase.auth.getUser() to verify ownership (lines 114-119 in page.tsx), but there's no mock for the Supabase client. Additionally, the fetch mock returns incomplete data that doesn't match the form's expected structure.

Add the following mocks:

+jest.mock('@/lib/supabase/client', () => ({
+  createClient: () => ({
+    auth: {
+      getUser: jest.fn().mockResolvedValue({
+        data: { user: { id: '1' } },
+        error: null,
+      }),
+    },
+  }),
+}));
+
 global.ResizeObserver = MockResizeObserver;
 global.fetch = jest
   .fn()
   .mockResolvedValue({
     ok: true,
-    json: () => Promise.resolve({ owner_id: '1', name: '' }),
+    json: () => Promise.resolve({
+      id: 'mock-group-id',
+      owner_id: '1',
+      name: 'Test Group',
+      description: 'Test Description',
+      drawing_date: '2025-12-20',
+      exchange_date: '2025-12-25',
+      budget: '10-20',
+      group_image: '/images/group-1.png',
+    }),
   });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
jest.mock('next/navigation', () => ({
useRouter: () => ({
push: jest.fn(),
refresh: jest.fn(),
}),
useParams: () => ({
id: 'mock-group-id',
}),
}));
beforeEach(() => {
jest.useFakeTimers();
jest.setSystemTime(currentDate);
class MockResizeObserver {
observe() {}
unobserve() {}
disconnect() {}
}
global.ResizeObserver = MockResizeObserver;
global.fetch = jest
.fn()
.mockResolvedValue({
ok: true,
json: () => Promise.resolve({ owner_id: '1', name: '' }),
});
jest.mock('next/navigation', () => ({
useRouter: () => ({
push: jest.fn(),
refresh: jest.fn(),
}),
useParams: () => ({
id: 'mock-group-id',
}),
}));
jest.mock('@/lib/supabase/client', () => ({
createClient: () => ({
auth: {
getUser: jest.fn().mockResolvedValue({
data: { user: { id: '1' } },
error: null,
}),
},
}),
}));
class MockResizeObserver {
observe() {}
unobserve() {}
disconnect() {}
}
global.ResizeObserver = MockResizeObserver;
global.fetch = jest
.fn()
.mockResolvedValue({
ok: true,
json: () => Promise.resolve({
id: 'mock-group-id',
owner_id: '1',
name: 'Test Group',
description: 'Test Description',
drawing_date: '2025-12-20',
exchange_date: '2025-12-25',
budget: '10-20',
group_image: '/images/group-1.png',
}),
});


describe('Edit Group Page', () => {
beforeAll(() => {
global.ResizeObserver = MockResizeObserver;
global.fetch = jest
.fn()
.mockResolvedValue({
ok: true,
json: () => Promise.resolve({ owner_id: '1', name: '' }),
});
});

afterEach(() => {
jest.useRealTimers();
jest.clearAllMocks();
});

describe('Close (X) button in edit group page', () => {
it('renders the X button with the correct href', () => {
render(<EditGroupPage />);

expect(screen.getByTestId('x-button')).toHaveAttribute(
'href',
'/dashboard',
);
});
});

it('disables past dates correctly', () => {
render(
<Calendar
mode="single"
selected={currentDate}
onSelect={() => {}}
disabled={[{ before: currentDate }]}
initialFocus
/>,
);

const pastDate = screen.getByText('13');
expect(pastDate).toBeDisabled();

const today = screen.getByText('15');
expect(today).not.toBeDisabled();

const tomorrow = screen.getByText('16');
expect(tomorrow).not.toBeDisabled();
describe('Cancel button in edit group page', () => {
it('renders the Cancel button with the correct href', () => {
render(<EditGroupPage />);

expect(screen.getByRole('link', { name: /cancel/i })).toHaveAttribute(
'href',
'/dashboard',
);
});
});

describe('Calendar component in edit group page', () => {
const currentDate = new Date('2025-10-15T00:00:00Z');

beforeEach(() => {
jest.useFakeTimers();
jest.setSystemTime(currentDate);
});

afterEach(() => {
jest.useRealTimers();
});

it('disables past dates correctly', () => {
render(
<Calendar
mode="single"
selected={currentDate}
onSelect={() => {}}
disabled={[{ before: currentDate }]}
initialFocus
/>,
);

const pastDate = screen.getByText('13');
expect(pastDate).toBeDisabled();

const today = screen.getByText('15');
expect(today).not.toBeDisabled();

const tomorrow = screen.getByText('16');
expect(tomorrow).not.toBeDisabled();
});
});
});
33 changes: 25 additions & 8 deletions app/gift-exchanges/[id]/edit/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
import { Check, ChevronsUpDown, ChevronLeft } from 'lucide-react';
import { Input } from '@/components/Input/Input';
import { Button } from '@/components/Button/button';
import { CalendarIcon } from 'lucide-react';
import { CalendarIcon, X } from 'lucide-react';
Comment on lines 16 to +19
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

The CalendarIcon and X imports from lucide-react on line 19 should be combined with the other lucide-react imports on line 16.

Currently:

import { Check, ChevronsUpDown, ChevronLeft } from 'lucide-react';
...
import { CalendarIcon, X } from 'lucide-react';

Should be:

import { CalendarIcon, Check, ChevronsUpDown, ChevronLeft, X } from 'lucide-react';

This follows the pattern used in app/create-group-page/page.tsx line 16.

Copilot uses AI. Check for mistakes.
import { Calendar } from '@/components/Calendar/calendar';
import {
Popover,
Expand Down Expand Up @@ -74,8 +74,8 @@ const formSchema = z
group_image: z
.string()
.refine((val) => groupImageUrls.includes(val), {
message: 'Group Theme Image must be selected',
}),
message: 'Group Theme Image must be selected',
}),
})
.refine((data) => data.exchange_date > data.drawing_date, {
message: 'Exchange Date must be after the Drawing Date',
Expand Down Expand Up @@ -192,9 +192,19 @@ export default function EditGroupPage() {
</div>
<div className="flex items-center justify-center h-full">
<div className="bg-white w-full xl:w-1/2 mb-5 flex justify-center align-center rounded flex-col ">
<h2 className="font-bold m-5">Edit Secret Santa Page</h2>
<div className="flex justify-end m-1.5">
<Button variant="ghost" asChild data-testid="x-button">
<Link href="/dashboard">
<X className="text-black" />
</Link>
</Button>
Comment on lines +196 to +200
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

The X button lacks an accessible label for screen readers. Users relying on assistive technologies won't know what this button does.

Add an aria-label to the Button component:

<Button variant="ghost" asChild data-testid="x-button" aria-label="Close and return to dashboard">
  <Link href="/dashboard">
    <X className="text-black" />
  </Link>
</Button>

The same issue should be addressed in the Create Group page if not already fixed.

Copilot uses AI. Check for mistakes.
</div>
<h2 className="font-bold mx-5 mb-5">Edit Secret Santa Page</h2>
<Form {...form}>
<form onSubmit={form.handleSubmit(onSubmit)} className="space-y-8">
<form
onSubmit={form.handleSubmit(onSubmit)}
className="space-y-8 mb-6 md:mb-4"
>
<FormField
control={form.control}
name="name"
Expand Down Expand Up @@ -400,14 +410,21 @@ export default function EditGroupPage() {
</FormItem>
)}
/>
<div className="flex md:justify-start justify-center md:m-5 m-0 w-full">
<Button className="m-2" type="submit">
<div className="sm:flex sm:justify-center md:justify-end sm:gap-3 mx-3 space-x-2 space-y-2 sm:space-x-0 sm:space-y-0">
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

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

The container div uses conflicting spacing utilities: space-x-2 space-y-2 are applied at the base level but then overridden with sm:space-x-0 sm:space-y-0 on small screens. This creates inconsistent spacing behavior.

On screens smaller than sm breakpoint, both space-x-2 and space-y-2 are applied, which will add spacing in both directions. This can cause layout issues since the buttons will stack vertically but also have horizontal spacing.

Consider using a consistent approach like the Create Group page (flex gap-2 justify-center md:justify-end p-3) or conditionally applying spacing based on flex direction.

Suggested change
<div className="sm:flex sm:justify-center md:justify-end sm:gap-3 mx-3 space-x-2 space-y-2 sm:space-x-0 sm:space-y-0">
<div className="flex flex-col sm:flex-row gap-2 justify-center md:justify-end p-3">

Copilot uses AI. Check for mistakes.
<Button
variant="secondary"
className="bg-slate-300 whitespace-nowrap"
asChild
>
<Link href="/dashboard">Cancel</Link>
</Button>
<Button className="whitespace-nowrap" type="submit">
Save Changes
</Button>
<Button
variant="outline"
type="button"
className="border border-red-600 text-red-600 hover:bg-red-600 hover:text-white m-2"
className="border border-red-600 text-red-600 hover:bg-red-600 hover:text-white whitespace-nowrap"
onClick={deleteGiftExchange}
>
Delete Group
Expand Down