Comments

[ksurya]

Comments for SciPy Central built over django.contrib.comments

1. Asynchronous comment submission using jQuery
2. support for flagging comments - email is sent to CommentModerators user group
3. Rss Feed of latest 40 comments in the site
4. Rss Feed of comments for individual revision object
5. Gravatar support

Note:

1. django.contrib.comments.views have to be overridden for Ajax support
2. Reputation for comments is attached in the Thumbs branch. It has to be updated once this is merged!

[lomegor]

Few things to fix before I look at the code:

1. No way to delete or edit comments. That is very important and should be available from the start.
2. The help text in the comment input should be similar to that in the description of submissions, with the examples and all.
3. The preview should be better delimited, maybe with a dotted border and a grey background. Something to make it different than the rest of the text. It should also have a small title if possible.
4. When a comment is flagged, instead of telling it in the lightbox, it would be better to change the 'flag' link to something like 'You have flagged this comment' in bold.
5. permalink links to scipy-central and not the current domain.
6. permalink doesn't take me to the comment as far as I can see, just to the entry.
7. I can click in the image of the commenter, but nothing happens. It should either take me to that user profile or a permalink to the comment.
8. I can't click on the username of the commenter to take me to the profile page.
9. Sign in or register looks weird on the comment section. Either make it text-only, or make the buttons the correct size, and not so small. Also, lose the bold for that, I believe.
10. Can't test yet comments in search page because of the bug I told you (I'm not sure if it's a problem with my db or a problem with the app yet).

Some suggestions:

1. Way to unflag a comment.
2. What happens when a comment is flagged? How can a moderator see it? How can I make myself a moderator to see that? Is there an interface for that? What can moderators do? They just have to delete it from the database? (I think the easier way is just to have more links in the comments itself for the moderators to delete any comment, and see the amount of flags).
3. Maybe I should ask this on the list (or you can do it), but should the submitter of the entry be able to moderate the comments on their entries?
4. Do you think it's possible (and easy) to add replies to comments?
5. Add comment count and a link to comments in the submissions list. The link to comments should scroll the page directly to the comments (with a title in the anchor).
6. Add a comment section to the profile page.

I think that's it for now. After you fixed this (feel free to rebase to make history prettier with all these changes since I haven't looked at commits and code yet), I'll see if there's anything more and I will look at the code itself.

[ksurya]

Sorry, I totally forgot about delete and edit features.. Its unusal but it happened! I added those features will push them soon. Regarding others things you mentioned

1. unflag is something I feel not required! Once its flagged, an email is sent to all Moderators in CommentModerators User group who have moderate comment permissions. Regarding the total number of flags for a commit - Sorry, it did not occur to me at the first place. As far as I know there is no such field in django that provides (have to look into docs again).

The whole reason why I used a lightbox for flag is that, I want to provide users to enter a message before they are flagging! I don't know if I will be able to do it during GSoC as I feel there are some more high priority issues :) Possibly, we can create a nice interface for moderators too by then.. more ideas popping up now.. didn't occur to my mind while writing proposal. However, this is a something I am anyway going to do after the GSoC period..

1. The submission list on the main page (I don't know if you meant that), has a envelope-icon pointing to comments list of every submission
2. Replies to comment --feature, putting them as sub-comments, I don't think its really needed.. But tagging other users in the site is something interesting. I don't think its right time for this because, we need a good notification mechanism.. some users want to get email, some want to see as a simple notification on the website. I think we need to build notification feature first for the site users and moderators in proper way, then, go on with this stuff.. And this is again big work to put in GSoC timeline since its not present
3. Providing submitters to moderate comments on their data is something unusual and probably not required. Users should be free enough to comment within the general rules of the site. People can anytime flag and get it removed.. (Its just my personal opinion though)

The remaining stuff like hightlight preview, link to user profile, etc stuff, I will fix them and push!

[pv]

Overall, this looks very good to me.

There's one issue though: when does the is_public flag on the comments turn on? It seems I can write comments fine, but they are not marked public. Perhaps this should be done when the comment is created?

[pv]

A final thing (not for this PR): scipy-central needs tests. Testing all the feature set manually is getting somewhat cumbersome.

It's possible to write tests using the Django test framework that emulate using the site with a browser. Doesn't run JS, but you can emulate the AJAX calls directly. Having a number of such tests for core functionalities of the site would be useful for the long term.

[ksurya]

@lomegor I have made all the changes - can you please look into it. Please let me know if I forgot any! @pv

[pv]

Hi: some remaining issues

• When editing comment, the screen stays darkened out after pressing 'submit'.
• The Delete and Flag buttons still stay visible after they are pressed.
• Some files are still indented with 2 spaces instead of 4, please run reindent on them.

Otherwise, seems to work nicely!

[ksurya]

Thanks for pointing out, I didn't observe it at all! I just checked it and found that

$('#spc-comment-list').load(' ' + '#spc-comment-list'); to reload comment list is causing it surprisingly.

I will hide those delete and flag buttons upon successful submission

[ksurya]

@lomegor its fixed now. Instead of loading the comment list again after every edit, I am just editing the comment on the client side itself (after receiving the data from server). I think this is simplest possible way..

The 'delete' and 'flag' buttons hide after successful form submission! In case of 'edit', I didn't do this because, user might want to edit it again and again!!

[lomegor]

Cool. I'm going to take a look at all of thus later. On 17 Jul 2013 21:39, "Surya Kasturi" notifications@github.com wrote:

@lomegor https://github.com/lomegor its fixed now. Instead of loading the comment list again after every edit, I am just editing the comment on the client side itself (after receiving the data from server). I think this is simplest possible way..

— Reply to this email directly or view it on GitHubhttps://github.com/scipy/SciPyCentral/pull/131#issuecomment-21143034 .

[lomegor]

Things to fix (still haven't looked at the code since it can change a lot):

Few things to fix before I look at the code:

1. The help text in the comment input should be similar to that in the description of submissions, with the examples and all.
2. When a comment is flagged, it's not necessary to show another lightbox telling the user it's flagged.
3. When a comment is flagged, I can flag it again if I refresh the page (could end up being a lot of spam for the admins).
4. permalink links to scipy-central and not the current domain. (not sure why)
5. permalink doesn't take me to the comment as far as I can see, just to the entry.
6. The username of the commenter looks like another link. It should say something like "By lomegor" to make it look different, or it should have different style than the other links.
7. Search is still not working for me. Is it working correctly for you? I deleted the database and started from scratch and I'm still having problems. Can you test doing that (starting from zero in another folder or branch)?
8. When I flag a comment the title of the lightbox should be "Are you sure you want to flag this comment?" instead of "Do you really want to flag this comment?". It looks less aggressive that way.
9. When a comment has been edited, the lightbox should close, instead of telling the user it was successful. The user already knows it was successful since the comment change. The same thing happens with Delete.
10. There is no description when deleting a message, which makes the lightbox look weird. Maybe add something like "this process is irreversible..." blah, blah, blah.
11. Change the title of the Delete lightbox to "Are you sure..."
12. The Delete lightbox does not get cleared after deleting a comment, so if I delete two comments in a row, when it opens again it tells me that the message has been deleted (which is not true), and I cannot delete another comment. The same thing happens with the Flag lightbox.
13. After deleting a comment, the title of the comment section has the wrong number (still has the number as if the comment existed.
14. Add round borders to the preview section... as everything has borders I think it will look nicer.
15. Also, do you think it's possible to have a live preview once it's opened? That way I can see what I'm writing without having to press preview again. If it's not possible, I think it would be best to close the preview if I keep writing the comment, because it's misleading.
16. The flagged thing gets cleared if I add a new comment. (It lets me flag the comment again.)

I think that's it, but there may be other things I'm missing. I may take a look at the code while you fix these things, so try not to rebase too much. If you do end up rebasing to make the commit history look better, let me know.

Another thing to keep in mind because it's something we should do is that after login or logout, user should be redirected to the page they were in. Also, the sidebar still says Copyrights which reads weird. This is not something to do in this PR, but you can open a new one for both I believe if you have the time.

[ksurya]

1. Regarding the font of help text, I used the one which we used in 'submissions' help text. Should be have to put this a bit different from rest of theml
2. I have used lightbox for flags to explicitly mention the reasons to flag. I thought its a nice place.. Although bootstrap offers us tool-tips but I think its another JS and probably not compatible way of describing rules to all user base
3. The permalinks don't work if your Site object domain name is 'scipy-centra.org' which is by default - you can change it in /admin/
4. The permalinks are not heading to comments--Ummm, I guess, there are very few comments and ID's don't really land well in that case. Can you suggest any better solution? 5.Search is working for me! But, this PR has nothing to do with search.
5. "Are you sure you want to flag" --I dont remember but I took it from default Django's template itself..We can change that
6. Regarding the description of "delete comment", Yeah, I will add some thing :p
7. The delete lightbox has to get cleared. I made sure of it.. don't know why its not happening for you- let me check
8. The title of the comments after deleting comment should reduce by 1.. yeah, I even noticed but didn't so far found a smart way.. Probably, I should load its value as an attr, access it in Ajax, then also edit it..
9. Round borders--okay
10. Live preview -- I initially tried to use Tabs one for comments and other for preview. The onclick event for preview renders the preview (similar to the one on github).. But that didn't suit our site (don't remember but I felt the 2nd option was quite right) --Anyway, I actually want to add some fancy javascript text editor to our site which even gives us the preview. once the basic setup is done, we can actually more further.. I have been looking into some tools for that and found one :) So, I feel, putting more into previewing is not worth as its going to change anyway... Not just it, the entire editing work on site including in submissions would be changing :) thats why I didn't do this work here
11. Yeah, I even noticed this 'flagged' turning into 'flag' on refresh.. I thought I have fixed it.. Oops! forgot..I will try to find a way out..

[lomegor]

1. It's not the font I was talking about. I meant all the examples that are in the help text for the description, that are not in the help for the comments. I think it's something nice to add (and it's more help than a link to the Markdown Help page).
2. Keep the lightbox to mention the reason to flags. I was just talking about the lightbox staying after something has been flagged with a success message. I think that second part is unnecessary.

I have to check the other things. Thanks!

[ksurya]

@lomegor the changes have been made now! please take a look. The thumbs PR is also ready!

[lomegor]

1. The username of the commenter looks like another link. It should say something like "By lomegor" to make it look different, or it should have different style than the other links.
2. After deleting a comment, the title of the comment section has the wrong number (still has the number as if the comment existed. 3.This number is also wrong after posting a new comment.
3. No need to say that undo can happen by contacting the Team. I don't think it's worth it, and it may mean a number of unnecessary e-mails going to people (and a bit of overhead to keep the comments, etc in backups).
4. Add round borders to the preview section... as everything has borders I think it will look nicer.
5. I think it would be best to close the preview if I keep writing the comment, because it's misleading.
6. The editing feature is not working if a comment is flagged (many problems: lightbox not getting cleared, not completed with comment, cannot submit edit, preview showing always, etc.)
7. Editing lightbox should close after clicking submit. Now it's even working weird, since it tells me it has been edited and it shows me the preview; the lightbox should just close and the user can see the comment updated.

I'm going to look at the commits now and may ask you to rebase them or change the message. When fixing these issues, take into consideration that you can rebase and try to make the commit history look pretty. Also, when you have fixed these issues, try to go number by number and test everything I told you again; last time there were at least 4 bugs that weren't fixed, and this time there are 5 bugs that still exist from my past correction.

[lomegor]

Comments about the commits:

1. 1982690 should go inside 1ca4fdc
2. 15dcb5d should go inside whatever merited the inclusion of simplejson
3. Where did you get the image for ec929a9?

I think that's it! After you've fixed these things, I'll take a look at the code (although I may take a look sooner if I have time).

[ksurya]

Okay. quick fixes.

That image is from our repository itself. But the current update is a scaled version..

[lomegor]

@ksurya Did you read my other comment about the bugs? I'm going to copy again, because I'm not seeing any of those bugs solved.

1. The username of the commenter looks like another link. It should say something like "By lomegor" to make it look different, or it should have different style than the other links.
2. After deleting a comment, the title of the comment section has the wrong number (still has the number as if the comment existed. 3.This number is also wrong after posting a new comment.
3. No need to say that undo can happen by contacting the Team. I don't think it's worth it, and it may mean a number of unnecessary e-mails going to people (and a bit of overhead to keep the comments, etc in backups).
4. Add round borders to the preview section... as everything has borders I think it will look nicer.
5. I think it would be best to close the preview if I keep writing the comment, because it's misleading.
6. The editing feature is not working if a comment is flagged (many problems: lightbox not getting cleared, not completed with comment, cannot submit edit, preview showing always, etc.)
7. Editing lightbox should close after clicking submit. Now it's even working weird, since it tells me it has been edited and it shows me the preview; the lightbox should just close and the user can see the comment updated.

I'm going to look at the commits now and may ask you to rebase them or change the message. When fixing these issues, take into consideration that you can rebase and try to make the commit history look pretty. Also, when you have fixed these issues, try to go number by number and test everything I told you again; last time there were at least 4 bugs that weren't fixed, and this time there are 5 bugs that still exist from my past correction.

[ksurya]

@lomegor

1. merged 1&2 commits
2. edit-comment lightbox closes after successful submission
3. The title of comments section showing wrong number is fixed
4. added round borders to preview
5. unable to reproduce the bug you were mentioning at [6]
6. The author name, submit-date data is grouped into one
.. like by suryak on July 7, 2013, 3:06 a.m.. the user name is still an hyperlink

[lomegor]

1. The number in the Comments title is still wrong if I delete two comments, or add two new comments. It works fine if I just do one deletion or posting, but "breaks" when doing more than one.
2. This is extremely weird, and it can be my fault, but you may be able to take a look at it: the exclamation mark (!) on the deletion warning (Clicking delete button deletes your comment forever!), looks like a lowercase L. Maybe the font is too small? Maybe we can just remove and a dot instead of an exclamation mark?
3. Again, @ksurya, the problem I'm having is that the preview of a comment doesn't auto update when I keep writing, so it looks misleading. For example if I write "asd" and hit preview I will see "asd" in the preview, but if I change the comment to "asd asd" the preview will still say "asd" until I click preview again. That's misleading. There are three options here: have the preview as tab like in Github so you can't see the preview and the comment at the same time, or auto-update the preview everytime someone writes, or just close it when someone writes again. The last option is the worse one, but I think is the quicker to do.
4. I can't reproduce the bug on [6] either... weird. Last time I could reproduce it every time a comment was flagged when the page loaded, but I cannot do it now... maybe some other small change fixed this. Cool!
5. Nice, I like the new editing message.

I think that's about it for the bugs. I'm going to look at the code. Remember to fix 1..3

[lomegor]

So, as I say, I'm going to comment a bit about how to use Javascript and jQuery when you have this amount of functions and code and everything. I'll try to explain everything, but ask me any question if you want to know more. Using these ideas, I'll need you to recreate the Javascript code. Also, these are ordered by importance; some of them you should always keep in mind when writing Javascript.

1. Don't clutter the global namespace! Use a closure for your code. When you define a variable in Javascript outside of a function, or class, or whatever, you are creating a property of the global window object. So if I do var a = 'b' in the begining of a script, window.a will now contain 'b' and every piece of code will be able to access it; outside of any function if you use a, from now on it will mean window.a and everyone can change it. This of course means that a lot of code may have problems because they use a variable with the same name.

   The most direct case I've seen it happen in your code is when you use variables without a prepending var. For example, here. You can try it yourself. Post a comment, and you will see in the developer Console that you now have a variable window.formData with exactly what you sent. This also may happen in other parts of your code, so check it out.

   There's a more hidden way in which this can happen. As you probably know by now, functions in Javascript are first class citizens. This means (among many things) that when you declare a function you are declaring a variable itself. So, when you do function postComment, you are now creating window.postComment which is a function. Anyone can call window.postComment, and anyone can change it. So, if for some mistake you create a variable called postComment in the global namespace, you will lose the function.

   It's important to note that this isn't a security issue. Since Javascript runs in the client, you can never ever trust it, even if you are declaring variables inside a closure. But, it's important to keep our code bug free, since using the global namespace can cause undesirable and unexpected situations.

   So, I'm not sure if you know what a closure is, but you can read it here although it may be difficult to understand. The simple idea is that functions in Javascript not only reference the code that is inside the function, but also the environment in which they were created. For example: var a = 1; function b() { console.log(a) }; b() will log into the console 1. It's a bit like if everything was a method inside an object, but with more difficult ideas that we may get to later.

   How do you do a closure in the script? Easy! Just have all the code you are doing surrounded by a self executing anonymous function, i.e. (function() { code here })(). The problem now is that those variables don't exist in the window object, so you can't use inline Javascript (like onclick in the element to call them). We'll get on how we can solve this issue later. Let's now focus on an important closure for jQuery.

   As I've told you, variables in Javascript are all global, this includes $, so when you are using it, you are never sure what variable you are calling. This gets more tricky, when you start to use other frameworks together. For example, Prototype, I believe, also uses the $ variable as a function. So how can you be sure that the $ you are using (really just window.$) is jQuery? Simple, just pass the jQuery object to your anonymous function and call it $.

   So, instead of only having an anonymous closure, you are now making us of it by reassuring you that you are using jQuery when you use the $ (which let's be honest, it's much simpler that the jQuery object window.jQuery). The anonymous closure should then be (function($) { code here })(window.jQuery);.

2. Almost everything in jQuery should run when the document is ready. This solves a lot of small issues. For example, let's say I'm using a really slow computer with a horrible browser; the page takes 3 minutes to load completely. If I click to submit a post, but the whole page isn't ready, maybe the elements you need to submit the form do not exist yet. You will probably get an undefined behavior, since some of the data will be empty.

   It's really easy to solve this issue. Just have everything in your code that is dependent on DOM elements to only load when the document is ready. jQuery provides a great of doing this $(document).ready(function() { code here });. The code that goes inside of that function will run when the document has loaded completely.

   So, to give you and idea, we now have:

   (function($) {
   $(document).ready(function() {
      // code here
   });
   )(window.jQuery);

3. Cache everything. You never know what machine the client is running, but it's always safe to assume that it's 10 times worse than yours (and you should consider mobile, too). I think Javascript is one of the few languages where pre-emptively optimizing is important; in almost all other languages you should test things before trying to make them faster. in Javascript, you should assume your code will run slowly.

   What does this mean? Well, as I said in a few comments in the diff, it means that you should avoid calling $('selector') every time you need an object from the DOM. Especially when the object does not ever change. You should try to initialize variables before using functions that need those elements, so you don't need to select elements every time. This isn't so important with your selectors (since they are all based on ids and those are fast as hell), but it's a good practice to keep.

   The code is getting more reasonable now, and it would look something like this:

   (function($) {
   $(document).ready(function() {
      // Initialize variables here, e.g:
      // var editCommentText = $('#edit-comment');
      // code here
   });
   )(window.jQuery);

   Since we are inside of the document ready function, you know that those elements will exist (unless they are created dynamically).

4. Abuse events. We just saw some kind of use of events with the ready function for the document, but we should use them as much as we can. Especially when the app gets really big (which is not the case here), even relying on your own events is helpful to keep code organized, pretty and stable.

   I suppose you already know what events are, but just in case, they are things that happen that you can listen to. So, for example, when the document is ready, it fires an event of type ready, and since you are listening to it (by using $(document).ready, your code will execute.

   Events solve one problem we had earlier: inline Javascript. Instead of using the onclick argument in the elements, you just do $(selector).on('click', function() { code here});, and the code will execute every time an element matching the selector gets clicked on. This makes the code easier to follow. These functions should be inside the document ready function, so you now have:

   (function($) {
   $(document).ready(function() {
      // Initialize variables here, e.g:
      // var editCommentText = $('#edit-comment');
      // code here (binding events, etc, e.g.:
      // $('#edit-comment').on('click', function() {
      //     edit comment
      // });
   });
   )(window.jQuery);

5. Remember that functions are first-class citizens. So you can pass a function as an argument or just have a general function for errors. You can even set your own event for errors and have a listener to it just in case. Either way, just remember that you can use functions as you use variables (you can even do var functionname = function() {}), although don't abuse this too much.

   The only problem you now face is how to keep track of which comment is being edited for example. But, with this you don't even have to declare any function now, since they are almost all based on events. Including the one to clear the modal, because it can listen on the show event of the bootstrap modal (look it up in Bootstrap documentation).

I think that's about it for general recommendations. I must be forgetting a lot, but I will get to it when I see it :) The problem you now have (tracking which comment is being edited, deleted or flagged) can be solved in many different ones. One way for example is using jQuery data function to attach objects or variables to elements. Or just using HTML5 data- attributes in elements to keep score of what comment an element is referring to. Or you can have a "global" (global within your closure) variable set every time a comment starts being edited. Or you can use a jQuery history plugin to keep track of what you are doing, and use that as events. There are many many solutions; just think what you like best, and I will probably shout at you later for not selecting the one I like :)

[lomegor]

If that's the case you shouldn't probably be using ids. ids are for elements that are unique or that need to be uniquely identified or that work in a unique way. A class is for a group of items that look similar or work similar or share some properties. You should probably be using a class in that case, which would be

$('.edit-coment').on('click', function() {

On 30 July 2013 10:14, Surya Kasturi notifications@github.com wrote:

@lomegor https://github.com/lomegor Thanks a lot for your comment above! Everything sounds good but I still see the tracking of comments problem! That is the reason why I did not use document.ready() before.

In the above mentioned example code $('#edit-comment').on('click', function() {

Since we have many comments, we need to have comment-id value before the onclick event. $('#edit-comment{{comment.id}}").on('click', function() {

There would be many comments and many edit, flag, delete links, even though we use HTML5 data- attribute (for instance), we need to have element Id to get them..

— Reply to this email directly or view it on GitHubhttps://github.com/scipy/SciPyCentral/pull/131#issuecomment-21778580 .

[lomegor]

@ksurya Please re read my comment and tell me if you didn't understand anything. I need you to fix the code before I take a look at it, because right now is even messier. Everything you write in Javascript should be inside the closure and the document.ready, and there should only be one of them (unless for some reason it's absolutely necessary to have code in different files, which I don't think it is). Otherwise, as I told you, you are pollutin the window objects with your variables. So, it should be like this:

(function($) {
  $(document).ready(function() {
    // ALL CODE SHOULD BE HERE
  });
})(window.jQuery);

There's no reason in this code to have more than one closure.

[lomegor]

Some more errors I noticed now:

1. If you are not signed in, the flag link is active, but when I click on it nothing happens.
2. I like the new preview a lot more. There is a problem though: as the size of the preview is usually smaller than the comment field, when I click preview, the window moves and it's disorienting.
3. There's another problem with the preview: as it may take some time to load since we are calling from the server, when it loads, it may have the previous preview instead of the current one. I would advise adding a loading icon and removing it (or hide it) when the preview loads.
4. Also, if I'm on the preview and I post the comment, the preview still shows with the old message, but I can't post it again because, of course, correctly, the text field is now empty. If I click post it should take me back to the comment tab.

I think that's it for that.

[lomegor]

There's some comments that you may have missed because they are not fixed nor do they have a response:

https://github.com/scipy/SciPyCentral/pull/131/files#L1R50

https://github.com/scipy/SciPyCentral/pull/131/files#L10R9

https://github.com/scipy/SciPyCentral/pull/131/files#L10R12

https://github.com/scipy/SciPyCentral/pull/131/files#L15R26

https://github.com/scipy/SciPyCentral/pull/131/files#L25R46

https://github.com/scipy/SciPyCentral/pull/131/files#L25R50

https://github.com/scipy/SciPyCentral/pull/131/files#L25R100

[lomegor]

I added some comments on the diff. I'm going to repeat things that I saw in all the code that I think are important, and that you should look for in the code and fix:

1. Use constant variables outside of functions (inside of the closure) for elements you need to select. Most of the elements you need are static, so you don't need to do $(selector) every time you call the function one one time at the beginning would suffice.
2. The success and error messages still exist in the Javascript.
3. Try to use variables for elements you use more than once inside a function. Don't use $(selector) every time you need them. Just one time at the beginning of the function should be OK.
4. In the same vein, there's no need to select the error and success elements after the AJAX finishes; you can also do $(selector) at the beginning of the function since they are static (only the content changes).
5. Prepend $ to variables which are jQuery objects.

I think that's about it, but read my comments carefully and look at the diff, because some of them may apply in more than one place.

[lomegor]

@ksurya You need to pay more attention to the comments we leave you. You missed at least 7 comments and you also committed mistakes I've already talked about (like having constant variables). I don't know how you are doing it right now, but you should go over all the comments in the Discussion, and after that, you should go to the Diff and read all the comments there. Even after you think you've fixed everything you should go over every comment again to be sure; you are missing quite a lot of things. If you don't understand something or if you are unsure, do ask.

Also, you are having quite a lot of bugs that can be fixed with simple testing. After fixing things, you should at least spend one hour testing different things until somethings breaks (something will probably break). Think of every possible way users will access the feature and do the most awful thinks you can think of (for example, posting a comment, deleting it, clicking edit on another comment, flagging, posting new comment, edit it, flag it, edit it, delete it). You should try these things both logged in and logged out, with the same user and with a different user, etc. Try almost every possible thing you can think of.

Thanks

[lomegor]

Things still needed to be fixed! You need to pay more attention. I'm going to bold problems that existed in previous versions and you haven't fixed.

1. The cursor is not a point in the help text button for comments.
2. I like the new preview a lot more. There is a problem though: as the size of the preview is usually smaller than the comment field, when I click preview, the window moves and it's disorienting.
3. Also, if I'm on the preview and I post the comment, the preview still shows with the old message, but I can't post it again because, of course, correctly, the text field is now empty. If I click post it should take me back to the comment tab.
4. I cannot delete comments. Sometimes I get an error, sometimes the lightbox just disappears but the comment does not. Not sure how that problem happens... I can't reproduce it every time . Try a lot of times to see if you can reproduce it. I think it happens after posting a comment.
5. More importantly, when that I get an error, the lightbox does not get cleared and every time I opened there's the error.
6. Yes! This happens every time after you add a comment. Did you not try adding a comment and then deleting? It also happens for flagging and for editing; all of them are broken. There's some errors in the console such as "NOT FOUND" and "error" which don't help at all. Use more descriptive error messages even in Javascript
7. In a similar note, error messages in Javascript should only appear if DEBUG is true in the python config. Otherwise they should be muted, because they are for developers, not for production. Create a function to report errors that checks for that flag (you can put that flag through django templates), and only shows errors if it's debug. If you want to go the extra mile, you can format the errors nicely, with he name of the function where they happened, etc. Look at some examples online.

[lomegor]

@ksurya, it seems I need to say it again. You need to do all these things:

1. Go over comments on the PR again after you've finished
2. Read the whole diff of the PR here on Github Read it again. Read it one more time. Read all your code now that you know all the problems you have and note where those problems are repeated.
3. Pay attention to what you are doing. Pay attention again.
4. Test, test, test, test, test. After you've done testing, test again. And then test it again. If I keep seeing errors like being unable to delete a message, I think we should add automatic testing, since you are missing a lot of things.

You are making a lot of mistakes that we've already talked about. Once you finish this, you should go to the reputation branch, and update everything to fix all the mistakes we've already talked about. I'm not going to look at the reputation branch if it's going to have a lot of the same errors than this one.

[lomegor]

Also, run pylint!

[pv]

This looks close to finished now, @lomegor?

[lomegor]

I'm going to take a look tomorrow On 17 Aug 2013 14:25, "Pauli Virtanen" notifications@github.com wrote:

This looks close to finished now, @lomegor https://github.com/lomegor?

— Reply to this email directly or view it on GitHubhttps://github.com/scipy/SciPyCentral/pull/131#issuecomment-22811818 .

[lomegor]

Small things to fix...

1. If I flag a comment, and without refreshing the page, I edit it, the text that says "edited" appears to the right of "flagged" (instead of the left).
2. If I'm on the preview tab, I can't post the comment I can click the button, I get a success message, but the comment does not get posted. It does work on edit, I believe.
3. This is something that I didn't care last time but since I'm sending back I think it could be done as an extra. Make the return key work with the modals for delete and flag.
4. There's a small thing in the edit modal, which is that the edit textarea seems to have a margin-bottom that the edit preview does not have, so when changing between preview and edit textarea, the buttons go up and down.
5. Another small thing: when posting a comment, the window scrolls up a bit because the loading is smaller than the textarea. It's a bit annoying.
6. When I click on the preview button on edit comment, the loading images appears too on the new comment area.

I think that's about it. They are all small stuff, but some are a bit important. For example the error when posting on the preview tab.

Overall, very good job! Keep working like this and next time I'll look at this it will be ready to merge!

Don't get discouraged by these errors; they are relatively small compared to all you have fixed. Thanks.

[ksurya]

1. Actually, the responses are placed in a separate div, floated right. So, the order of elements was the reason for it. I reordered it. Probably as I discussed earlier, we could have pulled the responses to the left of comment
2. I am sorry, but I couldn't reproduce [2] error. Probably you were trying to post the same comment once posted. This does not work. Internally django.contrib.comments filter it silently
3. regarding the return key, I have replaced <div> with <button> for submit buttons which enable tabindex. This should fix this
4. regarding the [4] and [5], I fixed them. min-height had to be provided along with some padding
5. The loading-images -- I just didn't notice at all. I added time.sleep() to delay on server side, and I could see the images on the both sides. Fixed it anyway

[lomegor]

@pv If it's OK with you, I think we can now merge this. Thanks, @ksurya!

[pv]

Thanks! Merged.

[ksurya]

Thanks!