Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Under review tab pagination bug. #2552

Merged
merged 6 commits into from
Sep 3, 2021
Merged

fix: Under review tab pagination bug. #2552

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
12f1c4d
fix under review tab pagination bug: fetch 5 proposals per request ti…
vibros68 Aug 26, 2021
2d11125
add condition variable, fix scanNextStatusTokens function bug, remove…
vibros68 Aug 27, 2021
ad10ad3
reproduce scan tokens logic
vibros68 Aug 28, 2021
e70c680
batch pagination testing
vibros68 Aug 30, 2021
61a94d2
fix e2e test: records and inventory pagination
vibros68 Aug 31, 2021
2636412
clean up code
vibros68 Aug 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/containers/Proposal/Actions/UnvettedActionsProvider.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ const UnvettedActionsProvider = ({ children, history }) => {
successTitle: "Proposal approved",
successMessage: (
<Text>
The proposal has been successfully approved! Now it will appear under{" "}
<Link to="/?tab=in%20discussion">In discussion</Link> tab among Public
The proposal has been successfully approved! Now it will appear in{" "}
<Link to="/?tab=under%20review">Under review</Link> tab among Public
Proposals.
</Text>
),
Expand Down
103 changes: 85 additions & 18 deletions src/hooks/api/useProposalsBatch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ export default function useProposalsBatch({
}) {
const [remainingTokens, setRemainingTokens] = useState([]);
const [voteStatuses, setStatuses] = useState(statuses);
const [previousPage, setPreviousPage] = useState(0);
const [initializedInventory, setInitializedInventory] = useState(false);
const isByRecordStatus = isUndefined(voteStatuses);
const proposals = useSelector(sel.proposalsByToken);
const recordState = unvetted
Expand All @@ -116,7 +116,9 @@ export default function useProposalsBatch({
isByRecordStatus ? sel.allByRecordStatus : sel.allByVoteStatus
);
const tokens = useMemo(
() => allByStatus[getProposalStatusLabel(currentStatus, isByRecordStatus)],
() =>
allByStatus[getProposalStatusLabel(currentStatus, isByRecordStatus)] ||
[],
[allByStatus, isByRecordStatus, currentStatus]
);
const page = useMemo(() => {
Expand All @@ -130,12 +132,59 @@ export default function useProposalsBatch({
const onFetchProposalsBatch = useAction(act.onFetchProposalsBatch);
const onFetchTokenInventory = useAction(act.onFetchTokenInventory);
const hasRemainingTokens = !isEmpty(remainingTokens);

const scanNextStatusTokens = (index, oldTokens) => {
const status = currentStatuses[index];
const newTokens = getUnfetchedTokens(
proposals,
uniq(allByStatus[getProposalStatusLabel(status, isByRecordStatus)] || [])
);

const tokens = [...oldTokens, ...newTokens];
if (tokens.length < proposalPageSize && index < currentStatuses.length) {
return scanNextStatusTokens(index + 1, tokens);
}
return {
index,
tokens
};
};

const [state, send] = useFetchMachine({
actions: {
initial: () => send(START),
start: () => {
if (hasRemainingTokens) return send(VERIFY);
if (page && page === previousPage) return send(RESOLVE);
if (remainingTokens.length >= proposalPageSize) return send(VERIFY);

// If remaining tokens length is smaller than proposal page size.
// Find more tokens from inventory or scan from the next status

// Condition to scan: inventory is initialized and
// the tokens are able to be fetched from the next page
const currentStatusTokens =
allByStatus[
getProposalStatusLabel(currentStatus, isByRecordStatus)
] || [];
const scanTokensCondition =
initializedInventory &&
!(
currentStatusTokens.length % INVENTORY_PAGE_SIZE === 0 &&
currentStatusTokens.length > 0
);
if (scanTokensCondition) {
Copy link
Member

@victorgcramos victorgcramos Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Member

@lukebp lukebp Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@lukebp lukebp Aug 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@vibros68 vibros68 Aug 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@victorgcramos victorgcramos Aug 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@tiagoalvesdulce tiagoalvesdulce Aug 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const { index, tokens } = scanNextStatusTokens(
statusIndex + 1,
remainingTokens
);
setStatusIndex(index);
setRemainingTokens(tokens);
if (isEmpty(tokens)) return send(RESOLVE);

return send(VERIFY);
}

// Fetch tokens from inventory. Fetch all statuses at the first page.
// Next times, fetch the next page of the current status.
onFetchTokenInventory(
recordState,
page && currentStatus, // first page fetches all status
Expand All @@ -144,23 +193,31 @@ export default function useProposalsBatch({
)
.catch((e) => send(REJECT, e))
.then(({ votes, records }) => {
setPreviousPage(page);
setInitializedInventory(true);
// prepare token batch to fetch proposal for given status
const proposalStatusLabel = getProposalStatusLabel(
currentStatus,
isByRecordStatus
);
const tokens = (isByRecordStatus ? records : votes)[
const newTokens = (isByRecordStatus ? records : votes)[
proposalStatusLabel
];
if (!tokens) return send(RESOLVE);
const tokens = [...newTokens, ...remainingTokens];
setRemainingTokens(tokens);
if (
tokens.length < proposalPageSize &&
statusIndex + 1 < currentStatuses.length
)
return send(RESOLVE);

return send(VERIFY);
});

return send(FETCH);
},
verify: () => {
if (!hasRemainingTokens) return send(RESOLVE, { proposals });

const [tokensToFetch, next] = getTokensForProposalsPagination(
remainingTokens,
proposalPageSize
Expand Down Expand Up @@ -192,27 +249,32 @@ export default function useProposalsBatch({
return send(VERIFY);
}
}

return send(RESOLVE);
})
.catch((e) => send(REJECT, e));

return send(FETCH);
},
done: () => {
// If there are not remaining tokens, check if have unscanned status.
// If yes, change the index to the new status and set up new tokens
if (!hasRemainingTokens && statusIndex + 1 < currentStatuses.length) {
const newIndex = statusIndex + 1;
const newStatus = currentStatuses[newIndex];
const unfetchedTokens = getUnfetchedTokens(
// If the remaining tokens is smaller than proposal page size and have not scanned status.
// Check the new status and set up new tokens
if (
remainingTokens.length < proposalPageSize &&
statusIndex + 1 < currentStatuses.length
) {
const nextIndex = statusIndex + 1;
const nextStatus = currentStatuses[nextIndex];
const nextStatusTokens = getUnfetchedTokens(
proposals,
uniq(
allByStatus[
getProposalStatusLabel(newStatus, isByRecordStatus)
getProposalStatusLabel(nextStatus, isByRecordStatus)
] || []
)
);
setRemainingTokens(unfetchedTokens);
setStatusIndex(newIndex);
setRemainingTokens([...remainingTokens, ...nextStatusTokens]);
setStatusIndex(nextIndex);
if (!values(proposals).length) {
// If no proposals are fetched yet.
// Continue the fetching cycle to fetch the first page.
Expand All @@ -238,6 +300,7 @@ export default function useProposalsBatch({
let unfetchedTokens = [],
index = 0;
const statuses = newStatuses || currentStatuses;

const foundPreviousSessionStatus = statuses.find((status, i) => {
const statusLabel = getProposalStatusLabel(status, isByRecordStatus);
unfetchedTokens = getUnfetchedTokens(
Expand All @@ -250,6 +313,7 @@ export default function useProposalsBatch({
}
return false;
});

setRemainingTokens(unfetchedTokens);
setStatusIndex(index);
if (isByRecordStatus) {
Expand All @@ -273,10 +337,13 @@ export default function useProposalsBatch({
tokens && tokens.length && tokens.length % INVENTORY_PAGE_SIZE === 0;

const onFetchMoreProposals = useCallback(() => {
if (isEmpty(remainingTokens) && isAnotherInventoryCallRequired)
if (
remainingTokens.length < proposalPageSize &&
isAnotherInventoryCallRequired
)
return send(START);
return send(VERIFY);
}, [send, remainingTokens, isAnotherInventoryCallRequired]);
}, [send, remainingTokens, proposalPageSize, isAnotherInventoryCallRequired]);

const anyError = error || state.error;
useThrowError(anyError);
Expand Down
17 changes: 11 additions & 6 deletions teste2e/cypress/e2e/records/recordsList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe("Records list", () => {
});
cy.wait("@records.records");
// each proposal should be rendered accordingly to inventory response
cy.assertListLengthByTestId("record-title", RECORDS_PAGE_SIZE + 3) // first records batch
cy.assertListLengthByTestId("record-title", RECORDS_PAGE_SIZE) // first records batch
.each(([{ id }], position) => {
const tokens = getTokensByStatusTab(inventory, "Under Review");
const expectedToken = shortRecordToken(tokens[position]);
Expand All @@ -50,22 +50,27 @@ describe("Records list", () => {
it("can render records and inventory pagination correctly", () => {
cy.visit(`/`);
cy.wait("@ticketvote.inventory");
// Fetch 'started', 'authorized' and 'unauthorized' proposals
cy.wait("@records.records");
cy.assertListLengthByTestId("record-title", 8);
// 3 items of started status and 2 items of unauthorized status
cy.assertListLengthByTestId("record-title", 5);
cy.scrollTo("bottom");
cy.wait("@records.records");
cy.assertListLengthByTestId("record-title", 13);
cy.assertListLengthByTestId("record-title", 10);
cy.scrollTo("bottom");
cy.wait("@records.records");
cy.assertListLengthByTestId("record-title", 18);
cy.assertListLengthByTestId("record-title", 15);
cy.scrollTo("bottom");
cy.wait("@records.records");
cy.assertListLengthByTestId("record-title", 23);
cy.assertListLengthByTestId("record-title", 20);
// finished first inventory page
cy.scrollTo("bottom");
cy.wait("@ticketvote.inventory").its("request.body.page").should("eq", 2);
cy.wait("@records.records");
// records from second inventory page
cy.assertListLengthByTestId("record-title", 25);
cy.scrollTo("bottom");
cy.wait("@records.records");
cy.assertListLengthByTestId("record-title", 28);
cy.scrollTo("bottom");
// wait to see if no requests are done, since inventory is fully fetched
Expand All @@ -80,7 +85,7 @@ describe("Records list", () => {
// navigate to in discussion tab
cy.findByTestId("tab-0").click();
cy.wait("@records.records");
cy.assertListLengthByTestId("record-title", 8);
cy.assertListLengthByTestId("record-title", 5);
});
it("can list legacy proposals", () => {
// for approved proposals
Expand Down