Ajaxified Adding Feeds. Put some validity checks around uploaded urls. W... #277

Merged
merged 1 commit into from May 3, 2012
View
22 app/controllers/RSSFeeds.java
@@ -1,5 +1,7 @@
package controllers;
+import org.hibernate.exception.ConstraintViolationException;
+
import java.util.*;
import org.w3c.dom.Document;
@@ -15,26 +17,28 @@
public class RSSFeeds extends OBController {
public static void addFeed(String url) {
- if(!url.equals("")){
+ if(url != null && !url.trim().equals("")){
User u = user();
- RSSFeed feed = new RSSFeed(u, url);
- // Add Unique Constraint Check
- feed.save();
+ RSSFeed feed = new RSSFeed(u, url.trim());
+ if (feed.is_valid) {
+ try {
+ feed.save();
+ renderJSON("1");
+ } catch (ConstraintViolationException e) {
@amshali
Collaborator
amshali added a note May 3, 2012

Exception is not handled! Why do you even need to catch this exception. It must not happen.

It does happen. Code falls through and returns a 0 in the event of an error.

Is this considered bad coding style? I could see how it may be considered obfuscated

@ProCynic
Collaborator
ProCynic added a note May 5, 2012

should use error() method to return a http 500 instead.

but it isn't an internal server error, which is what a 500 response indicates to my knowledge. Whenever I've seen an 500 error, it typically indicates some kind of server fault.

Is there a precedent for this, or a reason I should change it? I could send a 500, but would the interpretation of it in the javascript code be any less arbitrary than receiving a 1 or a 0? I've looked through a few of the controllers and I don't see error being used much -- if at all.

@ProCynic
Collaborator
ProCynic added a note May 5, 2012

Depends on the error. If the problem is that the request didn't pass validation, you want to send a 400 bad request error I think. Unfortunately play doesn't have a handy method for that, so you'd have to set the response header manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ }
+ }
}
-
- redirect("/");
+ renderJSON("0");
@amshali
Collaborator
amshali added a note May 3, 2012

What is 0?

@ProCynic
Collaborator
ProCynic added a note May 3, 2012

Is this a modified or not thing? If it is, you'd do better to use ok() and notModified()

304 error : This does not really indicate an error, but rather indicates that the resource for the requested URL has not changed since last accessed or cached.

Is 304 really the correct error to use here? I'm not aware of any codes, and infact, I did look through an abridged list, before I decided to return this *arbitrary string

@ProCynic
Collaborator
ProCynic added a note May 5, 2012

error() for error, ok() for successful change, notModified() for idempotent operation that did not change anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
public static void RSSfeeds(Long userId) {
User current = User.findById(userId);
if (current != null){
-
List<RSSFeed> feeds = new ArrayList<RSSFeed>();
-
for(RSSFeed f : current.feeds) {
+ if (!f.is_valid) continue;
feeds.add(f);
}
-
render(feeds);
}
}
View
2 app/views/Posts/post.json
@@ -5,4 +5,4 @@
"content" : "${post.content}",
"creationdate" : "${post.createdAt}",
"updatedate": "${post.updatedAt}"
-}
+}
View
0 app/views/RSSfeeds/RSSfeeds.html → app/views/RSSFeeds/RSSfeeds.html
File renamed without changes.
View
20 app/views/tags/rhsNav.html
@@ -3,12 +3,26 @@
<li>
<button id="showDivRSS" onclick ="ShowHide('RSSiFrame')" style="DISPLAY: none"><i>Hide</i> <i class="icon-chevron-up"></i></button>
<button id="hideDivRSS" onclick ="ShowHide('RSSiFrame')" style="DISPLAY: block"><i>Show</i> <i class="icon-chevron-down"></i></button>
- <iframe style="DISPLAY: none;border: none;'" id="RSSiFrame" src="@{RSSFeeds.RSSfeeds(_user.id)}" width="100%" height="100%" scrolling = "auto" align="middle" marginheight="0" marginwidth="0" hspace="5" vaspace="5" frameborder="0">
+ <iframe style="DISPLAY: none;border: none;" id="RSSiFrame" src="@{RSSFeeds.RSSfeeds(_user.id)}" width="100%" height="100%" scrolling = "auto" align="middle" marginheight="0" marginwidth="0" hspace="5" vaspace="5" frameborder="0">
</iframe>
- #{form action:@RSSFeeds.addFeed(), method:'GET', style:'display:inline'}
- <input type='text' name='url'> </input>
+ #{form action:@RSSFeeds.addFeed(), method:'GET', style:'display:inline', id:'RSSAddForm'}
+ <input type='text' name='url' style="width:100%"></input>
<input type='submit' value='Add Feed URL'></input>
#{/form}
+ <script type="text/javascript">
+ $('#RSSAddForm').submit(function() {
+ var form = $(this);
+ $.getJSON(form.attr('action'), form.serialize(), function(data){
+ if (data=='0') {
+ alert("'"+form.find('input[type="text"]').val()+"' is not a valid RSS url!");
@amshali
Collaborator
amshali added a note May 3, 2012

What is the alert?

@ProCynic
Collaborator
ProCynic added a note May 3, 2012

looks like it pops on failed validation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ } else
+ if (data=='1') {
+ $('#RSSiFrame').attr('src', $('#RSSiFrame').attr('src'));
+ }
+ });
+ return false;
+ });
+ </script>
</li>
<li class="nav-header">Upcoming Events</li>