fix: Under review tab pagination bug.

[vibros68]

close #2530

This diff updates how fetching is done on the Under Review tab.
Previously, if a vote status contained less than a full page of
proposals, a request would be sent to retrieve those proposals
without filling out the full page size with proposals from the next
vote status. This diff updates this behavior to combine the proposals
from different vote statuses until a full page is being requested.

Old behavior example:
Page size is 5 proposals.
There are 2 proposal being voted on.
There are 3 proposals that have not had their vote authorized yet.
A request would be sent to retrieve the 2 voting proposals.
A separate request would be sent to retrieve the 3 non-authorized
proposals.

New behavior example:
The 2 voting proposals and 3 non-authorized proposals are retrieved
in a single request since the page size is 5 proposals.

vokoscreen-2021-08-27_00-32-52.mp4

[victorgcramos]

Hey Vibros, thanks for the PR. Record is not rendered when I have only 1 unauthorized token in my inventory. Please, make sure you also cover this case on your tests.

Your PR:

Master:

Left some inline comments to improve your code readability. Adding comments to explain what you've done is good, but sometimes it can be redundant and doesn't add any value to the code context. It would be better if you use declarative variable names to describe your procedures and get a cleaner code.

[victorgcramos]

This comment is redundant, you can remove it.

[victorgcramos]

Long conditions should be attributed to variables, in order to provide a better readability.

Suggested change

	!(
	currentStatusTokens.length % INVENTORY_PAGE_SIZE === 0 &&
	currentStatusTokens.length > 0
	)
	canFetchNextPage

[victorgcramos]

Redundant comment.

[vibros68]

Hi @victorgcramos . I fixed all your request changes

[victorgcramos]

Hey mate, thanks for addressing my comments. 1 proposal is now being rendered as expected, but I left another inline comment regarding code readability.

One thing I guess you forgot was to add these cases into e2e tests. You can use the inventory middleware to control what your request returns. See following the example:

politeiagui/teste2e/cypress/e2e/records/recordsList.js

Lines 25 to 32 in 084889b

	cy.middleware("ticketvote.inventory", {
	approved: 4,
	authorized: 0,
	ineligible: 3,
	rejected: 5,
	started: 3,
	unauthorized: 25
	});

You can pass the tokens amount you need to test each case. Since this is one of the main features of our platform, it should have a better test coverage to catch these edge-cases.

[victorgcramos]

I guess you missed my point. The purpose of splitting it into multiple variables is to improve readability, not just create a variable. See, scanTokensCondition brings no value to the code context. It's a condition, but it's a little hard to understand what does that condition mean, and specially, in reviews, it's hard to tell whether this is true or false just by reading the code. This is the "readability" I was talking about.

I'd suggest this:

Suggested change

	const scanTokensCondition =
	initializedInventory &&
	!(
	currentStatusTokens.length % INVENTORY_PAGE_SIZE === 0 &&
	currentStatusTokens.length > 0
	);
	if (scanTokensCondition) {
	const hasAnotherInventoryPage =
	!(
	currentStatusTokens.length % INVENTORY_PAGE_SIZE === 0 &&
	currentStatusTokens.length > 0
	);
	if (initializedInventory && hasAnotherInventoryPage) {

[lukebp]

I agree with your comment and suggestion here, but I really don't like that blog post.

In the discussion that I was having with my senior, I found myself on the one
side passionately defending the declarative style of programming while my
senior was on the other making an argument favoring the imperative style of
programming. He argued that the fact that it’s familiar and intuitive to most
people makes it readable and understandable to a wide variety of programmers
with differing levels of programming experience and expertise. His argument was
that this facilitated teamwork and maintainability since more people can easily
and quickly comprehend the implementation detail of a piece of software.

The points his boss makes are the same design principles behind golang's design and why golang is so popular. Maybe the style he advocates for works when your entire stack is a single language, but that's not the case for us. Decred is mostly golang repos and golang uses the style that he is arguing against. I don't want developers to have to change their programming style when switching between the frontend and backend.

Don’t talk down on other developers by writing simplistic code

I disagree with this as well. Simple, clean code that doesn't make the reader think is easier to maintain, easier to extend, and easier to teach to somone.

[victorgcramos]

That article wasn't a good option, I misunderstood his point and realized after your comment. Removing it right now. I'm sorry.

But I personally think frontend approach is different than backend. JS allows many paradigms to get a cleaner code, and in terms of frontend development, it's harder to test edge-cases, specially when dealing with a non-typed language. Golang provides a strong typing, and indeed prevents many cases that JS don't. When we are talking about JS, you can compare and attribute almost any value to a variable, so that's why I opt to go for a more declarative and readable code. You can mess up a JS code completely just by passing an object where strings are expected, and many times, the error is shown at run-time.

[lukebp]

Understood. The main point I wanted to get across is that, in general, readability should be valued over elegence.

• Don't try and be overly clever. The simple solution is usually the better solution.
• It's ok if a solution adds more lines of code if it makes it more readable.
• It's ok if the performance is a little worse if it makes it more readable.
• Don't use obscure parts of JavaScript that people might have to look up. Use common libraries and idioms.

Think about reading a book that takes complicated topics and writes about them in a ELI5 (explain it to me like I'm 5) style. That's how I want my code to read. If someone has to stop and think "what is this doing" or "why is it doing this" then it's probably not readable enough.

[vibros68]

I understand your point. But this case includes next inventory page and next vote status. So if follow your style it should be canScanNextInventoryPageOrVoteStatus. @victorgcramos how do you thinks?

[victorgcramos]

You can use needsNextBatch, it's good enough :)

[tiagoalvesdulce]

I agree that readability > elegance. Sometimes a more imperative approach is easier to grasp than a declarative one and vice-versa. JS is very flexible and is the programmer's job to choose the best approach for each scenario.

For example, see the implementation of a function that takes an array and returns an array with only odd numbers:

Declarative approach:

const isOdd = n => n % 2 !== 0;
const odds = arr => arr.filter(isOdd);

Imperative approach (using the isOdd util defined above to be more fair):

const odds = arr => {
  const oddsArr = [];
  for (let i = 0; i<arr.length; i++) {
    if (isOdd(arr[i])) {
      oddsArr.push(arr[i]);
    };
  };
  return oddsArr;
}

In this case, the declarative approach reads way better. But now, take a look at this function we use on our code to generate a random color:

const getRandomColor = () => {
  const charOptions = "0123456789ABCDEF";
  let color = "#3F";
  for (let i = 0; i < 4; i++) {
    color += charOptions[Math.floor(Math.random() * 16)];
  };
  return color;
};

This is imperative code, right? How would we rewrite this using a declarative approach? Maybe something like:

const getRandomColor = () => {
  const charOptions = "0123456789ABCDEF";
  const remainingChars = Array(4).fill("");
  return remainingChars.reduce(acc => {
    return acc += charOptions[Math.floor(Math.random() * 16)];
  },"#3F");
}

Which one reads better? I'd go with the imperative code in this case.

So, for declarative vs imperative the answer is: it depends. Always ask yourself if a new programmer would understand your code. If not, try to simplify it. If you really can't do it, isolate the complicated code and add a good comment explaining it.

I didn't get the chance to read the article you guys are talking about (maybe @victorgcramos can add it back with a disclaimer?). But I strongly disagree with this too:

Don’t talk down on other developers by writing simplistic code

I think what @victorgcramos was trying to say is that most of the time, the declarative approach will be the right one because we are using React. React allows us to control data flow and state in a very declarative way and the benefit of this is that we don't get entangled in the implementation details. So, when using React we avoid doing things imperatively like directly manipulate the DOM and directly modify state. Very different than a lib that uses an imperative approach like jQuery, for example.

[lukebp]

Really great comments @tiagoalvesdulce. Thanks for taking the time to write that. I've saved this conversation to a gist for future reference.

https://gist.github.com/lukebp/790527592fba5987f2ec693216199766

[lukebp]

You forgot this part of the original issue.

If the array of tokens in the ticketvote inventory is equal to the inventory page size (20 tokens) 
then it indicates another inventory page should be retreived. It should not continue to the next 
vote status until all proposals have been retreived for the current vote status.

To reproduce this bug:

1. Start 30 proposal votes pictl votetestsetup --votes=30 [adminEmail] [adminPassword]
2. Scroll down the In Discussion tab
3. You'll notice the scrolling stops once 20 proposals are displayed. The remaining 10 proposals are never shown. This represents page 1 of the vote inventory response. Page 2 of the vote inventory response includes the 10 missing proposals.

You can use pictl to examine the different vote inventory pages.
$ pictl voteinv started 1
$ pictl voteinv started 2

[vibros68]

@lukebp Hi. I fixed the case you mentioned. Also I tested some cases that I think can be happened with teste2e. All the cases I tested are correct now.

[vibros68]

@victorgcramos I changed some things in logic. And changed the variable name as the new logic. Hope you can take a look

[lukebp]

tACK

UX looks good. Needs code approval from @tiagoalvesdulce and @victorgcramos.

[vibros68]

tACK

UX looks good. Needs code approval from @tiagoalvesdulce and @victorgcramos.

I will need to add some test cases

[victorgcramos]

LGTM! Good job!

[tiagoalvesdulce]

LGTM. God job!

[lukebp]

tACK