Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow markdown rendering to handle no-ops #139

Closed
bunsenmcdubbs opened this issue Jul 5, 2016 · 6 comments
Closed

Allow markdown rendering to handle no-ops #139

bunsenmcdubbs opened this issue Jul 5, 2016 · 6 comments

Comments

@bunsenmcdubbs
Copy link
Contributor

bunsenmcdubbs commented Jul 5, 2016

When creating new events, the client sends off an errant request to the server to render markdown that isn't yet there (sends a request directly to http://localhost:8080/admin/api/utils/render-commonmark)

GET /admin/api/utils/render-commonmark

This causes an error on the server because the endpoint expects an query parameter text.

00:41:04.854 [qtp51376124-21] ERROR alfio.controller.ControllerExceptionHandler - unexpected exception
org.springframework.web.bind.MissingServletRequestParameterException: Required String parameter 'text' is not present

This should be a simple fix on both client and server side. I can take care of it in a PR.

@cbellone
Copy link
Member

cbellone commented Jul 5, 2016

thank you 👍

@cbellone cbellone added this to the 1.8 milestone Jul 5, 2016
bunsenmcdubbs added a commit to bunsenmcdubbs/alf.io that referenced this issue Jul 5, 2016
@bunsenmcdubbs
Copy link
Contributor Author

I decided to leave the server alone because it makes sense to fail when there is no text param. Do you think I should make the text parameter optional and return an empty string when it isn't provided? @cbellone

bunsenmcdubbs added a commit to bunsenmcdubbs/alf.io that referenced this issue Jul 6, 2016
@bunsenmcdubbs
Copy link
Contributor Author

Pressing the "Preview" button in the displayCommonmarkPreview directive will have nothing show up when there is no text (provided to the directive -- there maybe text in the input but ng-minlength can eliminate any input).

I added the default value for the renderCommonmark endpoint in the previous commit (929b246) but I'm not sure if that is actually good. On second thought it might be better to have the API "fail fast" when there is no text parameter provided at all (different from an empty string text parameter which should always work regardless).

@cbellone
Copy link
Member

cbellone commented Jul 6, 2016

Hi @bunsenmcdubbs,

I would suggest the following:

  1. intercept the exception at Spring controller level, see https://github.com/exteso/alf.io/blob/master/src/main/java/alfio/controller/api/AttendeeApiController.java#L56 (the exception to be caught here could be org.springframework.web.bind.MissingServletRequestParameterException) and return an HTTP 400 (bad request) status. That would be nice to have.
  2. add a check in the Angular directive https://github.com/exteso/alf.io/blob/master/src/main/webapp/resources/js/admin/directive/admin-directive.js#L560 in order to call the server API only if needed (and doing nothing otherwise)

how does it sound?

bunsenmcdubbs added a commit to bunsenmcdubbs/alf.io that referenced this issue Jul 7, 2016
@bunsenmcdubbs
Copy link
Contributor Author

Sounds good. I created PR #141

syjer added a commit that referenced this issue Jul 11, 2016
@syjer
Copy link
Member

syjer commented Jul 11, 2016

merged! thank you!

@syjer syjer closed this as completed Jul 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants