RA-1889:Return Relatively Formatted Error Instead Of Returning Full Exception by sherrif10 · Pull Request #43 · openmrs/openmrs-module-htmlformentryui

sherrif10 · 2021-02-10T16:21:27Z

https://issues.openmrs.org/browse/RA-1889
cc @dkayiwa @ibacher @mozzy11 kindly review thanks
WHAT I DID
Added a groovy page that will be returned
Added a NullPointerclass to handle exception
Still Under Testing

mozzy11 · 2021-02-10T21:22:40Z

...module/htmlformentryui/page/controller/htmlform/ViewEncounterWithHtmlFormPageController.java

-            throw new IllegalArgumentException("encounter.form is not an HTML Form: " + encounter.getForm());
+             message = messageSourceService.getMessage("encounter.form is not an HTML Form" +encounter.getForm());
+            log.warn("Active drugs are cannot be editted");
+            model.addAttribute("htmlForm", htmlForm);


@sherrif10 already here the form be null , why then add a null to the model ??

Thanks @mozzy11 , Wanted to return a model rathan null, let me re-correct it

mozzy11 · 2021-02-10T21:23:25Z

...module/htmlformentryui/page/controller/htmlform/ViewEncounterWithHtmlFormPageController.java

+
        if (htmlForm == null) {
-            throw new IllegalArgumentException("encounter.form is not an HTML Form: " + encounter.getForm());
+             message = messageSourceService.getMessage("encounter.form is not an HTML Form" +encounter.getForm());


i gues this should take in a message code ,

It depends, it takes in both Actually message codes would be also another good idea since we are trying to hide the exception hence returning a readable message format

mozzy11 · 2021-02-10T21:24:31Z

omod/src/main/webapp/pages/htmlform/NullPointerException.gsp

+<html>
+   <body>
+   <p>Application has encountered an error. Please contact support on ...</p>
+     <% out << " ooppsss!!!! Active drugs cannot be edittted!" %>


what is this for ?? and how do you expect it to work anyway??

This should ideally be returned as an error page like returned in the controller class

Yes, instead of "active drugs cannot be editted", don't we want to just return the message below?

Sure thanks have collected the error return message

mogoodrich

I'm fine with the idea of returning a "nicer" error page, but I don't think this is the right way to go about it?

Also, I'm not sure if the fact that encounter.form is null is due to the fact one is trying to edit active drugs? It certainly isn't the only way this error could occur?

I added some comments on the ticket as well.

mozzy11 · 2021-02-11T05:07:35Z

@mogoodrich i dont think its very necesary to return a full page , Why not just a simple ui message ??

sherrif10 · 2021-02-11T06:23:57Z

Thanks @mogoodrich @mozzy11 sure simple ui message would ideally be worth it , though havent seen any ui error message example , feel free to share with me any example i also agree that my code need to be refactored.One more thing to consider is to use exceptionHandlers which can handle exceptions

mogoodrich · 2021-02-11T13:59:01Z

omod/src/main/webapp/pages/htmlform/NullPointerException.gsp

+<html>
+   <body>
+   <p>Application has encountered an error. Please contact support on ...</p>
+     <% out << " ooppsss!!!! Active drugs cannot be edittted!" %>


Yes, instead of "active drugs cannot be editted", don't we want to just return the message below?

mogoodrich · 2021-02-11T13:59:32Z

...module/htmlformentryui/page/controller/htmlform/ViewEncounterWithHtmlFormPageController.java

+
        if (htmlForm == null) {
-            throw new IllegalArgumentException("encounter.form is not an HTML Form: " + encounter.getForm());
+        	 log.warn("Active drugs are cannot be editted");


shouldn't this be "encounter.form is not an HTML Form"? that's the real error here, not that active drugs can't be edited.

Resolved thanks

mogoodrich · 2021-02-11T14:00:05Z

...module/htmlformentryui/page/controller/htmlform/ViewEncounterWithHtmlFormPageController.java

+
+     @ExceptionHandler(value= NullPointerException.class)
+     public String HandleNullPointerException(Exception e) {
+    	  log.warn("encounter.form is not an HTML Form");


doesn't this catch any Null Pointer Exception? How do we know that the "encounter.form" NPE was the one that caused it?

Have changed it to illegalArgumentException, this method would handle any exception that would occur when any api with in the controller class returns an exception is called

HerbertYiga · 2021-02-15T21:13:51Z

omod/src/main/webapp/pages/htmlform/IllegalArgumentException.gsp

+<html>
+   <body>
+   <p>Application has encountered an error. Please contact support on ...</p>
+     <% out << "encounter.form is not an HTML Form" %>


is this the error you are returning? down your controller ,add the error message to an attribute for example errorMessage and access it from the gsp as ${errorMessage }

thanks @HerbertYiga , ,Am not well convinced with this approach and am sure there is a clean way of doing this which am looking forward too.

sherrif10 added 2 commits February 10, 2021 19:18

RA-1889:Return Relativively Formatted Error Message

37aa36a

Collecting the exception handler

6c7e64a

sherrif10 marked this pull request as ready for review February 10, 2021 20:39

sherrif10 changed the title ~~RA-1889:Return Relatively Formatted Error Message~~ RA-1889:Return Relatively Formatted Error Instead Of Returning Full Exception Feb 10, 2021

mozzy11 reviewed Feb 10, 2021

View reviewed changes

adding error view page

64ffe44

sherrif10 force-pushed the RA-1889 branch from 58b72f7 to 64ffe44 Compare February 10, 2021 21:36

mozzy11 reviewed Feb 10, 2021

View reviewed changes

Using ArrayList

3302488

mogoodrich requested changes Feb 10, 2021

View reviewed changes

mogoodrich requested changes Feb 11, 2021

View reviewed changes

Collecting the error message

85a1720

HerbertYiga reviewed Feb 15, 2021

View reviewed changes

sherrif10 mentioned this pull request Feb 16, 2021

Return An error page digitalhealthcaresociety/openmrs-module-coreapps#1

Open

dkayiwa force-pushed the master branch from 7adc9ee to b16b149 Compare June 22, 2021 13:13

Conversation

sherrif10 commented Feb 10, 2021 • edited 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!

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!

mogoodrich left a comment

Choose a reason for hiding this comment

Uh oh!

mozzy11 commented Feb 11, 2021

Uh oh!

sherrif10 commented Feb 11, 2021

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!

sherrif10 Feb 11, 2021 • 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!

HerbertYiga Feb 15, 2021 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sherrif10 commented Feb 10, 2021 •

edited

Loading

sherrif10 Feb 11, 2021 •

edited

Loading

HerbertYiga Feb 15, 2021 •

edited

Loading