Microtask 1 #1

yashasingh · 2017-10-03T20:19:57Z

@tgr Please review

tgr

This PR doesn't contain most of the changes. You can always make a PR from the master into a new branch (instead of the other way around) if you don't want to rewrite master's history.

yashasingh · 2017-10-06T08:54:26Z

@tgr Sir, I have updated the PR to contain all the changes.

tgr · 2017-10-06T10:28:01Z

App/templates/App/index.html

+	    	Comment = {{x.comment}} <br>
+	    	Size = {{x.size}} <br>
+
+	    	<a href="https://en.wikipedia.org/w/index.php?title=User:{{x.user}}&diff=prev&oldid={{x.revid}}">


Technically, that's not valid HTML (which is why Github shows it in red); special characters like & or " need to be encoded in attributes. So it should be {{x.user}}&diff. In practice, it works anyway and no one really cares about, so feel free to ignore :) just mentioning for sake of completeness.

Rectified .

tgr · 2017-10-06T10:38:03Z

App/templates/App/index.html

+	    {% if userid %}
+	    	Username = {{username}} <br>
+	    	Userid = {{userid}} <br>
+	    	<a href="https://phabricator.wikimedia.org/p/{{username}}/"> Profile link </a>


The Phabricator username is not typically the same as the wiki username.

Alright. I added that link to augment to the information. Since I just got to know that phabricator and wiki usernames are not same, I'd rather remove this link. Thank you .

tgr · 2017-10-06T10:39:47Z

App/views.py

+	if(request.method=='POST'):
+		usr = request.POST['username']  #Here, we get the useranme fetched from html page
+		URL = "https://en.wikipedia.org/w/api.php?action=query&format=json&list=usercontribs&ucuser="
+		URL = URL + usr


What it the username contains a &?

Handled the situation. 👍

tgr · 2017-10-06T10:41:41Z

App/views.py

+		URL = "https://en.wikipedia.org/w/api.php?action=query&format=json&list=usercontribs&ucuser="
+		URL = URL + usr
+		r = requests.get(URL)  #Recieved data in json-format
+		lst = json.loads(r.text)


The request might fail (e.g. the API is down and responds with HTTP 503), in which case this won't work.

Ok. Will handle this, when response code is other than 200.

tgr · 2017-10-06T10:42:37Z

App/views.py

+		URL = URL + usr
+		r = requests.get(URL)  #Recieved data in json-format
+		lst = json.loads(r.text)
+		sub_lst = lst['query']['usercontribs'] #Contribution data extracted


The API might respond with an error, in which case this key might not exist. (You can test with a username containing <.)

tgr · 2017-10-06T10:43:41Z

App/views.py

+			fetched_user = sub_lst[0]['user']
+			fetched_user_id = sub_lst[0]['userid']
+		except:
+			fetched_user = fetched_user_id = 'No user found'


Or possibly the user was found but has no edits.

okay , this was a loophole, will handle it 👍

yashasingh · 2017-10-06T21:12:09Z

@tgr I have made the required changes . Please review.

tgr · 2017-10-06T22:15:20Z

App/views.py

+            lst = json.loads(r.text)
+            try:    
+                if(lst['error']):
+                    return(render(request,'App/index.html',{"error":'INVALID INPUT'}))


There might be other reasons for getting an API. You probably want to show the user the error message returned by the API instead.

tgr · 2017-10-06T22:20:36Z

App/views.py

+def main(request):
+    if(request.method=='POST'):
+        usr = request.POST['username']  #Here, we get the username fetched from html page
+        usr = usr.replace("&","%26")


But now the username will look weird when you print it out :)

Best practice is to sanitize close to the input, escape close to the output. (Sanitize = reject invalid input. Here the API takes care of that so you don't have to. Escape = convert the variable to a safe format. "safe" depends on context (you escape differently for text in a HTML document, for an URL part, for a Javascript variable...) so it has to be done when you already know how you'll use it; ideally in the template. Most templating languages (including Django templates) support that via some special notation.

I have tried to make the variable safe, by escaping &. Is It ok?

tgr · 2017-10-06T23:44:09Z

App/views.py

@@ -6,7 +6,7 @@
 def main(request):
    if(request.method=='POST'):
        usr = request.POST['username']  #Here, we get the username fetched from html page
-        usr = usr.replace("&","%26")
+        usr = usr.replace("&","")


That will just break the URL.

Sir so should I consider input containing "&" as invalid , not deal with trying correct the handle ? Please tell . Currently , if I enter a username ey bd&808 , it converts to bd808 and displays the result.

bd&808 is a different (valid) username though so that would prevent that person from using your tool. The percent-encoding approach wasn't wrong, you just did it at the wrong place (also, no point in limiting it to just &). See my earlier comment - you should do escaping in the template, and use whatever tools the templating language provides.

Sir, in django templating, escaping is done by default on. Now, I have used 'safe' filter in my html file , so the username will not be auto-escaped. Am I going right now ?

yashasingh · 2017-10-08T13:25:58Z

@tgr Please review this PR.

tgr · 2017-10-08T19:34:43Z

App/templates/App/index.html

@@ -19,7 +19,7 @@
 	    	{{error}}
 	    {% endif %}
 	    {% if username %}
-	    	Username = {{username}} <br>
+	    	Username = {{username | safe}} <br>


This turns off escaping. There is no reason to do that and it will result in some usernames looking wrong (if they contain a HTML entity). It could also be a security hole although here it probably won't be since MediaWiki disallows <> in usernames.

Default autoescaping works well when you just want to display text. Where it doesn't work is when you want use a variable in an URL or a Javascript variable or any other context where the escaping/encoding rules are different from that of HTML.

True, I understood not this thing in the first instance. Marking it safe is not the solution. So I have handled this now, using urlencoding. Now, the code works for all possible usernames else displays the error msg.

yashasingh · 2017-10-11T15:09:15Z

@tgr Sir, Please review, sorry for delay.

…development

tgr

Looks quite nice! The edit links use the wrong title parameter, but MediaWiki ignores the title anyway when oldid is set, so it doesn't matter much.

tgr · 2017-10-18T07:10:04Z

App/views.py

+            return render(request, "App/index.html", {"error": "Please enter a username"})
+
+        # if username is not blank
+        params = {'ucuser': username}


It would be more readable to put the constant parameters here as well.

yashasingh added 2 commits October 4, 2017 01:47

files to be reviewed

d7b2d0e

task 2 for review

5ad9407

tgr reviewed Oct 6, 2017

View reviewed changes

yashasingh added 3 commits October 6, 2017 13:52

Incomplete file deleted

cc7ea89

changes complete

8133e9a

conflict resolved

6dd4900

conflict resolved

ef6bda9

tgr suggested changes Oct 6, 2017

View reviewed changes

yashasingh added 3 commits October 6, 2017 17:39

review changes implemented

8c861d6

Sanitized database query

7bbf540

bug fixes done

db47853

yashasingh added 2 commits October 7, 2017 02:44

minor bugs fixed

97357e5

minor bugs fixed

3553348

tgr suggested changes Oct 6, 2017

View reviewed changes

minor fixes

863165f

tgr reviewed Oct 6, 2017

View reviewed changes

yashasingh added 3 commits October 8, 2017 17:42

issues fixed

2f5e74d

issues fixed

efcde18

issues fixed

a613a35

tgr suggested changes Oct 8, 2017

View reviewed changes

yashasingh added 7 commits October 9, 2017 03:34

issues resolved

f5c41a7

percentile issue resolved

8969f0c

merged code

fee5636

conflict resolved

c9e8057

minor fixes

7659411

code cleaned

66399e5

merged task1

1581136

yashasingh added 3 commits October 9, 2017 12:32

Merge branch 'devtask2' into complete

8df73a2

minor fixes

a8c655b

code made deployable

4c4a59f

yashasingh changed the title ~~files to be reviewed~~ Microtask 1 Oct 9, 2017

yashasingh added 3 commits October 11, 2017 19:10

UI updated

a7fea46

minor fixes

f3d082f

code updated

75a14b6

yashasingh added 4 commits October 12, 2017 11:54

pyc files ignored

0edfcac

merged master

cb49836

Merge branch 'complete' of https://github.com/yashasingh/Review into …

6e7a318

…development

minor fix

7f5fbe8

tgr approved these changes Oct 18, 2017

View reviewed changes

Minor changes

81cd71a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Microtask 1 #1

Microtask 1 #1

yashasingh commented Oct 3, 2017

tgr left a comment

yashasingh commented Oct 6, 2017

tgr Oct 6, 2017

yashasingh Oct 6, 2017

tgr Oct 6, 2017

yashasingh Oct 6, 2017

tgr Oct 6, 2017

yashasingh Oct 6, 2017

tgr Oct 6, 2017

yashasingh Oct 6, 2017

yashasingh Oct 6, 2017

tgr Oct 6, 2017

yashasingh Oct 6, 2017

tgr Oct 6, 2017

yashasingh Oct 6, 2017

yashasingh commented Oct 6, 2017

tgr Oct 6, 2017

yashasingh Oct 6, 2017

tgr Oct 6, 2017

yashasingh Oct 6, 2017

tgr Oct 6, 2017

yashasingh Oct 8, 2017

tgr Oct 8, 2017

yashasingh Oct 8, 2017

yashasingh commented Oct 8, 2017

tgr Oct 8, 2017

yashasingh Oct 14, 2017

yashasingh commented Oct 11, 2017

tgr left a comment

tgr Oct 18, 2017

yashasingh Nov 7, 2017

Microtask 1 #1

Are you sure you want to change the base?

Microtask 1 #1

Conversation

yashasingh commented Oct 3, 2017

tgr left a comment

Choose a reason for hiding this comment

yashasingh commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashasingh commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashasingh commented Oct 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashasingh commented Oct 11, 2017

tgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment