[PW] - DSM tissue request flow by GiCharkviani · Pull Request #2211 · broadinstitute/ddp-angular

GiCharkviani · 2023-09-13T10:27:12Z

Video from .spec file:

video.webm

GiCharkviani · 2023-09-13T10:35:24Z

playwright-e2e/tests/dsm/tissueRequestFlow/cmi-tissue-request-flow.spec.ts

@@ -0,0 +1,294 @@
+import {StudyEnum} from '../../../dsm/component/navigation/enums/selectStudyNav-enum';


Don't review .spec file.

aweng98

This is great. A lot of new classes and functions. I'd have written only what I needed for a new test and not write all at one time. :)
Took a first pass. Maybe we can talk about this tomorrow morning. Because I think we should pick out only needed files for a new test. Merge everything without accompanied tests is just confusing.

playwright-e2e/dsm/component/date-picker.ts

aweng98 · 2023-09-14T02:42:16Z

playwright-e2e/dsm/component/smid.ts

+        if (isInputVisible) {
+          await this.fillField(value, i);
+          selectCheckbox && await this.selectCheckbox(i);
+        } else {
+          await this.addField();
+          await this.fillField(value, i);
+          selectCheckbox && await this.selectCheckbox(i);
+        }


How about this?

if (isInputVisible) { await this.addField(); } await this.fillField(value, i); selectCheckbox && await this.selectCheckbox(i);

I will improve that this way:

if (!isInputVisible) { await this.addField(); } await this.fillField(value, i); selectCheckbox && await this.selectCheckbox(i);

playwright-e2e/dsm/component/smid.ts

aweng98 · 2023-09-14T03:03:58Z

playwright-e2e/dss/component/input.ts

+  public get maxLength(): Promise<string | null> {
+    return this.toLocator().getAttribute('maxlength');
+  }


async and get modifier are incompatible. It should be public async maxLength(): Promise<string | null>. If you don't use async modifier, eslint won't find error when calling functions not using await.

Actually, IDE can detect if you didn't use await, like so:

playwright-e2e/dss/component/checkbox.ts

aweng98 · 2023-09-14T03:13:03Z

playwright-e2e/dsm/pages/tissue-information-page/tissue-information-page.ts

+    return data?.trim() as string;
+  }
+
+  public async fillFaxSentDates(date1: FillDate, date2?: FillDate, date3?: FillDate): Promise<void> {


I think you want use rest parameter syntax: fillFaxSentDates(...date: FillDate)

I believe I'd like to keep it as it is since it appears more readable to me. With just three parameters to define, there's no need for looping through them. However, I do acknowledge that the rest parameter is a fantastic feature in JavaScript.

aweng98 · 2023-09-14T03:27:18Z

playwright-e2e/dsm/component/tables/onc-history-table.ts

+import Input from '../../../dss/component/input';
+
+export default class OncHistoryTable extends Table {
+  private readonly tissueInformationPage = new TissueInformationPage(this.page);


Hmm, I saw something else similar like this recently and I had to change because we should instatiate a new object like this outside the constructor. tissueInformationPage = new TissueInformationPage(this.page); should be inside the constructor because this.page is created when the constructor is running.

I didn't encounter any issues with it, which is why I left it as is. However, I do acknowledge that your suggestion is a better approach, so I'll make the necessary adjustments. Thank you for pointing that out.

aweng98 · 2023-09-14T03:32:08Z

playwright-e2e/dsm/component/tables/onc-history-table.ts

+  public async selectDeselectRow(index: number): Promise<void> {
+    const selectRowCheckbox = this.selectRowCheckbox(index);
+    await expect(selectRowCheckbox, 'Select row checkbox is not visible').toBeVisible();
+    await selectRowCheckbox.click();
+  }


Function is little confusing to me. Is it deselect or select a checkbox?

In fact, it has the capability to perform both actions. I hadn't initially implemented the functionality to check whether the checkbox is selected or not, but I now understand your perspective and agree with it. I will modify it into a select method that includes the necessary checks. Thank you for the suggestion.

playwright-e2e/dsm/component/tabs/sample-information-tab.ts

aweng98 · 2023-09-14T03:48:07Z

playwright-e2e/dsm/pages/tissue-information-page/tissue-information-page.ts

+    await this.fillDate(faxSentLocator, date);
+  }
+
+  private async fillDate(root: Locator, {date, today}: FillDate): Promise<void> {


Using calendar to select date is time consuming and slow. Can we just type in date string in input field? I'd code this if it's for a date-picker test.

For the time being, let's retain it in its current state to align with the DSM flow, as selecting the date provides more immediate benefits.

aweng98 · 2023-09-14T03:50:25Z

playwright-e2e/dsm/pages/tissue-information-page/tissue-information-page.ts

+    const checkbox = new Checkbox(this.page, {root});
+    const isChecked = await checkbox.isChecked();
+    const isDisabled = await checkbox.isCheckboxDisabled();
+    if (check && !isChecked && !isDisabled) {


I don't think we should be checking isDisabled in this function because if user wants to check the checkbox. It is expected to be enabled. If it is disabled, nothing will happen (no error) and user assumes check was sucessful.

This is necessary due to the buggy flow on this page. We might come across a disabled checkbox unexpectedly, so having this in place helps us handle such scenarios.

aweng98 · 2023-09-14T03:53:43Z

playwright-e2e/dsm/pages/tissue-information-page/tissue-information-page.ts

+  }
+
+  private get participantDynamicInformationTableXPath(): string {
+    return `${this.pageXPath}/div/div[last()]/table[not(contains(@class, 'table'))]`


Brittle xpath. I'd expect this is going to be a problem in future.
I noticed you're not using any functions from table.ts when constructing xpath. Is there anying I can do to help?

Yes, please assist in making your table component more versatile by ensuring it's compatible with DSM table components as well!

aweng98 · 2023-09-14T04:00:07Z

playwright-e2e/dsm/pages/tissue-information-page/tissue-information-page.ts

+    await this.checkCheckbox(this.problemsWithTissueUnableToObtainCheckbox, unableToObtainSelection);
+  }
+
+  public async selectGender(gender: 'Male' | 'Female'): Promise<void> {


I think we have more gender types now than just two.

I saw there only 2

aweng98

👍🏻 Thank you for your patience.

playwright-e2e/dsm/component/smid.ts

aweng98 · 2023-09-14T16:37:54Z

playwright-e2e/dsm/component/smid.ts

+  private async addField(): Promise<void> {
+    await this.addBtn.click();
+  }


Since this is a one liner function, it can be replaced by rename addBtn() to addFieldBtn()

await this.addFieldBtn.click();

playwright-e2e/dsm/component/smid.ts

playwright-e2e/dss/component/select.ts

playwright-e2e/dss/component/textarea.ts

aweng98 · 2023-09-14T17:30:23Z

playwright-e2e/utils/test-utils.ts

  }
 }

+export async function waitForRequest(page: Page, { uri, timeout }: WaitForRequest): Promise<Request> {


Can you explain requirement. Why wait for a request before click?

aweng98

LGTM. No issue with any tests on CircleCI.

playwright-e2e/dsm/component/tables/onc-history-table.ts

aweng98 · 2023-09-20T13:39:12Z

playwright-e2e/dsm/component/filters/sections/search/search.ts

  public async dates(columnName: string, { from: fromValue, to: toValue, additionalFilters }: Partial<DateConfig>): Promise<void> {
    await this.setAdditionalFilters(columnName, additionalFilters);

+    if (!fromValue && !toValue) { return; }


Good check!
Question: check should be moved up to be the first line of code, right?

Thanks.
No, because additional filters can still be applied.

aweng98

Can you also rename directory name from playwright-e2e/tests/dsm/tissueRequestFlow/ to playwright-e2e/tests/dsm/tissue-request/?
LGTM

playwright-e2e/dsm/pages/tissue-information-page/interfaces/tissue-information-interfaces.ts

aweng98 · 2023-09-20T13:56:12Z

playwright-e2e/tests/dsm/tissueRequestFlow/cmi-tissue-request-flow.spec.ts

+      const oncHistoryTab = await participantPage.clickTab<OncHistoryTab>(TabEnum.ONC_HISTORY);
+      const oncHistoryTable = oncHistoryTab.table;
+
+      await test.step('Update Onc History data - Facility', async () => {


Format indentation is wrong in this file.

What do you mean?

Tab size/whitespaces on new line. File looks okay now but I saw 4 whitespaces before.

GiCharkviani requested a review from aweng98 September 13, 2023 10:27

GiCharkviani changed the title ~~[PW] = DSM tissue request flow~~ [PW] - DSM tissue request flow Sep 13, 2023

GiCharkviani commented Sep 13, 2023

View reviewed changes

aweng98 reviewed Sep 14, 2023

View reviewed changes

aweng98 force-pushed the PW-DSM-Tissue-request-flow branch from fdc3843 to bb4c6b2 Compare September 15, 2023 13:49

aweng98 approved these changes Sep 15, 2023

View reviewed changes

playwright-e2e/dsm/component/tables/onc-history-table.ts Outdated Show resolved Hide resolved

playwright-e2e/dsm/component/tables/onc-history-table.ts Outdated Show resolved Hide resolved

GiCharkviani marked this pull request as ready for review September 20, 2023 13:15

aweng98 reviewed Sep 20, 2023

View reviewed changes

aweng98 approved these changes Sep 20, 2023

View reviewed changes

aweng98 approved these changes Sep 21, 2023

View reviewed changes

playwright tissue request test

808875a

aweng98 force-pushed the PW-DSM-Tissue-request-flow branch from 7355cb3 to 808875a Compare September 22, 2023 15:23

aweng98 self-requested a review September 22, 2023 17:56

aweng98 mentioned this pull request Sep 22, 2023

[Playwright] PEPPER-1125 New tissue request test #2235

Merged

		@@ -0,0 +1,294 @@
		import {StudyEnum} from '../../../dsm/component/navigation/enums/selectStudyNav-enum';

Conversation

GiCharkviani commented Sep 13, 2023 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweng98 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweng98 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweng98 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aweng98 Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweng98 left a comment

Choose a reason for hiding this comment

Uh oh!

GiCharkviani commented Sep 13, 2023 •

edited by atlassian bot

Loading

aweng98 Sep 20, 2023 •

edited

Loading