RA-1889:Return Relatively Formatted Error Instead Of Returning Full Exception #43
Conversation
sherrif10
changed the title
| 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); |
There was a problem hiding this comment.
@sherrif10 already here the form be null , why then add a null to the model ??
There was a problem hiding this comment.
Thanks @mozzy11 , Wanted to return a model rathan null, let me re-correct it
| 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()); |
There was a problem hiding this comment.
i gues this should take in a message code ,
There was a problem hiding this comment.
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
| <html> | ||
| <body> | ||
| <p>Application has encountered an error. Please contact support on ...</p> | ||
| <% out << " ooppsss!!!! Active drugs cannot be edittted!" %> |
There was a problem hiding this comment.
what is this for ?? and how do you expect it to work anyway??
There was a problem hiding this comment.
This should ideally be returned as an error page like returned in the controller class
There was a problem hiding this comment.
Yes, instead of "active drugs cannot be editted", don't we want to just return the message below?
There was a problem hiding this comment.
Sure thanks have collected the error return message
There was a problem hiding this comment.
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.
|
@mogoodrich i dont think its very necesary to return a full page , Why not just a simple ui message ?? |
|
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 |
| <html> | ||
| <body> | ||
| <p>Application has encountered an error. Please contact support on ...</p> | ||
| <% out << " ooppsss!!!! Active drugs cannot be edittted!" %> |
There was a problem hiding this comment.
Yes, instead of "active drugs cannot be editted", don't we want to just return the message below?
| if (htmlForm == null) { | ||
| throw new IllegalArgumentException("encounter.form is not an HTML Form: " + encounter.getForm()); | ||
| log.warn("Active drugs are cannot be editted"); |
There was a problem hiding this comment.
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.
|
|
||
| @ExceptionHandler(value= NullPointerException.class) | ||
| public String HandleNullPointerException(Exception e) { | ||
| log.warn("encounter.form is not an HTML Form"); |
There was a problem hiding this comment.
doesn't this catch any Null Pointer Exception? How do we know that the "encounter.form" NPE was the one that caused it?
There was a problem hiding this comment.
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
| <html> | ||
| <body> | ||
| <p>Application has encountered an error. Please contact support on ...</p> | ||
| <% out << "encounter.form is not an HTML Form" %> |
There was a problem hiding this comment.
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 }
There was a problem hiding this comment.
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.
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