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.

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

E2E default maximum tickets #1108

Merged
merged 7 commits into from
Jan 4, 2022
Merged

E2E default maximum tickets #1108

merged 7 commits into from
Jan 4, 2022

Conversation

marlonhario
Copy link
Contributor

For this ticket:

  • E2E test for default maximum tickets allowed at default settings.

@github-actions github-actions bot added C: tests 🧪 category D: Packages 📦 domain: Barista Packages labels Dec 20, 2021
@tn3rb tn3rb changed the title E2 e default maximum tickets E2E default maximum tickets Dec 20, 2021
import { Goto, DefaultSettingsManager, EventsListSurfer, createNewEvent } from '@e2eUtils/admin';
import { eventData } from '../../../shared/data';

const defaultSettingsManager = new DefaultSettingsManager();
Copy link
Member

Choose a reason for hiding this comment

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

👍🏻

Comment on lines 13 to 18
// delete all events from view all events link
await eventsListSurfer.deleteAllEventsByLink('View All Events');
// delete all events from draft link
await eventsListSurfer.deleteAllEventsByLink('Draft');
// delete permanently all events at trash link
await eventsListSurfer.deleteAllPermanentlyFromTrash();
Copy link
Member

Choose a reason for hiding this comment

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

do we need t o delete all events to test the default max tickets?
i would think that would unnecessarily add to the time it takes to run tests.
if the issue is just for the sake of cleaning up after tests, then maybe that should be moved into the afterAll() function?
could even be extracted into a utility function like: deleteEventsAfterTest() or postTestEventCleanup() or ???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes sorry, I think this one is not needed.

Comment on lines 29 to 36
let countEvents: number;
it('Count view all events link for test starting', async () => {
// Get selected default status and return weither option value or innertext of the option
countEvents = await defaultSettingsManager.getViewCount('View All Events');
console.log({ countEvents });
// assert the current selected registration status
expect(countEvents).toBe(0);
});
Copy link
Member

Choose a reason for hiding this comment

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

similar to deleting events... do we need to count and verify zero events prior to testing the default max tickets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes also this one is not needed sorry for that.

Comment on lines 44 to 47
const countEventsAfterCreatedNew = await defaultSettingsManager.getViewCount('View All Events');
const countAddedEvent = countEventsAfterCreatedNew - countEvents;
// assert the current selected registration status
expect(countEventsAfterCreatedNew).toBe(countAddedEvent + countEvents);
Copy link
Member

Choose a reason for hiding this comment

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

don't need to count anything for this test...
except... it might be good to verify that the event was created before proceeding,
so asserting that createNewEvent() succeeded would be good, but that can maybe be done within that function itself

Comment on lines 50 to 68
it('Create sample ticket', async () => {
// get the first event in trash
const firstItem = await defaultSettingsManager.getFirstListItem();
// got to "restore from trash" action link for the selected first event
const restoreLink = await defaultSettingsManager.getItemActionLinkByText(firstItem, 'Edit');
await page.goto(restoreLink);

// await Promise.all([page.waitForNavigation(), page.click('.ee-entity-list__footer button[type="button"]')]);
await page.click(
'#ee-event-editor .ee-entity-list__footer button[type="button"] span:has-text("Add New Ticket")'
);
await page.click('input[name="name"]');
await page.fill('input[name="name"]', 'Ticket sample name');
await page.fill('.public-DraftEditor-content', 'Ticket sample description');
await page.click('.ee-modal__footer span:has-text("Skip prices - assign dates")');
await page.click('#ee-ticket-assignments-manager-table-data-cell-row-0-col-1 button');
await page.click('button[type="submit"]');
console.log({ firstItem });

expect(0).toBe(0);
});
Copy link
Member

@tn3rb tn3rb Dec 20, 2021

Choose a reason for hiding this comment

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

we don't need to create any tickets for this test.
Creating a new event shoud result in at least one default ticket getting added, but that still has nothing to do with what we need to test here. All we need to confirm is that setting the value of this input:

default max tickets setting

results in the correct value here on a newly created event:

EDTR:
Screenshot 2021-12-20 123421

Legacy Event Editor:
Legacy Editor Reg Options

The code you added in this test may be useful somewhere else in another test, so maybe just copy it to a file somewhere on your local system for now (as opposed to just deleting it)

Copy link
Member

@tn3rb tn3rb left a comment

Choose a reason for hiding this comment

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

sry but this needs some refactoring.
it does not appear to be testing the correct functionality.
I think it might help if I create issues for each test ahead of time so that I can explain what each test needs to do and include screenshots if need be

@tn3rb tn3rb added the T: deprecated ⚠️ type: DEP - soon-to-be removed features label Dec 21, 2021
Copy link
Member

@tn3rb tn3rb left a comment

Choose a reason for hiding this comment

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

Better but still not quite right.

The biggest issue is a misunderstanding about how default settings work.

Default settings, such as the "Default Maximum Tickets Allowed" are for configuring the initial values used when creating a new event. So the process needs to work like this:

  • go to the "Default Settings" tab in the Events admin
  • set "Default Maximum Tickets Allowed" option and click "Save"
  • go to Events Overview tab and click "Add New Event"
  • "Max Registrations per Transaction" value under "Registration Options" for the new event should match what was set in the first step.

The following will NOT work

  • go to Events Overview tab and click "Add New Event"
  • take note of the "Max Registrations per Transaction" value
  • go to the "Default Settings" tab in the Events admin
  • set "Default Maximum Tickets Allowed" option to something new and click "Save"
  • return to Events Overview tab, find previously created event in list and click to edit it
  • "Max Registrations per Transaction" value under "Registration Options" will be the SAME value it was when we first viewed it and NOT match the new value that was just set for the "Default Maximum Tickets Allowed" option

So these tests need to be slightly rearranged so that a new event is created AFTER the "Default Maximum Tickets Allowed" option has been set to a new value.

As well, plz note that you can use hard coded values for this option. So for example you could refactor the tests to work like this:

  • go to the "Default Settings" tab in the Events admin
  • set "Default Maximum Tickets Allowed" option to 7 and click "Save"
  • go to Events Overview tab
  • click "Add New Event"
  • verify that "Max Registrations per Transaction" value equals 7

Copy link
Member

@tn3rb tn3rb left a comment

Choose a reason for hiding this comment

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

aaaaaalllllllllllmost there...

just have a few more tweaks that need to be done, but is otherwise looking really good

Comment on lines 31 to 34
// get added number
const getAdded = Number(checkEDTRMaxTicket) - Number(getDefaultMaxBeforeChange);
// assert maximum ticket allowed at EDTR
expect(Number(checkEDTRMaxTicket)).toBe(getAdded + Number(getDefaultMaxBeforeChange));
Copy link
Member

Choose a reason for hiding this comment

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

in this scenario we don't need to calculate the difference because whatever we set for the new default value is what it will be (in this case 7) regardless of anything else. So we can just compare directly against that value. ie:

Suggested change
// get added number
const getAdded = Number(checkEDTRMaxTicket) - Number(getDefaultMaxBeforeChange);
// assert maximum ticket allowed at EDTR
expect(Number(checkEDTRMaxTicket)).toBe(getAdded + Number(getDefaultMaxBeforeChange));
// assert maximum ticket allowed at EDTR
expect(Number(checkEDTRMaxTicket)).toBe( 7 );

In the previous tests you were writing for the Events List, we absolutely needed to calculate the number of events dynamically, because the number of events in our tests data array could easily change over time. Hardcoding values there would have resulted in failing tests whenever a new event was added to the test data.

Now that said, it might be worthwhile to verify that the value has actually changed and that the new value is indeed a new value, by incrementing its value, ex:

		// Get the default value set on default maximum ticket allowed field
		const getDefaultMaxBeforeChange = await defaultSettingsManager.getDefaultMaxTicket();
		// add 2 to max value
		newDefaultMax = getDefaultMaxBeforeChange + 2;
		// Set new value for default maximum ticket allowed
		await defaultSettingsManager.setNewValueForDefaultMaxTicket( `${newDefaultMax}` );

then you would later assert that the value set in the EDTR is equal to newDefaultMax instead of 7
But it's safe to just use a hardcoded value like 7 since the default value hardcoded in the PHP code is 10

@@ -23,24 +23,26 @@ describe('Default registration status test', () => {
// Get selected default status and return weither option value or innertext of the option
const getSelectedDefaultStatus = await defaultSettingsManager.getSelectedDefaultStatus({ value: true });
// assert the current selected registration status
expect(getSelectedDefaultStatus).toBe(defaultRegStatusOptions[getSelectedDefaultStatus].value);
expect(getSelectedDefaultStatus).toBe(
defaultSettingsData.defaultRegStatusOptions[getSelectedDefaultStatus].value
Copy link
Member

Choose a reason for hiding this comment

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

nice

Comment on lines 87 to 99
/**
* Go to add new registration
*/
goToAddNewReg = async (): Promise<void> => {
await Promise.all([page.waitForNavigation(), page.click('a:has-text("Add New Registration")')]);
};

/**
* Check if the value exist in select option innertext
*/
checkSelectOptionValue = async (value: string): Promise<string> => {
return await (await page.$(`.ticket-selector-tbl-qty-slct option:has-text("${value}")`)).innerText();
};
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking these should be moved to a different file:

  • goToAddNewReg
  • checkSelectOptionValue

appear to be for the Registrations admin page when filtered for a specific event,
so maybe something like:

packages/e2e-tests/utils/admin/registrations/RegListManager.ts

which would make it easier to find those utilities when working on tests for the Registrations admin and help avoid duplication

Comment on lines 111 to 116
/**
* get "Max Registrations per Transaction" value under "Registration Options"
*/
getEventRegMaxTicket = async (): Promise<string> => {
return await (await page.$('#max-registrants')).getAttribute('value');
};
Copy link
Member

Choose a reason for hiding this comment

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

and this should be moved too, maybe something like:

packages/e2e-tests/utils/admin/events/editor/RegistrationOptions.ts

which could contain utilities for all of the inputs in that card:

  • Default Registration Status
  • Ticket Selector Enabled
  • Max Registrations per Transaction
  • Alternative Registration Page

but you don't have to create functions for those in this PR.

again, this will help anyone (ie: you 😄) writing tests for the EDTR and hopefully help avoid duplication

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see hehehe I will update it, thank you for the comment @tn3rb

Copy link
Member

@tn3rb tn3rb left a comment

Choose a reason for hiding this comment

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

yup... perfect now 💯

@tn3rb tn3rb merged commit 3edfdf3 into master Jan 4, 2022
@tn3rb tn3rb deleted the E2E-default-maximum-tickets branch January 4, 2022 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: tests 🧪 category D: Packages 📦 domain: Barista Packages S11: completed 🚀 status T: deprecated ⚠️ type: DEP - soon-to-be removed features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants