-
Notifications
You must be signed in to change notification settings - Fork 56
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.
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
fix: Under review tab pagination bug. #2552
Merged
lukebp
merged 6 commits into
decred:master
from
vibros68:2530_under_review_pagination_bug
Sep 3, 2021
Merged
Changes from 1 commit
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 2d11125
add condition variable, fix scanNextStatusTokens function bug, remove…
vibros68 ad10ad3
reproduce scan tokens logic
vibros68 e70c680
batch pagination testing
vibros68 61a94d2
fix e2e test: records and inventory pagination
vibros68 2636412
clean up code
vibros68 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||||
|
@@ -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(() => { | ||||||||||||
|
@@ -130,12 +132,66 @@ export default function useProposalsBatch({ | |||||||||||
const onFetchProposalsBatch = useAction(act.onFetchProposalsBatch); | ||||||||||||
const onFetchTokenInventory = useAction(act.onFetchTokenInventory); | ||||||||||||
const hasRemainingTokens = !isEmpty(remainingTokens); | ||||||||||||
|
||||||||||||
const scanNextStatusTokens = (index, oldTokens) => { | ||||||||||||
if (index < currentStatuses.length) { | ||||||||||||
const status = currentStatuses[index]; | ||||||||||||
const newTokens = getUnfetchedTokens( | ||||||||||||
proposals, | ||||||||||||
uniq( | ||||||||||||
allByStatus[getProposalStatusLabel(status, isByRecordStatus)] || [] | ||||||||||||
) | ||||||||||||
); | ||||||||||||
const tokens = [...oldTokens, ...newTokens]; | ||||||||||||
if (tokens.length < proposalPageSize) { | ||||||||||||
// scan the next status. | ||||||||||||
return scanNextStatusTokens(index + 1, tokens); | ||||||||||||
} | ||||||||||||
return { | ||||||||||||
index, | ||||||||||||
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 remaining tokens length is greater than proposal page size. | ||||||||||||
// Fetch proposals. | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is redundant, you can remove it. |
||||||||||||
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) | ||||||||||||
] || []; | ||||||||||||
if ( | ||||||||||||
initializedInventory && | ||||||||||||
!( | ||||||||||||
currentStatusTokens.length % INVENTORY_PAGE_SIZE === 0 && | ||||||||||||
currentStatusTokens.length > 0 | ||||||||||||
) | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Long conditions should be attributed to variables, in order to provide a better readability.
Suggested change
|
||||||||||||
) { | ||||||||||||
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 | ||||||||||||
|
@@ -144,17 +200,23 @@ 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); | ||||||||||||
|
@@ -198,21 +260,24 @@ export default function useProposalsBatch({ | |||||||||||
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. | ||||||||||||
|
@@ -250,6 +315,7 @@ export default function useProposalsBatch({ | |||||||||||
} | ||||||||||||
return false; | ||||||||||||
}); | ||||||||||||
|
||||||||||||
setRemainingTokens(unfetchedTokens); | ||||||||||||
setStatusIndex(index); | ||||||||||||
if (isByRecordStatus) { | ||||||||||||
|
@@ -273,10 +339,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); | ||||||||||||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant comment.