Skip to content

Conversation

@labkey-ankurj
Copy link
Contributor

Rationale

Automated test updates for the new error page

Related Pull Requests

Comment on lines 289 to 290
getWrapper().goToHome();

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 change the behavior of this method (It only navigates if ends up on a permission denied page after impersonating). It is fairly widely used and some probably expect to be on the same page after impersonating.
Does the 403 page (or whatever page we end up on) not have a link to go to home anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not have home button on the error page anymore. Should I check for the response code from the title of the page and then route to home?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. And just click the logo in the upper-left. I'd rather not move goToHome into WebDriverWrapper.

Comment on lines 864 to 865
waitForAnyElement(Locators.labkeyError, Locators.labkeyErrorSubHeading);
assertElementPresent(Locators.labkeyErrorSubHeading.withText("Registration is not enabled."));
Copy link
Member

Choose a reason for hiding this comment

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

This check should be specific. We know what page we should end up on. If timing is a problem, we can wait for the error.

Suggested change
waitForAnyElement(Locators.labkeyError, Locators.labkeyErrorSubHeading);
assertElementPresent(Locators.labkeyErrorSubHeading.withText("Registration is not enabled."));
waitForElement(Locators.labkeyErrorSubHeading.withText("Registration is not enabled."));

Comment on lines 190 to 191
protected static final String PERMISSION_ERROR = "User does not have permission to perform this operation.";
protected static final String NOT_FOUND_ERROR = "notFound";
Copy link
Member

Choose a reason for hiding this comment

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

Move PERMISSION_ERROR and NOT_FOUND_ERROR to SecurityTest. The only other place that uses PERMISSION_ERROR is an unused method, which can be removed: BaseWebDriverTest.assertAtUserUserLacksPermissionPage.

Copy link
Member

@labkey-tchad labkey-tchad left a comment

Choose a reason for hiding this comment

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

A couple of questions.

  • In Crawler.isIgnoredError, we ignore 404s from modules that aren't enabled in the current container. Should we update that check or do we think 404 is an inappropriate response for those actions? It probably isn't appropriate since we have links to all modules in the admin menu, regardless of whether they are enabled.
  • Is Locator.css("table.server-error") still valid? Its used in Crawler.checkForServerError and Crawler.crawlLink.

@labkey-jeckels
Copy link
Contributor

@labkey-tchad An action from an inactive module won't automatically return a 404. Most should return 200 and work OK. For example, neither Study nor Flow is enabled in this folder:

https://www.labkey.org/home/study-begin.view?

https://www.labkey.org/home/flow-begin.view?

I see exactly one spot in the code that returns a 404 with the message about a module not being enabled, in a MS2 protein search action.

@labkey-ankurj
Copy link
Contributor Author

@labkey-tchad I think Locator.css("table.server-error") is still valid because that is still used in WebPartErrorRenderer.

@labkey-ankurj
Copy link
Contributor Author

@labkey-tchad all the tests passed. Let me know if there are changes to be made.

@labkey-ankurj labkey-ankurj merged commit 98bfc78 into develop Oct 9, 2020
@labkey-ankurj labkey-ankurj deleted the fb_ErrorPage_7767 branch October 9, 2020 18:06
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.

4 participants