Fish Trophy

wenqin got a fish trophy!

Fish Trophy

add tips feature for reviewboard motd extension

Review Request #10001 — Created June 6, 2018 and discarded

Information

rb-extension-pack
master

Reviewers

This change makes motd (message of the day) extension more useful by providing with a tips feature. Admin can manually add tips, or uploading a text file with tips, and it would display a random tip below the message of the day banner.

Tested in local environment

Description From Last Updated

E203 whitespace before ':'

reviewbotreviewbot

E203 whitespace before ':'

reviewbotreviewbot

E231 missing whitespace after ':'

reviewbotreviewbot

E231 missing whitespace after ':'

reviewbotreviewbot

E203 whitespace before ':'

reviewbotreviewbot

E203 whitespace before ':'

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

F401 'django.utils.six.moves.range' imported but unused

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

tips_file

solarmistsolarmist

Probably don't want to allow empty files.

solarmistsolarmist

Load tips file. One line per tip. If using HTML it must be properly escaped.

solarmistsolarmist

tips_file

solarmistsolarmist

I think this is all you need class ListEditWidgetWithClearButton(ListEditWidget): template_name = 'rbmotd/list_edit_widget_with_clear_button.html'

solarmistsolarmist

Not needed.

solarmistsolarmist

Are there any changes to this? It doesn't seems like it, but the code has changed. https://github.com/djblets/djblets/blob/master/djblets/forms/widgets.py

solarmistsolarmist

There are no changes on this function. I copied the codes over from ListEditWidget class from this file: https://github.com/djblets/djblets/blob/master/djblets/forms/widgets.py

wenqinwenqin

Not needed.

solarmistsolarmist

Look into less. I think this can re-use most of #motd-close #motd { ... } #motd-close { background: @bg-color-tip; }

solarmistsolarmist

I tried before and that did not work. I think the only two way to reuse a component is (1) …

wenqinwenqin

Just add this to the existing one.

solarmistsolarmist

Just add this to the existing one.

solarmistsolarmist

Indentation should be 4 spaces.

solarmistsolarmist

Please add a comment block like https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js

solarmistsolarmist

Comment block. https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js

solarmistsolarmist

Not needed.

solarmistsolarmist

Comment why are you seperating this?

solarmistsolarmist

Not needed.

solarmistsolarmist

Unneeded blank.

solarmistsolarmist

Comment block https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js

solarmistsolarmist

Ditto.

solarmistsolarmist

Comment block https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js

solarmistsolarmist

blank line between these

solarmistsolarmist

Comment block https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js

solarmistsolarmist

Blank line

solarmistsolarmist

Instead of a for loop use: tips.forEach{} https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach

solarmistsolarmist

https://api.jquery.com/last/

solarmistsolarmist

Use a variable to store this then manipulate it.

solarmistsolarmist

Comment block.

solarmistsolarmist

You can filter better so you don't need to use a for loop. Then use $.each(function(){$this.click()}. https://stackoverflow.com/questions/10651349/how-to-select-empty-inputs-value-using-jquery?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa

solarmistsolarmist

4 spaces for JS code.

solarmistsolarmist

Can we just make this two functions where we pass in {{motd_close_value}} or the new one and '#modt' or '#motd-tip', …

solarmistsolarmist

Make this a small block along with the motd one and move it to the top and keep the JS …

solarmistsolarmist

4 spaces for JS code.

solarmistsolarmist

I'd make this part of the code that checks the cookie.

solarmistsolarmist

Unneeded new lines.

solarmistsolarmist

Comment block

solarmistsolarmist

Shouldn't be needed if using .forEach

solarmistsolarmist

.forEach

solarmistsolarmist

I feel like this could be cleaner, but I'm not positive, but otherwise please add a comment about why this …

solarmistsolarmist

Comment block. Mention that this is to handle windows newlines.

solarmistsolarmist

Extra whitespace? Look at the original files and try to match the style that's already there.

solarmistsolarmist

E501 line too long (136 > 79 characters)

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

If this doesn't have any code changes it isn't needed, it'll just re-use the method from ListEditWidget.

solarmistsolarmist
Checks run (1 failed, 1 failed with error)
flake8 failed.
JSHint internal error.

flake8

wenqin
Review request changed
Commit:
6118a7c0e06d69c0f1caca2dfab639863d716979
4dfc9616caab8b342762bf926e1ce28527df3b4a

Checks run (1 failed, 1 failed with error)

flake8 failed.
JSHint internal error.

flake8

wenqin
wenqin
solarmist
  1. 
      
  2. rbmotd/rbmotd/forms.py (Diff revision 3)
     
     
    Show all issues

    tips_file

  3. rbmotd/rbmotd/forms.py (Diff revision 3)
     
     
    Show all issues

    Probably don't want to allow empty files.

  4. rbmotd/rbmotd/forms.py (Diff revision 3)
     
     
    Show all issues

    Load tips file. One line per tip. If using HTML it must be properly escaped.

  5. rbmotd/rbmotd/forms.py (Diff revision 3)
     
     
    Show all issues

    tips_file

  6. This is mostly a copy paste extension of the existing list_edit_widget, but the code lives in Djblets so it needed to be duplicated here.

  7. Show all issues

    I think this is all you need

    class ListEditWidgetWithClearButton(ListEditWidget):
        template_name = 'rbmotd/list_edit_widget_with_clear_button.html'
    
  8. rbmotd/rbmotd/list_edit_widget_with_clear_button.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Not needed.

  9. Show all issues

    Are there any changes to this? It doesn't seems like it, but the code has changed.

    https://github.com/djblets/djblets/blob/master/djblets/forms/widgets.py

    1. https://github.com/djblets/djblets/blob/master/djblets/forms/widgets.py
      The code copied from this file

  10. rbmotd/rbmotd/list_edit_widget_with_clear_button.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Not needed.

  11. rbmotd/rbmotd/static/css/motd.less (Diff revision 3)
     
     
    Show all issues

    Look into less. I think this can re-use most of

    #motd-close #motd {
    ...
    
    }
    
    #motd-close {
      background: @bg-color-tip;
    }
    
  12. rbmotd/rbmotd/static/css/motd.less (Diff revision 3)
     
     
    Show all issues

    Just add this to the existing one.

  13. rbmotd/rbmotd/static/css/motd.less (Diff revision 3)
     
     
    Show all issues

    Just add this to the existing one.

  14. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Indentation should be 4 spaces.

  15. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
    Show all issues

    Please add a comment block like
    https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js

  16. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
    Show all issues

    Comment block.

    https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js

  17. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
    Show all issues

    Not needed.

  18. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
    Show all issues

    Comment why are you seperating this?

  19. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
    Show all issues

    Not needed.

  20. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
    Show all issues

    Unneeded blank.

  21. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
    Show all issues

    Comment block
    https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js

  22. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
    Show all issues

    Ditto.

  23. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
    Show all issues

    Comment block
    https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js

  24. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
     
    Show all issues

    blank line between these

  25. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
    Show all issues

    Comment block
    https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js

  26. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
     
    Show all issues

    Blank line

  27. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
    Show all issues

    Instead of a for loop use:

    tips.forEach{}
    

    https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach

  28. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
    Show all issues

    https://api.jquery.com/last/

  29. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
     
    Show all issues

    Use a variable to store this then manipulate it.

  30. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
    Show all issues

    Comment block.

  31. rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
     
     
    Show all issues

    You can filter better so you don't need to use a for loop. Then use $.each(function(){$this.click()}.

    https://stackoverflow.com/questions/10651349/how-to-select-empty-inputs-value-using-jquery?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa

    1. Since we are filtering on '.list-edit-entry input' but trigger events on '.list-edit-remove-item', we need an index to keep trakc of which entry we are at right now

  32. This is also a copy paste from Djblets.

  33. This is the new code.

  34. rbmotd/rbmotd/templates/rbmotd/list_edit_widget_with_clear_button.html (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    4 spaces for JS code.

  35. rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Can we just make this two functions where we pass in {{motd_close_value}} or the new one and '#modt' or '#motd-tip', etc.

  36. rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
     
     
     
     
     
     
     
    Show all issues

    Make this a small block along with the motd one and move it to the top and keep the JS code at the bottom.

  37. rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    4 spaces for JS code.

  38. Show all issues

    I'd make this part of the code that checks the cookie.

    1. This is when the tips are enabled, but admin enters nothing in the tips field. We want to prevent empty tip showing up

  39. rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
     
     
     
     
     
     
    Show all issues

    Unneeded new lines.

  40. Show all issues

    Comment block

  41. rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
     
     
     
     
    Show all issues

    Shouldn't be needed if using .forEach

  42. Show all issues

    .forEach

  43. rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
     
     
     
     
     
     
    Show all issues

    I feel like this could be cleaner, but I'm not positive, but otherwise please add a comment about why this is needed.

  44. Show all issues

    Comment block.

    Mention that this is to handle windows newlines.

    1. what is a windows newlines?

    2. Windows uses '/r/n' as the new line character. Linux and Mac systems use only '/n' as their new line character.

  45. rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
     
     
     
    Show all issues

    Extra whitespace? Look at the original files and try to match the style that's already there.

  46. 
      
wenqin
  1. 
      
  2. Show all issues

    There are no changes on this function. I copied the codes over from ListEditWidget class from this file:
    https://github.com/djblets/djblets/blob/master/djblets/forms/widgets.py

  3. rbmotd/rbmotd/static/css/motd.less (Diff revision 3)
     
     
    Show all issues

    I tried before and that did not work.

    I think the only two way to reuse a component is
    (1) define a mixing and pass in variables
    (2) extension.

  4. 
      
wenqin
Review request changed
Commit:
71851cfa4a069283c15968c6a4d11ea6fd6c6396
8d8c3f79c8fd0397e354bb844549f52e873f9816

Checks run (1 failed, 1 failed with error)

flake8 failed.
JSHint internal error.

flake8

solarmist
  1. Looks good to me.

  2. rbmotd/rbmotd/forms.py (Diff revision 4)
     
     

    Try this.

    help_text=_('Load tips file. One line per tip. '
                'This field expects valid text file. '
                'If using HTML it must be properly escaped.')
    
  3. rbmotd/rbmotd/forms.py (Diff revision 4)
     
     

    I think I'd make some variables for message, tips and tips_file. That'll take care of the lines too long.

  4. Show all issues

    If this doesn't have any code changes it isn't needed, it'll just re-use the method from ListEditWidget.

  5. rbmotd/rbmotd/static/css/motd.less (Diff revision 4)
     
     

    Uppercase please.

  6. 
      
david
Review request changed
Status:
Discarded