Summary

Fix AccountPage to have default form classes on initial page registration

Review Request #7103 — Created March 22, 2015 and submitted April 22, 2015, 12:33 p.m.

Information

Owner

Sunxperous

Repository

Review Board

Branch

master

Bugs

Depends On

Blocks

7099

Commit

4a5e110...

Reviewers

Groups

reviewboard

People

chipx86

Description

When an extension uses AccountPagesHook with a pre-built list of forms (form_classes), the list form_classes is cleared when the extension's AccountPage is unregistered after the extension reloads. This causes the extension's AccountPage(s) to not show up in the account page after.

This fix keeps track of the initial form_classes defined by the AccountPage with _default_form_classes, such that the original defined list of form_classes will be restored after an extension reload.

Testing Done

Wrote two unit tests which passed alongside existing tests. Also verified that an extension which uses AccountPagesHook still shows up in account pages after a reload.

Issues

Description	From	Last Updated
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 22, 2015, 9:21 a.m.
Col: 20 E225 missing whitespace around operator	reviewbot	March 22, 2015, 9:21 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 22, 2015, 9:21 a.m.
I'm really confused by this. We initialize one from the other and vice versa? Can we keep the flow of …	david	April 17, 2015, 1:03 a.m.
Since we are comparing between empty or not empty list, I think this is possible as well. elif not page_cls.form_classes:	XU xuanyi	April 17, 2015, 12:12 a.m.
No period at the end of test case docstrings (nose will add a ... when printing the names)	david	April 17, 2015, 12:13 a.m.
No period at the end of test case docstrings (nose will add a ... when printing the names)	david	April 17, 2015, 12:13 a.m.
Blank line between after class docstring.	brennie	April 17, 2015, 10:58 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/accounts/tests.py
    reviewboard/accounts/pages.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/accounts/tests.py
    reviewboard/accounts/pages.py

reviewboard/accounts/pages.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 20
 E225 missing whitespace around operator
```
reviewboard/accounts/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Change Summary:

I thought it was 80, not 79.

Commit:

a36bfc7de3c618af166d5887d0312e4f5f238996

75261225ee10f92bdec523a8c18a8d8d359e82d2

Diff:

Revision 2 (+44)

Show changes

	reviewboard/accounts/pages.py
	reviewboard/accounts/tests.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/accounts/tests.py
    reviewboard/accounts/pages.py



Tool: Pyflakes
Processed Files:
    reviewboard/accounts/tests.py
    reviewboard/accounts/pages.py

reviewboard/accounts/pages.py (Diff revision 2)

The issue has been resolved. Show all issues

Since we are comparing between empty or not empty list, I think this is possible as well.

elif not page_cls.form_classes:

Sunxperous March 25, 2015, 8:34 p.m.

Yes, that is more Pythonic, but there are also occurrences of the explicit condition elsewhere in the code base, so I am not sure what the standard is.

reviewboard/accounts/pages.py (Diff revision 2)

The issue has been resolved. Show all issues

I'm really confused by this. We initialize one from the other and vice versa? Can we keep the flow of data one direction?

Sunxperous April 17, 2015, 1:03 a.m.

We initialize _default_form_classes only when it first registers. I have separated the block to make this clearer.

reviewboard/accounts/tests.py (Diff revision 2)

The issue has been resolved. Show all issues

No period at the end of test case docstrings (nose will add a ... when printing the names)

reviewboard/accounts/tests.py (Diff revision 2)

The issue has been resolved. Show all issues

No period at the end of test case docstrings (nose will add a ... when printing the names)

Change Summary:

Fix syntax and separate if-elif block.

Commit:

75261225ee10f92bdec523a8c18a8d8d359e82d2

7dde823091e5aab88326d05c3a2dda8edc54dd31

Diff:

Revision 3 (+44)

Show changes

	reviewboard/accounts/pages.py
	reviewboard/accounts/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/accounts/tests.py
    reviewboard/accounts/pages.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/accounts/tests.py
    reviewboard/accounts/pages.py

reviewboard/accounts/pages.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line between after class docstring.
```

Change Summary:

Add blank line

Commit:

7dde823091e5aab88326d05c3a2dda8edc54dd31

4a5e1102bde9e24fa1fae0d383490b2edadb5ac2

Diff:

Revision 4 (+45)

Show changes

	reviewboard/accounts/pages.py
	reviewboard/accounts/tests.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/accounts/tests.py
    reviewboard/accounts/pages.py



Tool: Pyflakes
Processed Files:
    reviewboard/accounts/tests.py
    reviewboard/accounts/pages.py

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.0.x (d075eb2)