Allow collapsed entries to fetch data on request
Review Request #11886 — Created Nov. 30, 2021 and updated
Review Board will collapse old reviews to streamline information.
While the data is visually hidden it will still be computed
and loaded to the client which can cause slowness.This change makes collapsed review entries get sent to the client
empty, then fetch its content on request. When they expand the entry, we
first check if this entry already has content loaded, if not then load it
by using the new function "fetchEntryUpdates" in reviewRequestPageModel.js
which can trigger an immediate update to load the content of an entry.
This function uses and extends the machinery related to "page pending updates"
In order to make it feel even snappier, I add a mouseover event listener
on the expand button and the expand all button. We start to pull down information
before they even lift the mouse button.Only load the entry content if the entry is of type "review". Without this restriction,
when they click the expand all button, the entry of type "initial_status_updates" or
"changedesc"also tries to check whether it has content which causes an error.
Created entryViewTests.es6.js that contains test cases for the
behavior of expanding an entry. All tests passed.Ran all Javascript unit tests and no newly introduced test fails.
Summary | ID | Author |
---|---|---|
e5efe2ddb94478ad7ed00ed9f96d6851a5894d43 | Your Name | |
6d22271b67203158e4df73ea6aac8003b2cb0467 | Your Name | |
62e4b4ccb21a2ad45381f4fafd7164b90a2ca957 | Your Name | |
ad5df68ce4b4c6204e2af1aca5cbc3ea8f7addba | Your Name | |
a729ceb24d2c26f71097e64be3d7580385f84a8c | Your Name | |
41ccf48b1b2a3160c3a70a9fac4840eaeecb366e | Your Name | |
e3a0f17e22bfcb4a09cba8a7aa1ca1513d89f535 | Your Name | |
bf3cd0b4df11ae02cb765fdcaa0a2128157d63f5 | Your Name | |
74d904995f62be003fb653c7c6eb8b814bc4e809 | Your Name | |
b667b710a6b7d86853fabe942923f25c1db5c2b0 | Your Name | |
52273caa718ee0933466cf82195965169ebe1129 | Your Name | |
1bae1c99c7b799bdbedb30edba3de1f3433207ad | Your Name | |
ed1bab7a6d126d95ba8968405a9265d4730195c6 | Your Name | |
77205195ee98439162f8971d1d91d1dbf943fd4c | Your Name | |
1fc906713e9341cdea44207ef4d37012d60f217e | Your Name | |
03e49d2d6fbf68ede0aa21b2cd7aa54ec281f3d0 | Your Name |
Description | From | Last Updated |
---|---|---|
W293 blank line contains whitespace |
reviewbot | |
Col: 49 Expected '===' and instead saw '=='. |
reviewbot | |
Col: 24 Expected '===' and instead saw '=='. |
reviewbot | |
Col: 45 Expected '===' and instead saw '=='. |
reviewbot | |
Can you add another paragraph explaining what this means? |
david | |
Can you add another paragraph explaining what this means? |
david | |
There's an extra blank line here. |
david | |
This can use an ES6 template string. |
david | |
Add spaces after commas here. |
david | |
Instead of creating a variable for this, let's just define the fat arrow function inline: parsed.load((metadata, html) => this._reloadFromUpdate(entry, metadata, … |
david | |
Your editor completely reformatted this file to add indentation. Please revert the changes here. |
david | |
Same with this file. |
david | |
Add a trailing comma here. Let's also name the function _onToggleCollapseMouseover |
david | |
Add a space between ) and { |
david | |
This needs additional spaces: Between if and ( Between ) and { |
david | |
Use single quotes here. |
david | |
=== will result in a bool. We don't need the ? true : false |
david | |
This needs spaces like the if above. |
david | |
This file got unwanted indentation. |
david | |
An unwanted space was added at the beginning of this line. |
david | |
The numbers on the names model, model2, view1, etc. don't really add anything useful, and they're all scoped to the … |
david | |
This can be const (we only assign to the name once). Same in other test cases. |
david | |
Some more blank lines in here might make it a little more readable. We can also skip creating a variable … |
david | |
"give" isn't a typical term used for this kind of function. A better one might be "fetch". How about calling … |
chipx86 | |
urlQueryStr is checking whether urlQuery has anything in it, but we're setting it to something immediately before. We probably can … |
chipx86 | |
I think this function is likely to be more for internal use, so I'd suggest making it private (prefix with … |
chipx86 | |
EntryView shouldn't know what kind of subclasses there will be. In fact, extensions can provide whole new types of entries, … |
chipx86 | |
Based on our naming conventions, this should be templateWithoutBody. Likewise, below should be templateWithBody. |
chipx86 | |
There's an extra sapce at the end of the class list. |
chipx86 | |
Here, too. |
chipx86 | |
And in these spots. |
chipx86 | |
Can you add a blank line between these? |
chipx86 | |
Extra space at the end of the string. |
chipx86 | |
There are extra blank lines at the end of this file. Can you get rid of those? |
chipx86 | |
Can you reorder this to be in alphabetical order? Should be before issueSummaryTableViewTests. |
chipx86 | |
Since you're adding a new block tag, you'll need to indent all the block tags inside by one. For Django … |
chipx86 |
- Commits:
-
Summary ID Author e5efe2ddb94478ad7ed00ed9f96d6851a5894d43 Your Name 6d22271b67203158e4df73ea6aac8003b2cb0467 Your Name 62e4b4ccb21a2ad45381f4fafd7164b90a2ca957 Your Name ad5df68ce4b4c6204e2af1aca5cbc3ea8f7addba Your Name a729ceb24d2c26f71097e64be3d7580385f84a8c Your Name 41ccf48b1b2a3160c3a70a9fac4840eaeecb366e Your Name e5efe2ddb94478ad7ed00ed9f96d6851a5894d43 Your Name 6d22271b67203158e4df73ea6aac8003b2cb0467 Your Name 62e4b4ccb21a2ad45381f4fafd7164b90a2ca957 Your Name ad5df68ce4b4c6204e2af1aca5cbc3ea8f7addba Your Name a729ceb24d2c26f71097e64be3d7580385f84a8c Your Name 41ccf48b1b2a3160c3a70a9fac4840eaeecb366e Your Name e3a0f17e22bfcb4a09cba8a7aa1ca1513d89f535 Your Name bf3cd0b4df11ae02cb765fdcaa0a2128157d63f5 Your Name 74d904995f62be003fb653c7c6eb8b814bc4e809 Your Name b667b710a6b7d86853fabe942923f25c1db5c2b0 Your Name - Diff:
-
Revision 2 (+2088 -1706)
Checks run (2 succeeded)
- Testing Done:
-
Created entryViewTests.es6.js that contains test cases for the
behavior of expanding an entry. All tests passed. ~ Ran all Javascript unit tests, 6 tests related to UI didn't pass,
~ Ran all Javascript unit tests and no newly introduced test fails.
- I am not sure what is the cause but looks like it is not related to my changes, - please see the screenshot attached. - - -
-
-
-
-
-
-
-
Instead of creating a variable for this, let's just define the fat arrow function inline:
parsed.load((metadata, html) => this._reloadFromUpdate(entry, metadata, html));
-
-
-
-
-
-
-
-
-
-
-
The numbers on the names
model
,model2
,view1
, etc. don't really add anything useful, and they're all scoped to the test body anyway. Since we're just instantiating one per test, let's just call themmodel
andview
. -
-
Some more blank lines in here might make it a little more readable. We can also skip creating a variable for
$collapseButton
because we only use it once.
- Change Summary:
-
- Addressed all the issues.
- Added mouseover event listener on the expand all button.
- Restricted the target entry to the one with type "review".
- Modified unit tests in accordance to the latest changes.
- Description:
-
Review Board will collapse old reviews to streamline information.
While the data is visually hidden it will still be computed and loaded to the client which can cause slowness. ~ This change lets collapsed review entries get sent to the client
~ empty then fetch its content on request. When they expand the entry, we ~ This change makes collapsed review entries get sent to the client
~ empty, then fetch its content on request. When they expand the entry, we first check if this entry already has content loaded, if not then load it by using the new function "giveImmediateUpdates" in reviewRequestPageModel.js which can trigger an immediate update to load the content of an entry. This function uses and extends the machinery related to "page pending updates" In order to make it feel even snappier, I add a mouseover event listener ~ on the expand button. We start to pull down information before they ~ even lift the mouse button. ~ on the expand button and the expand all button. We start to pull down information ~ before they even lift the mouse button. ~ ~ ~ Only load the entry content if the entry is of type "review" or "changedesc".
~ Only load the entry content if the entry is of type "review". Without this restriction,
~ when they click the expand all button, the entry of type "initial_status_updates" or ~ "changedesc"also tries to check whether it has content which causes an error. - Without this restriction, when they click the expand all button, the entry - of type "initial_status_updates" also tries to check whether it has content - which cause an error. - Commits:
-
Summary ID Author e5efe2ddb94478ad7ed00ed9f96d6851a5894d43 Your Name 6d22271b67203158e4df73ea6aac8003b2cb0467 Your Name 62e4b4ccb21a2ad45381f4fafd7164b90a2ca957 Your Name ad5df68ce4b4c6204e2af1aca5cbc3ea8f7addba Your Name a729ceb24d2c26f71097e64be3d7580385f84a8c Your Name 41ccf48b1b2a3160c3a70a9fac4840eaeecb366e Your Name e3a0f17e22bfcb4a09cba8a7aa1ca1513d89f535 Your Name bf3cd0b4df11ae02cb765fdcaa0a2128157d63f5 Your Name 74d904995f62be003fb653c7c6eb8b814bc4e809 Your Name b667b710a6b7d86853fabe942923f25c1db5c2b0 Your Name e5efe2ddb94478ad7ed00ed9f96d6851a5894d43 Your Name 6d22271b67203158e4df73ea6aac8003b2cb0467 Your Name 62e4b4ccb21a2ad45381f4fafd7164b90a2ca957 Your Name ad5df68ce4b4c6204e2af1aca5cbc3ea8f7addba Your Name a729ceb24d2c26f71097e64be3d7580385f84a8c Your Name 41ccf48b1b2a3160c3a70a9fac4840eaeecb366e Your Name e3a0f17e22bfcb4a09cba8a7aa1ca1513d89f535 Your Name bf3cd0b4df11ae02cb765fdcaa0a2128157d63f5 Your Name 74d904995f62be003fb653c7c6eb8b814bc4e809 Your Name b667b710a6b7d86853fabe942923f25c1db5c2b0 Your Name 52273caa718ee0933466cf82195965169ebe1129 Your Name 1bae1c99c7b799bdbedb30edba3de1f3433207ad Your Name ed1bab7a6d126d95ba8968405a9265d4730195c6 Your Name 77205195ee98439162f8971d1d91d1dbf943fd4c Your Name - Diff:
-
Revision 3 (+2280 -1770)
Checks run (2 succeeded)
-
This looks great! I have suggestions to bring it up to our in-house standards, and a way to better generalize the conditional content loading part. Once fixed up, I think we'll be able to get this into 5.0!
-
"give" isn't a typical term used for this kind of function. A better one might be "fetch". How about calling this
fetchEntryUpdates
? -
urlQueryStr
is checking whetherurlQuery
has anything in it, but we're setting it to something immediately before. We probably can simplify this quite a bit by replacing all this code with:const typeID = entry.get('typeID'); const urlQueryStr = `entries=${typeID}:${entry.id}`;
Nice and simple. No need to join the array, because we only ever have the one entry.
-
I think this function is likely to be more for internal use, so I'd suggest making it private (prefix with a
_
and group it with other private functions). -
EntryView
shouldn't know what kind of subclasses there will be. In fact, extensions can provide whole new types of entries, which may want to load based on their own logic.I'd suggest instead keeping state on the model indicating if the entry has been loaded, which would be part of the initial model data on the page. If loaded, have this function do nothing. If not loaded, load it.
And actually, given that the state would be in the model, and the operation that loads the updates is also in the model, I think this function would be better off in the model. Based on how we often name things, maybe call it
ensureContentLoaded
. This communicates that the function will load the content if it's needed (and otherwise do nothing — it just ensures that we end up with content one way or another in the end). -
Based on our naming conventions, this should be
templateWithoutBody
. Likewise, below should betemplateWithBody
. -
-
-
-
-
-
-
-
Since you're adding a new block tag, you'll need to indent all the block tags inside by one.
For Django templates, we'd do this like:
{% if not entry.collapsed %} {% if review.body_top ... %} {% blah blah %} ... {% endif %} {% endif %}
- Change Summary:
-
- Addressed issues.
- Modified unit tests in accordance to the latest changes.
- Commits:
-
Summary ID Author e5efe2ddb94478ad7ed00ed9f96d6851a5894d43 Your Name 6d22271b67203158e4df73ea6aac8003b2cb0467 Your Name 62e4b4ccb21a2ad45381f4fafd7164b90a2ca957 Your Name ad5df68ce4b4c6204e2af1aca5cbc3ea8f7addba Your Name a729ceb24d2c26f71097e64be3d7580385f84a8c Your Name 41ccf48b1b2a3160c3a70a9fac4840eaeecb366e Your Name e3a0f17e22bfcb4a09cba8a7aa1ca1513d89f535 Your Name bf3cd0b4df11ae02cb765fdcaa0a2128157d63f5 Your Name 74d904995f62be003fb653c7c6eb8b814bc4e809 Your Name b667b710a6b7d86853fabe942923f25c1db5c2b0 Your Name 52273caa718ee0933466cf82195965169ebe1129 Your Name 1bae1c99c7b799bdbedb30edba3de1f3433207ad Your Name ed1bab7a6d126d95ba8968405a9265d4730195c6 Your Name 77205195ee98439162f8971d1d91d1dbf943fd4c Your Name e5efe2ddb94478ad7ed00ed9f96d6851a5894d43 Your Name 6d22271b67203158e4df73ea6aac8003b2cb0467 Your Name 62e4b4ccb21a2ad45381f4fafd7164b90a2ca957 Your Name ad5df68ce4b4c6204e2af1aca5cbc3ea8f7addba Your Name a729ceb24d2c26f71097e64be3d7580385f84a8c Your Name 41ccf48b1b2a3160c3a70a9fac4840eaeecb366e Your Name e3a0f17e22bfcb4a09cba8a7aa1ca1513d89f535 Your Name bf3cd0b4df11ae02cb765fdcaa0a2128157d63f5 Your Name 74d904995f62be003fb653c7c6eb8b814bc4e809 Your Name b667b710a6b7d86853fabe942923f25c1db5c2b0 Your Name 52273caa718ee0933466cf82195965169ebe1129 Your Name 1bae1c99c7b799bdbedb30edba3de1f3433207ad Your Name ed1bab7a6d126d95ba8968405a9265d4730195c6 Your Name 77205195ee98439162f8971d1d91d1dbf943fd4c Your Name 1fc906713e9341cdea44207ef4d37012d60f217e Your Name 03e49d2d6fbf68ede0aa21b2cd7aa54ec281f3d0 Your Name - Diff:
-
Revision 4 (+2421 -1915)
Checks run (2 succeeded)
- Description:
-
Review Board will collapse old reviews to streamline information.
While the data is visually hidden it will still be computed and loaded to the client which can cause slowness. This change makes collapsed review entries get sent to the client
empty, then fetch its content on request. When they expand the entry, we first check if this entry already has content loaded, if not then load it ~ by using the new function "giveImmediateUpdates" in reviewRequestPageModel.js ~ by using the new function "fetchEntryUpdates" in reviewRequestPageModel.js which can trigger an immediate update to load the content of an entry. This function uses and extends the machinery related to "page pending updates" In order to make it feel even snappier, I add a mouseover event listener on the expand button and the expand all button. We start to pull down information before they even lift the mouse button. Only load the entry content if the entry is of type "review". Without this restriction,
when they click the expand all button, the entry of type "initial_status_updates" or "changedesc"also tries to check whether it has content which causes an error.