Already shortened url message fixed #38
Conversation
pygmyui/pygmy/views.py
Outdated
| err = {} | ||
| err["error"] = (" ".join(API_ERROR(e.args[0])['error']) | ||
| if isinstance(API_ERROR(e.args[0])['error'],list) | ||
| else API_ERROR(e.args[0])['error']) |
There was a problem hiding this comment.
Good find. This PR will fix the issue but I reckon this same issue might occur for some other endpoints also and that would require us to copy this code there. I think we should fix the source of the problem. This https://github.com/amitt001/pygmy/blob/master/pygmy/rest/shorturl.py#L44 is returning a dict {'error': ['URL is already a pygmy shortened link.'], 'long_url': ['URL is already a pygmy shortened link.']} with a list as the value. This should be a string. Would you mind updating the PR to fix this?
There was a problem hiding this comment.
I have changed code, and convert error message to string in API part. But a question came to my mind, isn't it possible to have multiple error in the same time? If it possible, is converting to string good idea?
There was a problem hiding this comment.
Thanks!
yes, it's possible to have multiple errors at the same time but not with how this webapp works. All form validation is already done on the Django app side and this is the only check that's performed in the backed.
But you are right that it may not be the best idea to join all errors if API is used independently of the webapp. One way, I think, this issue can be solved is by keep on passing error list and on webapp side(frontend or Django app) format it in the form that it shows all the errors properly.
Frontend already supports this for form validation
|
Instead of joining API errors, i have changed the error_msg.py file which getting errors from API and convert dict for rendering, it now return fully lists instead of list&string mix and because of that i also changed some(400.html,404.html,500.html) templates.Instead of printing error directly i've added for loops for printing elements of list. |
pygmy/rest/shorturl.py
Outdated
| @@ -44,7 +44,7 @@ def post(self): | |||
| if errors: | |||
| log.error('Error in the request payload %s', errors) | |||
| if errors.get('long_url'): | |||
| errors.update({'error': errors.get('long_url')}) | |||
| errors.update({'error': " ".join(errors.get('long_url'))}) | |||
There was a problem hiding this comment.
I think we can get rid of this. This is not required anymore after changes to the template.
There was a problem hiding this comment.
Oops. Sorry, i forgot to remove join.
|
Good job! Thanks for the PR :) |
I think it(#37 ) happens because of some errors return a list instead of string, so i convert this list to string if it is list.