wenqin got a fish trophy!

Summary

add tips feature for reviewboard motd extension

Review Request #10001 — Created June 6, 2018 and discarded Jan. 29, 2019, 2:52 p.m.

Information

Owner

wenqin

Repository

rb-extension-pack

Branch

master

Bugs

Depends On

Reviewers

Groups

rb-extension-pack

People

solarmist

Description

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.

Testing Done

Tested in local environment

Issues

Description	From	Last Updated
E203 whitespace before ':'	reviewbot	June 6, 2018, 3:46 p.m.
E203 whitespace before ':'	reviewbot	June 6, 2018, 3:46 p.m.
E231 missing whitespace after ':'	reviewbot	June 6, 2018, 3:46 p.m.
E231 missing whitespace after ':'	reviewbot	June 6, 2018, 3:46 p.m.
E203 whitespace before ':'	reviewbot	June 6, 2018, 3:46 p.m.
E203 whitespace before ':'	reviewbot	June 6, 2018, 3:46 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	June 6, 2018, 3:46 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	June 6, 2018, 3:46 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	June 6, 2018, 3:46 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	June 6, 2018, 3:46 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	June 6, 2018, 3:46 p.m.
W391 blank line at end of file	reviewbot	June 6, 2018, 3:46 p.m.
F401 'django.utils.six.moves.range' imported but unused	reviewbot	June 6, 2018, 3:46 p.m.
W391 blank line at end of file	reviewbot	June 6, 2018, 3:46 p.m.
W292 no newline at end of file	reviewbot	June 6, 2018, 3:46 p.m.
W292 no newline at end of file	reviewbot	June 6, 2018, 3:46 p.m.
tips_file	solarmist	June 7, 2018, 4:40 p.m.
Probably don't want to allow empty files.	solarmist	June 7, 2018, 4:40 p.m.
Load tips file. One line per tip. If using HTML it must be properly escaped.	solarmist	June 8, 2018, 3:21 p.m.
tips_file	solarmist	June 8, 2018, 3:21 p.m.
I think this is all you need class ListEditWidgetWithClearButton(ListEditWidget): template_name = 'rbmotd/list_edit_widget_with_clear_button.html'	solarmist	June 7, 2018, 4:40 p.m.
Not needed.	solarmist	June 7, 2018, 4:40 p.m.
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	solarmist	June 7, 2018, 4:40 p.m.
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	wenqin	June 18, 2018, 9:49 a.m.
Not needed.	solarmist	June 7, 2018, 4:40 p.m.
Look into less. I think this can re-use most of #motd-close #motd { ... } #motd-close { background: @bg-color-tip; }	solarmist	June 7, 2018, 4:40 p.m.
I tried before and that did not work. I think the only two way to reuse a component is (1) …	wenqin	June 18, 2018, 9:49 a.m.
Just add this to the existing one.	solarmist	June 7, 2018, 4:40 p.m.
Just add this to the existing one.	solarmist	June 7, 2018, 4:40 p.m.
Indentation should be 4 spaces.	solarmist	June 8, 2018, 3:23 p.m.
Please add a comment block like https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js	solarmist	June 7, 2018, 4:40 p.m.
Comment block. https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js	solarmist	June 7, 2018, 4:40 p.m.
Not needed.	solarmist	June 7, 2018, 4:41 p.m.
Comment why are you seperating this?	solarmist	June 7, 2018, 4:41 p.m.
Not needed.	solarmist	June 7, 2018, 4:42 p.m.
Unneeded blank.	solarmist	June 7, 2018, 4:42 p.m.
Comment block https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js	solarmist	June 7, 2018, 4:42 p.m.
Ditto.	solarmist	June 7, 2018, 4:42 p.m.
Comment block https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js	solarmist	June 7, 2018, 4:42 p.m.
blank line between these	solarmist	June 7, 2018, 4:42 p.m.
Comment block https://github.com/djblets/djblets/blob/master/djblets/static/djblets/js/forms/views/listEditView.es6.js	solarmist	June 7, 2018, 4:42 p.m.
Blank line	solarmist	June 7, 2018, 4:42 p.m.
Instead of a for loop use: tips.forEach{} https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach	solarmist	June 8, 2018, 12:22 a.m.
https://api.jquery.com/last/	solarmist	June 8, 2018, 12:28 a.m.
Use a variable to store this then manipulate it.	solarmist	June 8, 2018, 12:28 a.m.
Comment block.	solarmist	June 8, 2018, 12:28 a.m.
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	solarmist	June 8, 2018, 10:23 a.m.
4 spaces for JS code.	solarmist	June 8, 2018, 10:46 a.m.
Can we just make this two functions where we pass in {{motd_close_value}} or the new one and '#modt' or '#motd-tip', …	solarmist	June 8, 2018, 11:03 a.m.
Make this a small block along with the motd one and move it to the top and keep the JS …	solarmist	June 8, 2018, 10:46 a.m.
4 spaces for JS code.	solarmist	June 8, 2018, 11:05 a.m.
I'd make this part of the code that checks the cookie.	solarmist	June 14, 2018, 9:39 p.m.
Unneeded new lines.	solarmist	June 8, 2018, 11:09 a.m.
Comment block	solarmist	June 8, 2018, 12:41 p.m.
Shouldn't be needed if using .forEach	solarmist	June 8, 2018, 12:55 p.m.
.forEach	solarmist	June 8, 2018, 12:55 p.m.
I feel like this could be cleaner, but I'm not positive, but otherwise please add a comment about why this …	solarmist	June 8, 2018, 12:55 p.m.
Comment block. Mention that this is to handle windows newlines.	solarmist	June 8, 2018, 3:25 p.m.
Extra whitespace? Look at the original files and try to match the style that's already there.	solarmist	June 8, 2018, 3:25 p.m.
E501 line too long (136 > 79 characters)	reviewbot	June 18, 2018, 9:50 a.m.
E501 line too long (86 > 79 characters)	reviewbot	June 18, 2018, 9:54 a.m.
E501 line too long (83 > 79 characters)	reviewbot	June 18, 2018, 9:54 a.m.
If this doesn't have any code changes it isn't needed, it'll just re-use the method from ListEditWidget.	solarmist	June 18, 2018, 12:33 p.m.

flake8 failed.

JSHint internal error.

flake8

rbmotd/rbmotd/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E203 whitespace before ':'
```
rbmotd/rbmotd/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E203 whitespace before ':'
```
rbmotd/rbmotd/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E231 missing whitespace after ':'
```
rbmotd/rbmotd/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E231 missing whitespace after ':'
```
rbmotd/rbmotd/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E203 whitespace before ':'
```
rbmotd/rbmotd/extension.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E203 whitespace before ':'
```
rbmotd/rbmotd/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
rbmotd/rbmotd/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
rbmotd/rbmotd/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
rbmotd/rbmotd/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
rbmotd/rbmotd/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E251 unexpected spaces around keyword / parameter equals
```
rbmotd/rbmotd/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
W391 blank line at end of file
```
rbmotd/rbmotd/list_edit_widget_with_clear_button.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F401 'django.utils.six.moves.range' imported but unused
```
rbmotd/rbmotd/list_edit_widget_with_clear_button.py (Diff revision 1)
The issue has been resolved. Show all issues
```
W391 blank line at end of file
```

Commit:

6118a7c0e06d69c0f1caca2dfab639863d716979

4dfc9616caab8b342762bf926e1ce28527df3b4a

Diff:

Revision 2 (+307 -3)

Show changes

	rbmotd/rbmotd/extension.py
	rbmotd/rbmotd/forms.py
	rbmotd/rbmotd/list_edit_widget_with_clear_button.py
	rbmotd/rbmotd/static/css/motd.less
	rbmotd/rbmotd/static/js/motd_tip.js
	rbmotd/rbmotd/templates/rbmotd/list_edit_widget_with_clear_button.html
	rbmotd/rbmotd/templates/rbmotd/motd.html

Checks run (1 failed, 1 failed with error)

flake8 failed.

JSHint internal error.

flake8

rbmotd/rbmotd/forms.py (Diff revision 2)
The issue has been resolved. Show all issues
```
W292 no newline at end of file
```
rbmotd/rbmotd/list_edit_widget_with_clear_button.py (Diff revision 2)
The issue has been resolved. Show all issues
```
W292 no newline at end of file
```

Commit:

4dfc9616caab8b342762bf926e1ce28527df3b4a

71851cfa4a069283c15968c6a4d11ea6fd6c6396

Diff:

Revision 3 (+307 -3)

Show changes

	rbmotd/rbmotd/extension.py
	rbmotd/rbmotd/forms.py
	rbmotd/rbmotd/list_edit_widget_with_clear_button.py
	rbmotd/rbmotd/static/css/motd.less
	rbmotd/rbmotd/static/js/motd_tip.js
	rbmotd/rbmotd/templates/rbmotd/list_edit_widget_with_clear_button.html
	rbmotd/rbmotd/templates/rbmotd/motd.html

Checks run (1 succeeded, 1 failed with error)

flake8 passed.

JSHint internal error.

Description:: ~
add tips feature for reviewboard motd extension
~
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.

rbmotd/rbmotd/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
tips_file
```
rbmotd/rbmotd/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Probably don't want to allow empty files.
```

rbmotd/rbmotd/forms.py (Diff revision 3)

The issue has been resolved. Show all issues

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

rbmotd/rbmotd/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
tips_file
```

rbmotd/rbmotd/list_edit_widget_with_clear_button.py (Diff revision 3)

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.

rbmotd/rbmotd/list_edit_widget_with_clear_button.py (Diff revision 3)

The issue has been resolved. Show all issues

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

rbmotd/rbmotd/list_edit_widget_with_clear_button.py (Diff revision 3)

The issue has been resolved. Show all issues

Not needed.

rbmotd/rbmotd/list_edit_widget_with_clear_button.py (Diff revision 3)

The issue has been resolved. 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

wenqin June 8, 2018, 3:25 p.m.

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

rbmotd/rbmotd/list_edit_widget_with_clear_button.py (Diff revision 3)

The issue has been resolved. Show all issues

Not needed.

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

The issue has been resolved. Show all issues

Look into less. I think this can re-use most of 
#motd-close #motd {
...

}

#motd-close {
  background: @bg-color-tip;
}

rbmotd/rbmotd/static/css/motd.less (Diff revision 3)
The issue has been resolved. Show all issues
```
Just add this to the existing one.
```
rbmotd/rbmotd/static/css/motd.less (Diff revision 3)
The issue has been resolved. Show all issues
```
Just add this to the existing one.
```

rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)

The issue has been resolved. Show all issues

Indentation should be 4 spaces.

rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)

The issue has been resolved. 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

rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)

The issue has been resolved. Show all issues

Comment block.

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

rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Not needed.
```
rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Comment why are you seperating this?
```
rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Not needed.
```
rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Unneeded blank.
```

rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)

The issue has been resolved. Show all issues

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

rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Ditto.
```

rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)

The issue has been resolved. Show all issues

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

rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
The issue has been resolved. Show all issues
```
blank line between these
```

rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)

The issue has been resolved. Show all issues

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

rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line
```

rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)

The issue has been resolved. 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

rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
The issue has been resolved. Show all issues
```
https://api.jquery.com/last/
```
rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Use a variable to store this then manipulate it.
```
rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Comment block.
```

rbmotd/rbmotd/static/js/motd_tip.js (Diff revision 3)

The issue has been resolved. 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

wenqin June 8, 2018, 3:25 p.m.

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

rbmotd/rbmotd/templates/rbmotd/list_edit_widget_with_clear_button.html (Diff revision 3)
```
This is also a copy paste from Djblets.
```

rbmotd/rbmotd/templates/rbmotd/list_edit_widget_with_clear_button.html (Diff revision 3)

This is the new code.

rbmotd/rbmotd/templates/rbmotd/list_edit_widget_with_clear_button.html (Diff revision 3)

The issue has been resolved. Show all issues

4 spaces for JS code.

rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)

The issue has been resolved. 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.

rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)

The issue has been resolved. 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.

rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)

The issue has been resolved. Show all issues

4 spaces for JS code.

rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)

The issue has been dropped. Show all issues

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

wenqin June 8, 2018, 3:25 p.m.

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

rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
The issue has been resolved. Show all issues
```
Unneeded new lines.
```
rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
The issue has been resolved. Show all issues
```
Comment block
```
rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
The issue has been resolved. Show all issues
```
Shouldn't be needed if using .forEach
```
rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)
The issue has been resolved. Show all issues
```
.forEach
```

rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)

The issue has been resolved. 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.

rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)

The issue has been resolved. Show all issues

Comment block.

Mention that this is to handle windows newlines.

wenqin June 8, 2018, 3:25 p.m.
```
what is a windows newlines?
```

solarmist June 14, 2018, 9:46 p.m.

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

rbmotd/rbmotd/templates/rbmotd/motd.html (Diff revision 3)

The issue has been resolved. Show all issues

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

rbmotd/rbmotd/list_edit_widget_with_clear_button.py (Diff revision 3)

The issue has been resolved. 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

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

The issue has been resolved. 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.

Commit:

71851cfa4a069283c15968c6a4d11ea6fd6c6396

8d8c3f79c8fd0397e354bb844549f52e873f9816

Diff:

Revision 4 (+318 -17)

Show changes

	rbmotd/rbmotd/extension.py
	rbmotd/rbmotd/forms.py
	rbmotd/rbmotd/list_edit_widget_with_clear_button.py
	rbmotd/rbmotd/static/css/motd.less
	rbmotd/rbmotd/static/js/motd.js
	rbmotd/rbmotd/static/js/motd_tip.js
	rbmotd/rbmotd/templates/rbmotd/list_edit_widget_with_clear_button.html
	rbmotd/rbmotd/templates/rbmotd/motd.html

Checks run (1 failed, 1 failed with error)

flake8 failed.

JSHint internal error.

flake8

rbmotd/rbmotd/forms.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E501 line too long (136 > 79 characters)
```
rbmotd/rbmotd/forms.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E501 line too long (86 > 79 characters)
```
rbmotd/rbmotd/forms.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E501 line too long (83 > 79 characters)
```

Ship it!

```
Looks good to me.
```

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.')

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.

rbmotd/rbmotd/list_edit_widget_with_clear_button.py (Diff revision 4)

The issue has been resolved. Show all issues

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

rbmotd/rbmotd/static/css/motd.less (Diff revision 4)
```
Uppercase please.
```

Status:: Discarded