Item 7767: View details section of ErrorPage Redesign - Story 2 #1589

labkey-ankurj · 2020-09-22T22:07:18Z

Rationale

This is the continued work of error page redesign.

Related Pull Requests

Item 7767: Render new error page LabDevKitModules#54

Changes

View details section on Error Page

…ble object

cnathe

Just some quick comments. I'll do a more thorough code review after manual testing.

core/src/client/ErrorHandler/ErrorHandler.tsx

cnathe · 2020-09-24T20:00:47Z

core/src/client/ErrorHandler/ErrorType.tsx

+            If you are subscribed to LabKey, then reach out to your account manager for immediate help. If you are using
+            the community edition, your support ticket will be resolved within the next 2 to 3 weeks.
+        </div>
+        <br />
+        <div className="labkey-error-details">
+            {helpLinkNode('default', 'LabKey support documentation')} is another immediate resource that is available to
+            all users. A search through the documentation or previous forum questions may also help troubleshoot your
+            issue.


Does anyone know if this text has been reviewed by the support team? Looking at this page, it looks like the response time of community forum posts is "as time allows". Ankur, if you haven't chatted with the support team and reviewed these pages with them yet, I'm happy to do so.

I asked the services team around the wording of config exception in the specs but not all the pages. Yes, it'd be great if you don't mind chatting with the team. Thanks.

core/src/client/ErrorHandler/ErrorType.tsx

# Conflicts: # assay/package-lock.json # assay/package.json # core/package-lock.json # core/package.json # experiment/package-lock.json # experiment/package.json # issues/package-lock.json # issues/package.json # list/package-lock.json # list/package.json # query/package-lock.json # query/package.json # study/package-lock.json # study/package.json

…ffering media page widths

core/src/client/ErrorHandler/ErrorHandler.spec.tsx

core/src/client/ErrorHandler/ErrorHandler.tsx

core/src/org/labkey/core/webdav/DavController.java

cnathe · 2020-09-28T19:48:47Z

core/src/client/ErrorHandler/ErrorType.tsx

+                </li>
+            </ul>
+        </div>
+        {getServerContext().impersonatingUser !== undefined && (


It would be good to get some jest tests for the various different displays here for the permissions error with and without the impersonate user.

Not sure how to impersonate in jest tests, do you have any pointers to existing example?

If it is just a matter of setting some specific context on the LABKEY object for a certain test case (i.e. the LABKEY.impersonatingUser in this case), then we have an example of that in labkey-ui-components which might work here:
labkey-ui-components/packages/components/src/internal/app/utils.spec.ts

labkey-nicka · 2020-09-28T20:49:40Z

@labkey-ankurj I've closed the related @labkey/components PR so this branch can just update to the latest version of that package (which might result in no diff from develop).

labkey-nicka

I'd like to discuss the different usages of ErrorView and see if we can consolidate them so that the implementation that is specific to the view isn't the callers responsibility.

core/src/org/labkey/core/webdav/DavController.java

experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java

api/src/org/labkey/api/util/ExceptionUtil.java

labkey-ankurj · 2020-09-29T17:44:11Z

core/src/org/labkey/core/webdav/DavController.java

                            super.setStatus(HttpServletResponse.SC_OK);
                        }
                    }
+                    else if (CONTENT_TYPE_HTML.equals(getRequest().getHeader("Content-Type)")) ||


@labkey-nicka Let me know if this looks right. I tried to test through the URL link that you showed yesterday and it renders the error page in browser since the content type is text/html but returns json when apt content type is passed in the request.

Yes, this is now appropriately scoped and looks to still respond with JSON.

It is now assuming that the response code is 404 (not found) but then returning a status of 400 (bad request). What happens if a different type of error occurs? If is a 404 shouldn't we respond as such?

updated the status code here, but error type here I think should always be not found as this is being utilized only for not found pages.

api/src/org/labkey/api/util/ExceptionUtil.java

labkey-nicka · 2020-09-29T19:25:13Z

core/src/org/labkey/core/webdav/DavController.java

                            super.setStatus(HttpServletResponse.SC_OK);
                        }
                    }
+                    else if (CONTENT_TYPE_HTML.equals(getRequest().getHeader("Content-Type)")) ||


Yes, this is now appropriately scoped and looks to still respond with JSON.

It is now assuming that the response code is 404 (not found) but then returning a status of 400 (bad request). What happens if a different type of error occurs? If is a 404 shouldn't we respond as such?

core/src/org/labkey/core/webdav/DavController.java

labkey-nicka

Looks good to me. Only a couple nit items. I'll let @cnathe approve the PR once he's reviewed again.

labkey-nicka · 2020-10-01T17:09:16Z

api/src/org/labkey/api/view/template/errorView.jsp

+    LABKEY.requiresScript('core/gen/errorHandler.js', function() {
+    // LABKEY.requiresScript('http://localhost:3001/errorHandler.js', function() {
+
+        LABKEY.App.__app__.isDOMContentLoaded = true;


@labkey-ankurj Can you open a ticket to me about this. It's fine to leave this "as-is" in this PR but the app loader should correctly identify when the DOM is loaded so you don't have to touch it's "private" state.

labkey-nicka · 2020-10-01T17:12:27Z

api/src/org/labkey/api/view/template/errorView.jsp

-        message: "<%=unsafe(model.getHeading())%>",
-        errorType: "<%=unsafe(model.getErrorType())%>"
+    // pulling in theme based css for the not found pages to still respect the site's theme
+    LABKEY.requiresCss(<%=q("/core/css/" + PageFlowUtil.resolveThemeName(getContainer()) + ".css")%>);


nit: I might further update this comment to state why the CSS is being pulled in here rather than in the client dependencies for the JSP. Something like:

This error page may be invoked without the themes having been loaded for this container. We load the theme artifact for this container here to ensure the correct theme is loaded as this cannot be resolved during "addClientDependencies()" for this view.

labkey-jeckels · 2020-10-01T22:40:25Z

api/src/org/labkey/api/util/ExceptionUtil.java

        catch (Exception e)
        {
+            // config exceptions that occur before jsps have been initialized
+            if (ex instanceof ConfigurationException && e instanceof NullPointerException)


Can we do a more direct check if JSPs are ready to render? What's null that's causing this exception?

labkey-ankurj added 9 commits September 14, 2020 15:13

Item 7767: basic setup of details section

4f3ee39

Per error type specific components / pages with details content

66565b6

help docs links and back button functionality

67debf9

Permissions page handling of impersonated user

6956771

jest tests

e8edecf

Merge branch 'develop' into fb_ErrorPage_7767

4644051

upgrade ui-components alpha version

18949d1

stop impersonating button, address todos, remove old code

9f2adbc

remove todo

13f9091

labkey-ankurj requested a review from cnathe September 22, 2020 22:38

labkey-nicka and others added 3 commits September 22, 2020 16:47

Code review feedback

c95cb73

Merge branch 'develop' into fb_ErrorPage_7767

97c5eed

flip impersonating user and logged in user info

9b19060

cnathe requested a review from labkey-nicka September 23, 2020 21:20

handle the case when error page is displayed without an actual throwa…

ac44ac9

…ble object

labkey-ankurj mentioned this pull request Sep 24, 2020

Item 7767: Render new error page LabKey/LabDevKitModules#54

Merged

cnathe requested changes Sep 24, 2020

View reviewed changes

labkey-ankurj and others added 8 commits September 25, 2020 07:14

Merge branch 'develop' into fb_ErrorPage_7767

cef1097

fix string in jsp errors

ec06582

Merge branch 'develop' into fb_ErrorPage_7767

facc79c

code review changes - css updates, text updates, html encoding

5590f3b

update css on stop impersonating button

261b475

replace br tags with css prop

8b04770

Fix (mostly) for view details triangle pointer position issue with di…

e04fdeb

…ffering media page widths

cnathe requested changes Sep 28, 2020

View reviewed changes

labkey-nicka requested changes Sep 28, 2020

View reviewed changes

server side code review changes

cf6bef2

labkey-ankurj commented Sep 29, 2020

View reviewed changes

labkey-ankurj requested a review from labkey-nicka September 29, 2020 17:46

labkey-nicka requested changes Sep 29, 2020

View reviewed changes

labkey-ankurj added 6 commits September 29, 2020 14:23

more code review feedback changes

dd4b8c9

Merge branch 'develop' into fb_ErrorPage_7767

9cdc64b

update ui-components version to release version

1ce8c9c

jest test updates

92da00b

different subheading checks in jest tests for diff error types

868af9d

fix ClientApiTest.webDavAPITest

8e5df94

labkey-ankurj requested a review from labkey-nicka October 1, 2020 16:43

labkey-nicka reviewed Oct 1, 2020

View reviewed changes

labkey-nicka approved these changes Oct 1, 2020

View reviewed changes

cnathe approved these changes Oct 1, 2020

View reviewed changes

labkey-ankurj added 2 commits October 1, 2020 12:12

fix ClientAPITest.contentTypeResponseTest

37d6af1

test text for impersonating user in permission error page

dbb4194

labkey-ankurj requested a review from labkey-jeckels October 1, 2020 20:49

labkey-jeckels reviewed Oct 1, 2020

View reviewed changes

labkey-ankurj added 5 commits October 2, 2020 12:45

fix SecurityTest

d404836

check if there are config exceptions during startup

006e67b

revised wordings

d23bccf

Merge branch 'develop' into fb_ErrorPage_7767

19ae4c1

revised wordings update

44801fc

labkey-ankurj mentioned this pull request Oct 5, 2020

Item 7767: Automated test updates for the new error page LabKey/testAutomation#491

Merged

labkey-ankurj and others added 6 commits October 6, 2020 15:49

drop the extension to refer lib.xml in prod builds

36343bb

add message to title

ce38e75

fix multiple Daily failures

7bcda53

Merge branch 'develop' into fb_ErrorPage_7767

febb96a

Request library definitions from root container

9f6f09c

Merge branch 'develop' into fb_ErrorPage_7767

1206d89

labkey-ankurj merged commit 6f95e4d into develop Oct 9, 2020

labkey-ankurj deleted the fb_ErrorPage_7767 branch October 9, 2020 18:05

Item 7767: View details section of ErrorPage Redesign - Story 2 #1589

Item 7767: View details section of ErrorPage Redesign - Story 2 #1589

Uh oh!

Conversation

labkey-ankurj commented Sep 22, 2020 • edited by labkey-nicka Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Related Pull Requests

Changes

Uh oh!

cnathe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labkey-ankurj Sep 24, 2020 • 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!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

labkey-nicka commented Sep 28, 2020

Uh oh!

labkey-nicka left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

labkey-nicka left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

labkey-ankurj commented Sep 22, 2020 •

edited by labkey-nicka

Loading

labkey-ankurj Sep 24, 2020 •

edited

Loading