Add unit test for Modal Page Service

Hi!

I started to write something. I'm still working on this but any tips/contributions are welcome as it's my first unit test.

I got help from @diegors and @gquisini, if you can please give credits.

• added variable doc comments;
• changed mocks creation to build it with the general best-practice defaults instead of create with my own requirements;
• fixed testIfModalHasNoType doc comments;
• created method to tests getCurrentPath().

Hi!

We couldn't set up a modal by parameters so we don't write tests for this now. Here's what we did:

• we edit the welcome modal that comes by default changing the extra config to use modal by parameter instead of page;
• fill the "Delay to display" field with 0;
• fill the "Parameters" textarea with /?modal=welcome as suggested in the field's description.

See modal-by-parameter-settings.png attached.

Maybe a bug here?

I checkout the Modal Settings documentation and didn't find any references to "parameter". Maybe can we create an issue to add this later?

With that, the methods left to test/discuss if it's worth testing are:

• getModalToShowByParameter
• verifyModalShouldAppearOnThisLanguage
• clearText
• verifyModalShouldAppearOnThisPath
• getAllowTags
• importModalConfigToEntity
• extractFilesUuid
• recordFileUsage

Ok, @RenatoG!
Thanks for the quick response!

submitting new patch:

• removed experimental test for getModalToShowByParameter;
• added test to verifyModalShouldAppearOnThisLanguage() method without languages to show.

Changing status to "needs review" because we're starting to feel stuck.

This is our first unit test, so we may have made some mistakes. Feel free to suggest modifications and improvements, we'll be willing to fix them out.

Kindly review.

Fixed! Thanks.

Hi!

Thanks for the review, @sophiavs! I'll work on this.

Added method to test loadModalsToShow() with multiple modals having different attributes.

I found out a bug in testGetModalsToShowWithoutModals(). The mock was not made correctly, so the modals used to build the object came actually from setUp() - which had created 1 modal. As it was not defined in setUp() if the modal was published, getModalsToShow() returned FALSE anyway.

Fixed!

I took the opportunity to add a forgotten $this-> in the previous method.

After I finish @sophiavs suggestions, I'll debug all tests and double check if they're really working as they should.

Added method to test getModalToShowByPage() with multiple pages.

In this patch I:
- replaced assertTrue with assertEquals where it's more suitable;
- broke lines to be no longer than 80 characters, whenever possible, to follow the coding standards.

Addressed #18:

After I finish @sophiavs suggestions, I'll debug all tests and double check if they're really working as they should.

I debugged the real behavior of getModalToShowByPage method setting the modal with multiple pages from site. With this, I verified that testShowByPageWithMultiplePages was not correct. It's a void function in this case, we have to assert this.

I also checked that testIfUserHasPermissionOnModal is returning FALSE before getRoles is called. Futhermore it's duplicated with testIfUserHasPermissionOnModalWithPermissions. Must be deleted.

I'm working on it.

Patch addressed #21.

@sophiavs warned me about another bug, since testShowByPageSetToNode is checking if the modal is displayed for /node/1, its current path must also be node/1.

Thank you, @sophiavs! Attaching new patch to fix this.