Allow collapsed entries to fetch data on request

Review Request #11886 — Created Nov. 29, 2021 and updated

cynthia
Review Board
master
reviewboard

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 Author
add console log for debug
Your Name
load the collapsed entry info, finally get it working
Your Name
finalize project
Your Name
code clean up
Your Name
remove white spaces
Your Name
remove white space
Your Name
review bot issues
Your Name
review bot issues
Your Name
remove white spaces
Your Name
remove white spaces
Your Name
Address the comments and issues opened by David on the review request
Your Name
remove white spaces
Your Name
remove white spaces
Your Name
change test
Your Name
address issues opened
Your Name
remove space
Your Name
Description From Last Updated

W293 blank line contains whitespace

reviewbotreviewbot

Col: 49 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 24 Expected '===' and instead saw '=='.

reviewbotreviewbot

Col: 45 Expected '===' and instead saw '=='.

reviewbotreviewbot

Can you add another paragraph explaining what this means?

daviddavid

Can you add another paragraph explaining what this means?

daviddavid

There's an extra blank line here.

daviddavid

This can use an ES6 template string.

daviddavid

Add spaces after commas here.

daviddavid

Instead of creating a variable for this, let's just define the fat arrow function inline: parsed.load((metadata, html) => this._reloadFromUpdate(entry, metadata, ...

daviddavid

Your editor completely reformatted this file to add indentation. Please revert the changes here.

daviddavid

Same with this file.

daviddavid

Add a trailing comma here. Let's also name the function _onToggleCollapseMouseover

daviddavid

Add a space between ) and {

daviddavid

This needs additional spaces: Between if and ( Between ) and {

daviddavid

Use single quotes here.

daviddavid

=== will result in a bool. We don't need the ? true : false

daviddavid

This needs spaces like the if above.

daviddavid

This file got unwanted indentation.

daviddavid

An unwanted space was added at the beginning of this line.

daviddavid

The numbers on the names model, model2, view1, etc. don't really add anything useful, and they're all scoped to the ...

daviddavid

This can be const (we only assign to the name once). Same in other test cases.

daviddavid

Some more blank lines in here might make it a little more readable. We can also skip creating a variable ...

daviddavid

"give" isn't a typical term used for this kind of function. A better one might be "fetch". How about calling ...

chipx86chipx86

urlQueryStr is checking whether urlQuery has anything in it, but we're setting it to something immediately before. We probably can ...

chipx86chipx86

I think this function is likely to be more for internal use, so I'd suggest making it private (prefix with ...

chipx86chipx86

EntryView shouldn't know what kind of subclasses there will be. In fact, extensions can provide whole new types of entries, ...

chipx86chipx86

Based on our naming conventions, this should be templateWithoutBody. Likewise, below should be templateWithBody.

chipx86chipx86

There's an extra sapce at the end of the class list.

chipx86chipx86

Here, too.

chipx86chipx86

And in these spots.

chipx86chipx86

Can you add a blank line between these?

chipx86chipx86

Extra space at the end of the string.

chipx86chipx86

There are extra blank lines at the end of this file. Can you get rid of those?

chipx86chipx86

Can you reorder this to be in alphabetical order? Should be before issueSummaryTableViewTests.

chipx86chipx86

Since you're adding a new block tag, you'll need to indent all the block tags inside by one. For Django ...

chipx86chipx86
Checks run (2 failed)
flake8 failed.
JSHint failed.

flake8

JSHint

cynthia
cynthia
david
  1. 
      
  2. Can you add another paragraph explaining what this means?

  3. Can you add another paragraph explaining what this means?

  4. This can use an ES6 template string.

  5. 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));
    
  6. Your editor completely reformatted this file to add indentation. Please revert the changes here.

  7. reviewboard/static/rb/js/reviewRequestPage/views/changeEntryView.es6.js (Diff revision 2)
     
     
     
     
     
     
     
     
     
     

    Same with this file.

  8. Add a trailing comma here. Let's also name the function _onToggleCollapseMouseover

  9. Add a space between ) and {

  10. This needs additional spaces:

    • Between if and (
    • Between ) and {
  11. === will result in a bool. We don't need the ? true : false

  12. This needs spaces like the if above.

  13. This file got unwanted indentation.

  14. An unwanted space was added at the beginning of this line.

  15. 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 them model and view.

  16. This can be const (we only assign to the name once). Same in other test cases.

  17. 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.

  18. 
      
cynthia
chipx86
  1. 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!

  2. "give" isn't a typical term used for this kind of function. A better one might be "fetch". How about calling this fetchEntryUpdates?

  3. urlQueryStr is checking whether urlQuery 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.

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

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

    1. Hi Christian, thanks a lot for your detailed code review, I've learned a lot from it!

      I fixed all issues opened except this one. I believe it's a great idea to add a new state "loaded" in the entry model. I moved "ensureContentLoaded" to the model class and I tried to add the "loaded" state to my local branch but had some problems with running it. Sadly, I don't think I get enough time to fix it and add corresponding tests as there are less than two hours left until the pencils down time :(

      I think I'll just keep my current working version of this project without using the load state in the review request. If possible, I'll come back to work on this after I finish all my finals.I would really appreciate your understanding.

    2. I ended up publishing a new WIP review request.

  6. Based on our naming conventions, this should be templateWithoutBody. Likewise, below should be templateWithBody.

  7. There's an extra sapce at the end of the class list.

  8. Can you add a blank line between these?

  9. Extra space at the end of the string.

  10. There are extra blank lines at the end of this file. Can you get rid of those?

  11. reviewboard/staticbundles.py (Diff revision 3)
     
     
     
     

    Can you reorder this to be in alphabetical order? Should be before issueSummaryTableViewTests.

  12. 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 %}
    
  13. 
      
cynthia
cynthia
Review request changed

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.

Loading...