Summary

Fix the star column not updating from mobile mode

Review Request #8155 — Created May 11, 2016 and submitted May 27, 2016, 4:27 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-2.5.x

Bugs

Depends On

Commit

cfb19d6...

Reviewers

Groups

reviewboard

People

Description

Previously, when a datagrids switched from mobile mode to desktop mode,
all state (such as whether a review request or group was starred) that
changed in mobile mode would be lost. Now, we keep track of these
changes with the RB.StarManagerView, which will update the affected
rows in the datagrid when transitioning to desktop mode.

Testing Done

Manually tested the following star icons:

Review request datagrid
Group datagrid
Individual review request pages

Ran JS tests.

Issues

Description	From	Last Updated
Trailing ws.	david	May 13, 2016, 3:37 p.m.
Trailing ws.	david	May 13, 2016, 3:38 p.m.
Semicolon.	david	May 13, 2016, 3:38 p.m.
You never use this inside this function, so there's no reason to bind.	david	May 13, 2016, 3:38 p.m.
Should we have an else clause that asserts?	david	May 13, 2016, 3:41 p.m.
Since you only test whether this is falsy, you don't need the \|\| false part.	david	May 25, 2016, 10:54 p.m.
Still repeating this twice.	david	May 25, 2016, 10:53 p.m.
Still repeating this twice.	david	May 25, 2016, 10:52 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/starManagerView.js
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/models/starManagerModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/starManagerView.js
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/models/starManagerModel.js

reviewboard/static/rb/js/common.js (Diff revision 1)

Woohoo! Very happy to see common.js disappearing.

reviewboard/static/rb/js/models/starManagerModel.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing ws.
```
reviewboard/static/rb/js/models/starManagerModel.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Trailing ws.
```
reviewboard/static/rb/js/models/starManagerModel.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Semicolon.
```
reviewboard/static/rb/js/views/starManagerView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
You never use this inside this function, so there's no reason to bind.
```
reviewboard/static/rb/js/views/starManagerView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Should we have an else clause that asserts?
```

Change Summary:

Addressed David's issues.

Diff:

Revision 2 (+193 -67)

Show changes

	reviewboard/staticbundles.py
	reviewboard/scmtools/testdata/hg_repo/.hg/cache/rbc-names-v1
	reviewboard/scmtools/testdata/hg_repo/.hg/cache/rbc-revs-v1
	reviewboard/static/rb/js/common.js
	reviewboard/static/rb/js/models/starManagerModel.js
	reviewboard/static/rb/js/pages/views/datagridPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/views/starManagerView.js

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/scmtools/testdata/hg_repo/.hg/cache/rbc-revs-v1
    reviewboard/static/rb/js/views/starManagerView.js
    reviewboard/scmtools/testdata/hg_repo/.hg/cache/rbc-names-v1
    reviewboard/static/rb/js/models/starManagerModel.js
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/scmtools/testdata/hg_repo/.hg/cache/rbc-revs-v1
    reviewboard/static/rb/js/views/starManagerView.js
    reviewboard/scmtools/testdata/hg_repo/.hg/cache/rbc-names-v1
    reviewboard/static/rb/js/models/starManagerModel.js
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js

It looks like this breaks starring for review requests? Or have those moved beyond using the code in common.js? If this is truly specific to datagrids now, I feel like the naming needs to be more specific, as it sounds generic (StarManager) and yet is only in the datagrid bundle.

brennie May 15, 2016, 2:26 p.m.

No, it does not. The new JS is in both the reviews and datagrids bundle.

Change Summary:

Remove .hg cache stuff

Diff:

Revision 3 (+192 -67)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/common.js
	reviewboard/static/rb/js/models/starManagerModel.js
	reviewboard/static/rb/js/pages/views/datagridPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/views/starManagerView.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/starManagerView.js
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/models/starManagerModel.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/starManagerView.js
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/models/starManagerModel.js

```
Ping.
```

We really shouldn't duplicate the same files in multiple bundles. Stick it into common?

Change Summary:

Only use common bundle

Diff:

Revision 4 (+192 -67)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/common.js
	reviewboard/static/rb/js/models/starManagerModel.js
	reviewboard/static/rb/js/pages/views/datagridPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/views/starManagerView.js

reviewboard/static/rb/js/views/starManagerView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Since you only test whether this is falsy, you don't need the || false part.
```
reviewboard/staticbundles.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Still repeating this twice.
```
reviewboard/staticbundles.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Still repeating this twice.
```

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/starManagerView.js
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/models/starManagerModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/starManagerView.js
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/models/starManagerModel.js

Diff:

Revision 5 (+190 -67)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/common.js
	reviewboard/static/rb/js/models/starManagerModel.js
	reviewboard/static/rb/js/pages/views/datagridPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/views/starManagerView.js

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/starManagerView.js
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/models/starManagerModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/starManagerView.js
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/models/starManagerModel.js

Diff:

Revision 6 (+190 -67)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/common.js
	reviewboard/static/rb/js/models/starManagerModel.js
	reviewboard/static/rb/js/pages/views/datagridPageView.js
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/views/starManagerView.js

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/starManagerView.js
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/models/starManagerModel.js



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/starManagerView.js
    reviewboard/static/rb/js/common.js
    reviewboard/static/rb/js/pages/views/datagridPageView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/static/rb/js/models/starManagerModel.js

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (c979e8b)