Skip to content
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

refactor: the timer should not start when the user ignores the 'Todayplan' popup #3159

Merged
merged 5 commits into from
Oct 24, 2024
Merged

refactor: the timer should not start when the user ignores the 'Todayplan' popup #3159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7ca6a45
refactor: the timer should not start when the user ignores the 'Today…
CREDO23 Oct 17, 2024
07f0a94
refactor: the timer should not start when the user ignores the 'Today…
CREDO23 Oct 17, 2024
3665230
refactor: enhance the enforce plan mode
CREDO23 Oct 18, 2024
d517bf1
Merge pull request #3161 from ever-co/3155-bug-settings--enforce-plan…
Cedric921 Oct 18, 2024
9746b83
add requested changes
CREDO23 Oct 18, 2024
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
2 changes: 1 addition & 1 deletion apps/web/app/hooks/features/useStartStopTimerHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ export function useStartStopTimerHandler() {
if (timerStatusFetching || !canRunTimer) return;
if (timerStatus?.running) {
stopTimer();
} else if (requirePlan && !isActiveTaskPlaned) {
} else if (requirePlan && hasPlan && !isActiveTaskPlaned) {
openEnforcePlannedTaskModal();
} else {
if (
Expand Down
4 changes: 1 addition & 3 deletions apps/web/app/hooks/features/useTimer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,9 +265,7 @@ export function useTimer() {
user?.isEmailVerified &&
((!!activeTeamTask && activeTeamTask.status !== 'closed') ||
// If timer is running at some other source and user may or may not have selected the task
timerStatusRef.current?.lastLog?.source !== TimerSource.TEAMS) &&
// If team settings require to have a plan to be able track
canTrack;
timerStatusRef.current?.lastLog?.source !== TimerSource.TEAMS)

// Local time status
const {
Expand Down
54 changes: 30 additions & 24 deletions apps/web/lib/components/modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ type Props = {
className?: string;
alignCloseIcon?: boolean;
showCloseIcon?: boolean;
closeOnOutsideClick?: boolean;
} & PropsWithChildren;

export function Modal({
Expand All @@ -27,7 +28,8 @@ export function Modal({
description,
className,
alignCloseIcon,
showCloseIcon = true
showCloseIcon = true,
closeOnOutsideClick = false
}: Props) {
const refDiv = useRef(null);

Expand All @@ -44,36 +46,40 @@ export function Modal({
>
<Dialog
initialFocus={refDiv}
onClose={closeModal}
onClose={closeOnOutsideClick ? closeModal : () => null}
as="div"
className="fixed inset-0 backdrop-brightness-90 backdrop-blur-sm z-[9999] w-full h-full"
>
<div ref={refDiv} className="absolute inset-0 flex items-center justify-center p-4 w-full">
<Dialog.Overlay
className={clsxm('flex justify-center items-center flex-col space-y-1 relative', className)}
>
{title && <Dialog.Title className={clsxm(titleClass)}>{title}</Dialog.Title>}
{description && <Dialog.Description>{description}</Dialog.Description>}
{showCloseIcon && (
<div
onClick={() => {
closeModal();
customCloseModal?.();
}}
className={`absolute ${
alignCloseIcon ? 'right-2 top-3' : 'right-3 top-3'
} md:right-2 md:top-3 cursor-pointer z-50`}
>
<Image
src={'/assets/svg/close.svg'}
alt="close"
width={28}
height={28}
className="w-6 md:w-7"
/>
</div>
)}
{children}
<Dialog.Panel
className={clsxm('flex justify-center items-center flex-col space-y-1 relative', className)}
>
{title && <Dialog.Title className={clsxm(titleClass)}>{title}</Dialog.Title>}
{description && <Dialog.Description>{description}</Dialog.Description>}
{showCloseIcon && (
<div
onClick={() => {
closeModal();
customCloseModal?.();
}}
className={`absolute ${
alignCloseIcon ? 'right-2 top-3' : 'right-3 top-3'
} md:right-2 md:top-3 cursor-pointer z-50`}
>
<Image
src={'/assets/svg/close.svg'}
alt="close"
width={28}
height={28}
className="w-6 md:w-7"
/>
</div>
)}
{children}
</Dialog.Panel>
</Dialog.Overlay>
</div>
</Dialog>
Expand Down
4 changes: 4 additions & 0 deletions apps/web/lib/components/switch.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { Switch } from '@headlessui/react';
import { useCallback, useEffect, useState } from 'react';
import { Text } from './typography';
import { useTranslations } from 'next-intl';
import { DAILY_PLAN_SUGGESTION_MODAL_DATE } from '@app/constants';

export default function TimeTrackingToggle({ activeManager }: { activeManager: OT_Member | undefined }) {
const t = useTranslations();
Expand Down Expand Up @@ -129,6 +130,9 @@ export function RequireDailyPlanToTrack() {
.filter((value, index, array) => array.indexOf(value) === index)
});
setEnabled(!enabled);
if (!enabled) {
localStorage.removeItem(DAILY_PLAN_SUGGESTION_MODAL_DATE);
}
}
}, [activeTeam, editOrganizationTeam, enabled]);

Expand Down
43 changes: 30 additions & 13 deletions apps/web/lib/features/daily-plan/add-daily-plan-work-hours-modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Card, InputField, Modal, Text } from 'lib/components';
import { Card, InputField, Modal, SpinnerLoader, Text } from 'lib/components';
import { Button } from '@components/ui/button';
import { useCallback, useMemo, useState } from 'react';
import { DAILY_PLAN_ESTIMATE_HOURS_MODAL_DATE } from '@app/constants';
Expand All @@ -19,25 +19,38 @@ export function AddDailyPlanWorkHourModal(props: IAddDailyPlanWorkHoursModalProp
const { updateDailyPlan } = useDailyPlan();
const { startTimer } = useTimerView();
const { activeTeam } = useTeamTasks();

const [workTimePlanned, setworkTimePlanned] = useState<number | undefined>(plan.workTimePlanned);
const currentDate = useMemo(() => new Date().toISOString().split('T')[0], []);
const requirePlan = useMemo(() => activeTeam?.requirePlanToTrack, [activeTeam?.requirePlanToTrack]);
const hasWorkHours = useMemo(() => plan.workTimePlanned && plan.workTimePlanned > 0, [plan.workTimePlanned]);
const [loading, setLoading] = useState(false);

const handleCloseModal = useCallback(() => {
localStorage.setItem(DAILY_PLAN_ESTIMATE_HOURS_MODAL_DATE, currentDate);
closeModal();
startTimer();
}, [closeModal, currentDate, startTimer]);
}, [closeModal, currentDate]);

const handleSubmit = useCallback(async () => {
try {
setLoading(true);

// Update the plan work time only if the user changed it
if (plan && plan.workTimePlanned !== workTimePlanned) {
await updateDailyPlan({ workTimePlanned }, plan.id ?? '');
}

startTimer();

const handleSubmit = useCallback(() => {
updateDailyPlan({ workTimePlanned }, plan.id ?? '');
handleCloseModal();
}, [handleCloseModal, plan.id, updateDailyPlan, workTimePlanned]);
handleCloseModal();
} catch (error) {
console.log(error);
} finally {
setLoading(false);
}
}, [handleCloseModal, plan, startTimer, updateDailyPlan, workTimePlanned]);

return (
<Modal isOpen={isOpen} closeModal={handleCloseModal} showCloseIcon={requirePlan ? false : true}>
<Modal isOpen={isOpen} closeModal={handleCloseModal} showCloseIcon>
<Card className="w-full" shadow="custom">
<div className="flex flex-col justify-between">
<div className="mb-7">
Expand Down Expand Up @@ -66,19 +79,23 @@ export function AddDailyPlanWorkHourModal(props: IAddDailyPlanWorkHoursModalProp
<Button
variant="outline"
type="submit"
className="py-3 px-5 rounded-md font-light text-md dark:text-white dark:bg-slate-700 dark:border-slate-600"
className="py-3 px-5 min-w-[10rem] rounded-md font-light text-md dark:text-white dark:bg-slate-700 dark:border-slate-600"
onClick={handleCloseModal}
>
{t('common.SKIP_ADD_LATER')}
</Button>
<Button
variant="default"
type="submit"
disabled={requirePlan ? (hasWorkHours ? false : true) : false}
className="py-3 px-5 rounded-md font-light text-md dark:text-white"
disabled={loading || (requirePlan && !hasWorkHours)}
className="py-3 px-5 min-w-[10rem] rounded-md font-light text-md dark:text-white"
onClick={handleSubmit}
>
{t('timer.todayPlanSettings.START_WORKING_BUTTON')}
{loading ? (
<SpinnerLoader variant="light" size={20} />
) : (
t('timer.todayPlanSettings.START_WORKING_BUTTON')
)}
</Button>
</div>
</div>
Expand Down
39 changes: 21 additions & 18 deletions apps/web/lib/features/daily-plan/add-task-estimation-hours-modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,10 @@ export function AddTasksEstimationHoursModal(props: IAddTasksEstimationHoursModa
const t = useTranslations();
const { updateDailyPlan, myDailyPlans } = useDailyPlan();
const { startTimer, timerStatus } = useTimerView();
const { activeTeam, activeTeamTask, setActiveTask } = useTeamTasks();
const { activeTeamTask, setActiveTask } = useTeamTasks();
const [showSearchInput, setShowSearchInput] = useState(false);
const [workTimePlanned, setWorkTimePlanned] = useState<number>(plan?.workTimePlanned ?? 0);
const currentDate = useMemo(() => new Date().toISOString().split('T')[0], []);
const requirePlan = useMemo(() => activeTeam?.requirePlanToTrack, [activeTeam?.requirePlanToTrack]);
const tasksEstimationTimes = useMemo(
() => (plan && plan.tasks ? estimatedTotalTime(plan.tasks).timesEstimated / 3600 : 0),
[plan]
Expand Down Expand Up @@ -108,12 +107,24 @@ export function AddTasksEstimationHoursModal(props: IAddTasksEstimationHoursModa
/**
* The function that close the Planned tasks modal when the user ignores the modal (Today's plan)
*/
const closeModalAndStartTimer = useCallback(() => {
handleCloseModal();
if (canStartWorking) {
startTimer();
const closeModalAndSubmit = useCallback(async () => {
try {
setLoading(true);

// Update the plan work time only if the user changed it
plan &&
plan.workTimePlanned !== workTimePlanned &&
(await updateDailyPlan({ workTimePlanned }, plan.id ?? ''));

setPlanEditState({ draft: false, saved: true });

handleCloseModal();
} catch (error) {
console.log(error);
} finally {
setLoading(false);
}
}, [canStartWorking, handleCloseModal, startTimer]);
}, [handleCloseModal, plan, updateDailyPlan, workTimePlanned]);

/**
* The function that opens the Change task modal if conditions are met (or start the timer)
Expand Down Expand Up @@ -454,19 +465,11 @@ export function AddTasksEstimationHoursModal(props: IAddTasksEstimationHoursModa
</div>
<div className=" flex justify-between items-center">
<Button
disabled={
loading || isRenderedInSoftFlow
? canStartWorking && requirePlan
: timerStatus?.running
? canStartWorking && requirePlan && (planEditState.draft || warning)
? true
: false
: canStartWorking && requirePlan && Boolean(warning)
}
disabled={loading}
variant="outline"
type="submit"
className="py-3 px-5 w-40 rounded-md font-light text-md dark:text-white dark:bg-slate-700 dark:border-slate-600"
onClick={isRenderedInSoftFlow ? closeModalAndStartTimer : handleCloseModal}
onClick={isRenderedInSoftFlow ? closeModalAndSubmit : handleCloseModal}
>
{isRenderedInSoftFlow ? t('common.SKIP_ADD_LATER') : t('common.CANCEL')}
</Button>
Expand All @@ -489,7 +492,7 @@ export function AddTasksEstimationHoursModal(props: IAddTasksEstimationHoursModa
return (
<>
{isRenderedInSoftFlow ? (
<Modal isOpen={isOpen} closeModal={closeModalAndStartTimer} showCloseIcon={requirePlan ? false : true}>
<Modal isOpen={isOpen} closeModal={closeModalAndSubmit} showCloseIcon>
<Card className="w-[36rem]" shadow="custom">
{content}
</Card>
Expand Down
23 changes: 17 additions & 6 deletions apps/web/lib/features/daily-plan/suggest-daily-plan-modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { useCallback, useMemo } from 'react';
import { DAILY_PLAN_SUGGESTION_MODAL_DATE, HAS_SEEN_DAILY_PLAN_SUGGESTION_MODAL } from '@app/constants';
import { useTranslations } from 'next-intl';
import Link from 'next/link';
import { useAuthenticateUser } from '@app/hooks';
import { useAuthenticateUser, useTeamTasks, useTimer } from '@app/hooks';
import { usePathname } from 'next/navigation';

interface ISuggestDailyPlanModalProps {
Expand All @@ -14,27 +14,38 @@ interface ISuggestDailyPlanModalProps {

export function SuggestDailyPlanModal(props: ISuggestDailyPlanModalProps) {
const { isOpen, closeModal } = props;

const { hasPlan } = useTimer();
const { activeTeam } = useTeamTasks();
const { user } = useAuthenticateUser();
const name = useMemo(
() => user?.name || user?.firstName || user?.lastName || user?.username || '',
[user?.firstName, user?.lastName, user?.name, user?.username]
);
const requirePlan = useMemo(() => activeTeam?.requirePlanToTrack, [activeTeam?.requirePlanToTrack]);
const path = usePathname();
const t = useTranslations();

const currentDate = useMemo(() => new Date().toISOString().split('T')[0], []);

const handleCloseModal = useCallback(() => {
localStorage.setItem(HAS_SEEN_DAILY_PLAN_SUGGESTION_MODAL, currentDate);
if (!requirePlan || (requirePlan && hasPlan)) {
localStorage.setItem(HAS_SEEN_DAILY_PLAN_SUGGESTION_MODAL, currentDate);
}
Comment on lines +31 to +33
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify conditional expressions in handleCloseModal

The conditions if (!requirePlan || (requirePlan && hasPlan)) can be simplified to if (!requirePlan || hasPlan) for clarity and maintainability, as they are logically equivalent.

Apply this diff:

const handleCloseModal = useCallback(() => {
-    		if (!requirePlan || (requirePlan && hasPlan)) {
+    		if (!requirePlan || hasPlan) {
    			localStorage.setItem(HAS_SEEN_DAILY_PLAN_SUGGESTION_MODAL, currentDate);
    		}
    		if (path.split('/')[1] == 'profile') {
-    			if (!requirePlan || (requirePlan && hasPlan)) {
+    			if (!requirePlan || hasPlan) {
    				localStorage.setItem(DAILY_PLAN_SUGGESTION_MODAL_DATE, currentDate);
    			}
    		}
    		closeModal();
    	}, [closeModal, currentDate, hasPlan, path, requirePlan]);

Also applies to: 35-37

Copy link
Member

Choose a reason for hiding this comment

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

@CREDO23 why you not follow those!?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

if (path.split('/')[1] == 'profile') {
localStorage.setItem(DAILY_PLAN_SUGGESTION_MODAL_DATE, currentDate);
if (!requirePlan || (requirePlan && hasPlan)) {
localStorage.setItem(DAILY_PLAN_SUGGESTION_MODAL_DATE, currentDate);
}
}
closeModal();
}, [closeModal, currentDate, path]);
}, [closeModal, currentDate, hasPlan, path, requirePlan]);

return (
<Modal isOpen={isOpen} closeModal={handleCloseModal} showCloseIcon={false}>
<Modal
closeOnOutsideClick={requirePlan}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Correct the modal's closing behavior logic

The current implementation allows users to close the modal even when a plan is required but not yet created. To prevent users from dismissing the modal under these conditions, update the closeOnOutsideClick and showCloseIcon properties to reflect both requirePlan and hasPlan.

Apply this diff to fix the logic:

<Modal
-    			closeOnOutsideClick={requirePlan}
+    			closeOnOutsideClick={!requirePlan || hasPlan}
     			isOpen={isOpen}
     			closeModal={handleCloseModal}
-    			showCloseIcon={requirePlan}
+    			showCloseIcon={!requirePlan || hasPlan}
    		>

Also applies to: 47-47

isOpen={isOpen}
closeModal={handleCloseModal}
showCloseIcon={requirePlan}
>
<Card className="w-full" shadow="custom">
<div className="flex flex-col items-center justify-between">
<div className="mb-7">
Expand Down
Loading
Loading