Skip to content

Adds tests for date overrides#6027

Merged
zomars merged 1 commit intomainfrom
fixes/date-overrides-testing-and-troubleshooter
Dec 15, 2022
Merged

Adds tests for date overrides#6027
zomars merged 1 commit intomainfrom
fixes/date-overrides-testing-and-troubleshooter

Conversation

@zomars
Copy link
Copy Markdown
Contributor

@zomars zomars commented Dec 14, 2022

What does this PR do?

Type of change

  • Chore (refactoring code, technical debt, workflow improvements)

How should this be tested?

  • yarn e2e apps/web/playwright/availability.e2e.ts

@vercel
Copy link
Copy Markdown

vercel Bot commented Dec 14, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
cal ✅ Ready (Inspect) Visit Preview Dec 14, 2022 at 9:42PM (UTC)

@zomars zomars requested a review from a team December 14, 2022 21:35
Copy link
Copy Markdown
Contributor Author

@zomars zomars left a comment

Choose a reason for hiding this comment

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

Self review done

onChange={(ranges) => append({ ranges })}
Trigger={
<Button color="secondary" StartIcon={Icon.FiPlus}>
<Button color="secondary" StartIcon={Icon.FiPlus} data-testid="add-override">
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Needed for testing

Comment on lines +131 to +133
render={({ field }) => (
<EditableHeading isReady={!isLoading} {...field} data-testid="availablity-title" />
)}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same

Comment on lines +19 to +21
const { date, setQuery: setSelectedDate } = useRouterQuery("date");
const selectedDate = dayjs(date);
const formattedSelectedDate = selectedDate.format("YYYY-MM-DD");
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Use URL instead of state for dates

Comment on lines +35 to +44
const overrides =
data?.dateOverrides.reduce((acc, override) => {
if (
formattedSelectedDate !== dayjs(override.start).format("YYYY-MM-DD") &&
formattedSelectedDate !== dayjs(override.end).format("YYYY-MM-DD")
)
return acc;
acc.push({ ...override, source: "Date override" });
return acc;
}, [] as IBusySlot[]) || [];
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Filter overrides specific to the selected date

{t("your_day_starts_at")} {convertMinsToHrsMins(user.startTime)}
</div>
</div>
{isLoading ? (
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm starting to dislike ternaries for these use cases.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree. It makes the code very confusing if ternary is more than 1 level

);

if (data && (data.busy.length > 0 || overrides.length > 0))
return [...data.busy, ...overrides]
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Reusing this for both override and busy times

});
});

test("Availablity pages", async ({ page }) => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added basic adding availability test

});

test("Date Overrides", async ({ page }) => {
await test.step("Can add a date override", async () => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Basic test

await page.locator('[form="availability-form"][type="submit"]').click();
});

await test.step("Date override is displayed in troubleshooter", async () => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Check the added override actually works

Copy link
Copy Markdown
Member

@hariombalhara hariombalhara left a comment

Choose a reason for hiding this comment

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

Looks good. Except the troubleshooter issue which is a separate bug and not related to Date Overrides.

});

test("Date Overrides", async ({ page }) => {
await test.step("Can add a date override", async () => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Didn't know you can break tests logically like this.👍

{t("your_day_starts_at")} {convertMinsToHrsMins(user.startTime)}
</div>
</div>
{isLoading ? (
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree. It makes the code very confusing if ternary is more than 1 level

);

const overrides =
data?.dateOverrides.reduce((acc, override) => {
Copy link
Copy Markdown
Member

@hariombalhara hariombalhara Dec 15, 2022

Choose a reason for hiding this comment

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

Date overrides aren't shown in troubleshooter if there are more than 1 availabilities and date override is used in second one.

This seems to be due to the reason that getUserAvailability picks the first availability/schedule(because eventTypeId isn't provided)
If a default schedule is set, I think that's used.

Screenshot 2022-12-15 at 1 15 31 PM

This seems to be a bug in troubleshooter flow and not specific to Date overrides.
cc @emrysal

Also, I am confused as to if troubleshooter is expected to troubleshoot one schedule or all the schedules of user in general.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a job for a followup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants