Trial Accounts #3710
Trial Accounts #3710
Conversation
WalkthroughThis PR introduces a comprehensive trial user experience with three primary features: a TrialVideoModal that displays onboarding content for first-time trial users, a TrialRegistration component implementing a two-step signup flow with OTP verification and account allocation, and a TrialExpiryBanner that displays expiration warnings when 7 days or fewer remain. Supporting changes include updates to the Auth component to support trial registration mode with external loading state, modifications to AuthService to persist and retrieve trial-related session data, new API endpoint constants, routing for the trial registration page, CSP updates for Canva iframe embedding, and extensive CSS modules and test coverage for all new components. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚀 Deployed on https://deploy-preview-3710--glific-frontend.netlify.app |
Glific
|
||||||||||||||||||||||||||||
| Project |
Glific
|
| Branch Review |
trial-account
|
| Run status |
|
| Run duration | 27m 22s |
| Commit |
|
| Committer | Amisha Bisht |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
2
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
191
|
| View all changes introduced in this branch ↗︎ | |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3710 +/- ##
==========================================
- Coverage 82.84% 82.82% -0.02%
==========================================
Files 317 320 +3
Lines 11854 11994 +140
Branches 2486 2524 +38
==========================================
+ Hits 9820 9934 +114
- Misses 1280 1300 +20
- Partials 754 760 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/App.tsx (1)
13-53: MovesessionDatalookup into the component to avoid stale auth state.
sessionDatais computed once at module load (Line 14). If a user logs in after initial load,sessionDatastaysnull, soTrialVideoModalwon’t render even thoughisAuthenticatedbecomes true. This effectively breaks the feature for normal login flow.🐛 Proposed fix
-import TrialVideoModal from 'components/UI/TrialVideoModal/TrialVideoModal'; -const sessionData = getAuthSession('session'); +import TrialVideoModal from 'components/UI/TrialVideoModal/TrialVideoModal'; const App = () => { const navigate = useNavigate(); const [drawerOpen, setDrawerOpen] = useState(true); - const isAuthenticated = !!getAuthSession('access_token'); + const isAuthenticated = !!getAuthSession('access_token'); + const sessionData = getAuthSession('session');
🤖 Fix all issues with AI agents
In `@src/components/UI/TrialVideoModal/TrialVideoModal.tsx`:
- Around line 35-44: The Dialog in the TrialVideoModal component sets
aria-describedby="trial-welcome-description" but no matching element exists;
either remove the aria-describedby prop from the Dialog or add a description
element (e.g., a visually-hidden <div> or <p>) with
id="trial-welcome-description" inside the TrialVideoModal so the reference
points to an actual DOM node (update the JSX near the Dialog return where
showModal, handleClose and styles.dialog are used).
In `@src/containers/Organization/TrialRegistration/TrialRegistration.tsx`:
- Around line 117-124: The handleResendOTP function currently accepts a captcha
token parameter but never uses it; update handleResendOTP to forward the token
to sendOTP (e.g., call sendOTP(formValues, token)) and update the sendOTP
function signature/implementation to accept the token and include it in the API
payload used to resend the OTP. If captcha validation is not required for
resends, remove the token parameter from handleResendOTP and the Captcha
integration to avoid dead params. Ensure you update all references to sendOTP
and handleResendOTP to match the new signature.
In `@src/containers/TrialBanner/TrialExpiryBanner.test.tsx`:
- Around line 8-25: The test saves originalWindowOpen and originalLocationHref
but replaces the entire window.location object, which can break restoration;
capture the original window.location descriptor (e.g., via
Object.getOwnPropertyDescriptor) before modifying in beforeEach, then in
afterEach restore window.open from originalWindowOpen and restore
window.location using the saved descriptor (instead of only setting href) so the
original behavior is fully reinstated; update beforeEach/afterEach around
mockWindowOpen, originalWindowOpen, originalLocationHref and the location
replacement to use descriptor save/restore.
🧹 Nitpick comments (11)
src/components/UI/Layout/Layout.tsx (1)
17-21: Consider memoizingtrialSessionDatato avoid unnecessary re-renders.The
trialSessionDataobject is recreated on every render with a new reference, which may cause unnecessary re-renders inTrialVideoModalif it relies on reference equality for props or uses the object in effect dependencies.Additionally,
getAuthSessionparses localStorage JSON each time it's called. Consolidating into a single parse would be slightly more efficient.♻️ Suggested improvement using useMemo
-import { useContext } from 'react'; +import { useContext, useMemo } from 'react'; import MenuIcon from '@mui/icons-material/Menu'; import { SideDrawerContext } from 'context/session'; import GlificLogo from 'assets/images/logo/Logo.svg'; import { getAuthSession } from 'services/AuthService'; import TrialVideoModal from 'components/UI/TrialVideoModal/TrialVideoModal'; import { SideDrawer } from './Navigation/SideDrawer/SideDrawer'; import styles from './Layout.module.css'; export const Layout = ({ children }: LayoutProps) => { const { setDrawerOpen } = useContext(SideDrawerContext); - const trialSessionData = { - last_login_time: getAuthSession('last_login_time'), - is_trial: getAuthSession('is_trial'), - trial_expiration_date: getAuthSession('trial_expiration_date'), - }; + const trialSessionData = useMemo(() => { + const session = getAuthSession('session'); + const parsed = session ? JSON.parse(session) : {}; + return { + last_login_time: parsed.last_login_time, + is_trial: parsed.is_trial, + trial_expiration_date: parsed.trial_expiration_date, + }; + }, []);src/containers/Organization/TrialRegistration/TrialRegistration.test.tsx (1)
1-246: Refactor to useuserEventto align with established codebase patterns.The
@testing-library/user-eventdependency is already available (^14.6.1) and widely used across the codebase. Similar auth test files (Registration.test.tsx, Login.test.tsx) already useuserEventwith theuserEvent.setup()pattern, which simulates real user interactions more accurately thanfireEvent.Replace
fireEvent.change()calls withuserEvent.type()andfireEvent.click()withuserEvent.click(). This will reduce timing edge cases and keep the test suite consistent with the rest of the project.♻️ Example adjustment
-import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { render, screen, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; ... - fireEvent.change(screen.getByPlaceholderText('Organization Name'), { - target: { value: 'Testing Org' }, - }); + const user = userEvent.setup(); + await user.type(screen.getByPlaceholderText('Organization Name'), 'Testing Org');src/components/UI/TrialVideoModal/TrialVideoModal.tsx (1)
54-61: Consider extracting hardcoded Canva URL to configuration.The iframe source URL is hardcoded. If this video needs to be updated or differs per environment, consider moving it to a config constant.
src/containers/Organization/TrialRegistration/TrialRegistration.tsx (3)
181-181: Avoid usingArray<any>type.The
formFieldsarray is typed asArray<any>, losing type safety. Consider defining a proper interface for form field configuration.♻️ Proposed type definition
interface FormFieldConfig { component: React.ComponentType<any>; name: keyof TrialFormValues; type: string; placeholder: string; styles?: string; disabled?: boolean; autoComplete?: string; helperText?: string; endAdornment?: React.ReactNode; } const formFields: FormFieldConfig[] = [ // ... ];
76-101: Inconsistent API response handling pattern.The success check at line 79 (
data.data?.message || (!data.error && data.data)) differs from the check at line 149 (data.success && data.data?.login_url). Consider aligning response handling or documenting the expected response structures for each endpoint.
52-65: Dynamic validation schema may cause unnecessary re-renders.The
FormSchemais recreated on every render due to theotpSentdependency in the OTP validation rule. Consider memoizing the schema or restructuring to avoid recreation.♻️ Proposed optimization
import { useMemo } from 'react'; // Inside component: const FormSchema = useMemo(() => Yup.object().shape({ organizationName: Yup.string() .required('Organization name is required') .matches(/^[A-Za-z\s]+$/, 'Organization name can only contain alphabets and spaces') .min(5, 'Organization must be at least 5 characters'), // ... other fields otp: otpSent ? Yup.string().required('OTP is required') : Yup.string(), }), [otpSent, t]);src/components/UI/TrialVideoModal/TrialVideoModal.test.tsx (1)
5-9: Consider sharing theSessionDatainterface.The
SessionDatainterface is duplicated fromTrialVideoModal.tsx. Consider exporting it from the component file or a shared types module to maintain a single source of truth.src/containers/TrialBanner/TrialExpiryBanner.module.css (1)
112-119: Consider adding text truncation fallback for very narrow containers.The
BannerSubtextusestext-overflow: ellipsiswhich is good, but if the parent container has unusual sizing, the text might not truncate as expected. This is a minor consideration given the banner is typically in a sidebar.src/containers/Auth/Auth.module.css (1)
361-406: Consider MUI v7 recommended patterns for input styling.While
.MuiInputBase-rootand.MuiOutlinedInput-rootare stable documented class names, MUI v7 officially recommends using the slots/slotProps API, thesxprop, or theme component overrides instead of direct CSS class selectors for long-term maintainability and consistency with the standardized customization pattern.src/containers/TrialBanner/TrialExpiryBanner.tsx (2)
33-39: Consider extracting hardcoded URLs to constants.The support email and calendar URL are hardcoded. For maintainability, consider moving these to a constants file or configuration, especially if they might be reused or need environment-specific values.
Example extraction
// In common/constants.ts or similar export const TRIAL_SUPPORT_EMAIL = 'connect@glific.org'; export const TRIAL_SCHEDULE_CALL_URL = 'https://calendar.app.google/USJMfhSsDvW5yS6B7';
97-114: Consider using the custom Button component for consistency.The action buttons use MUI's
Buttondirectly, while other parts of the codebase use the customButtonwrapper fromcomponents/UI/Form/Button/Button. For consistency across the application, consider using the custom component.Suggested change
-import { Button, Collapse, IconButton } from '@mui/material'; +import { Collapse, IconButton } from '@mui/material'; +import { Button } from 'components/UI/Form/Button/Button';
| return ( | ||
| <Dialog | ||
| open={showModal} | ||
| onClose={handleClose} | ||
| maxWidth="md" | ||
| fullWidth | ||
| className={styles.dialog} | ||
| aria-labelledby="trial-welcome-title" | ||
| aria-describedby="trial-welcome-description" | ||
| > |
There was a problem hiding this comment.
Missing element for aria-describedby reference.
The Dialog specifies aria-describedby="trial-welcome-description" but no element with id="trial-welcome-description" exists in the component. Either remove the attribute or add a description element.
🔧 Proposed fix
<Dialog
open={showModal}
onClose={handleClose}
maxWidth="md"
fullWidth
className={styles.dialog}
aria-labelledby="trial-welcome-title"
- aria-describedby="trial-welcome-description"
>
+ <DialogTitle id="trial-welcome-title" className={styles.visuallyHidden}>
+ Welcome to Glific Trial
+ </DialogTitle>
<IconButton aria-label="Close trial welcome video" onClick={handleClose} className={styles.closeButton}>Alternatively, add a visually hidden description element with id="trial-welcome-description".
📝 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.
| return ( | |
| <Dialog | |
| open={showModal} | |
| onClose={handleClose} | |
| maxWidth="md" | |
| fullWidth | |
| className={styles.dialog} | |
| aria-labelledby="trial-welcome-title" | |
| aria-describedby="trial-welcome-description" | |
| > | |
| return ( | |
| <Dialog | |
| open={showModal} | |
| onClose={handleClose} | |
| maxWidth="md" | |
| fullWidth | |
| className={styles.dialog} | |
| aria-labelledby="trial-welcome-title" | |
| > | |
| <DialogTitle id="trial-welcome-title" className={styles.visuallyHidden}> | |
| Welcome to Glific Trial | |
| </DialogTitle> |
🤖 Prompt for AI Agents
In `@src/components/UI/TrialVideoModal/TrialVideoModal.tsx` around lines 35 - 44,
The Dialog in the TrialVideoModal component sets
aria-describedby="trial-welcome-description" but no matching element exists;
either remove the aria-describedby prop from the Dialog or add a description
element (e.g., a visually-hidden <div> or <p>) with
id="trial-welcome-description" inside the TrialVideoModal so the reference
points to an actual DOM node (update the JSX near the Dialog return where
showModal, handleClose and styles.dialog are used).
| const handleResendOTP = async (token: string) => { | ||
| if (!formValues) { | ||
| return; | ||
| } | ||
|
|
||
| setAuthError(''); | ||
| await sendOTP(formValues); | ||
| }; |
There was a problem hiding this comment.
Unused captcha token in handleResendOTP.
The token parameter received from the Captcha component is never used in the sendOTP call. If captcha verification is required for resending OTP, the token should be included in the API payload. Otherwise, the captcha serves no protective purpose.
🔧 Proposed fix if captcha should be validated
- const handleResendOTP = async (token: string) => {
+ const handleResendOTP = async (captchaToken: string | null) => {
if (!formValues) {
return;
}
+ if (!captchaToken) {
+ setAuthError('Captcha verification failed. Please try again.');
+ return;
+ }
setAuthError('');
- await sendOTP(formValues);
+ await sendOTP(formValues, captchaToken);
};Then update sendOTP to accept and include the token in the payload.
🤖 Prompt for AI Agents
In `@src/containers/Organization/TrialRegistration/TrialRegistration.tsx` around
lines 117 - 124, The handleResendOTP function currently accepts a captcha token
parameter but never uses it; update handleResendOTP to forward the token to
sendOTP (e.g., call sendOTP(formValues, token)) and update the sendOTP function
signature/implementation to accept the token and include it in the API payload
used to resend the OTP. If captcha validation is not required for resends,
remove the token parameter from handleResendOTP and the Captcha integration to
avoid dead params. Ensure you update all references to sendOTP and
handleResendOTP to match the new signature.
| const originalWindowOpen = window.open; | ||
| const originalLocationHref = window.location.href; | ||
|
|
||
| let mockWindowOpen: typeof window.open; | ||
|
|
||
| beforeEach(() => { | ||
| mockWindowOpen = vi.fn(); | ||
| window.open = mockWindowOpen; | ||
|
|
||
| delete (window as unknown as { location: unknown }).location; | ||
| (window as unknown as { location: { href: string } }).location = { href: '' }; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| window.open = originalWindowOpen; | ||
| (window as unknown as { location: { href: string } }).location = { href: originalLocationHref }; | ||
| vi.clearAllMocks(); | ||
| }); |
There was a problem hiding this comment.
Window location restoration may not work correctly.
The originalLocationHref captures the initial href string value, but in afterEach you're assigning it back after replacing the entire location object. This may not properly restore the original location behavior in subsequent tests.
🔧 Suggested improvement
- const originalWindowOpen = window.open;
- const originalLocationHref = window.location.href;
-
- let mockWindowOpen: typeof window.open;
+ let mockWindowOpen: ReturnType<typeof vi.fn>;
+ let originalLocation: Location;
beforeEach(() => {
mockWindowOpen = vi.fn();
window.open = mockWindowOpen;
-
- delete (window as unknown as { location: unknown }).location;
- (window as unknown as { location: { href: string } }).location = { href: '' };
+ originalLocation = window.location;
+ Object.defineProperty(window, 'location', {
+ value: { href: '' },
+ writable: true,
+ });
});
afterEach(() => {
- window.open = originalWindowOpen;
- (window as unknown as { location: { href: string } }).location = { href: originalLocationHref };
+ window.open = originalLocation.constructor.prototype.open;
+ Object.defineProperty(window, 'location', {
+ value: originalLocation,
+ writable: true,
+ });
vi.clearAllMocks();
});🤖 Prompt for AI Agents
In `@src/containers/TrialBanner/TrialExpiryBanner.test.tsx` around lines 8 - 25,
The test saves originalWindowOpen and originalLocationHref but replaces the
entire window.location object, which can break restoration; capture the original
window.location descriptor (e.g., via Object.getOwnPropertyDescriptor) before
modifying in beforeEach, then in afterEach restore window.open from
originalWindowOpen and restore window.location using the saved descriptor
(instead of only setting href) so the original behavior is fully reinstated;
update beforeEach/afterEach around mockWindowOpen, originalWindowOpen,
originalLocationHref and the location replacement to use descriptor
save/restore.
Summary
Test Plan
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.