-
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
feat: multiple billing status changes. #2629
feat: multiple billing status changes. #2629
Conversation
This diff consumes the new pi API setting `billingstatuschangesmax`, which should be used as the maximum allowed billing status changes. After this commit all approved proposals with less billing status changes than the maximum will have the `Set Biiling Status` enabled.
needs a e2e test. |
billing status changes in pages.
/billingstatuschanges
.
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.
tACK
one thing to keep in mind that in some cases we gonna show the loading indicator |
1 makes sense. Not much we can do about that. For 2, I think the loading indicator should be displayed until all required data has been fetched. I'd rather we display the loading indcator a little longer then necessary for proposals that don't need the button then to add elements to the DOM after the list views have already been rendered. |
sweet, will add an indicator |
I had some additional thoughts on this. You can improve the performance if you send the billing status changes request concurrently with the other requests. You don't really need to wait for the vote summary to be returned. You already know the approved proposals from looking at the inventory reply. Fetch the billing statuses for the approved proposals if the user is an admin. |
/billingstatuschanges
.
After some discussions on matrix we came up with the following TODOs:
|
Addressed in decred/politeia#1543. |
…then in a separate redux store slice.
ditch loading spinner
Progress:
Screen recording of list view with 7 approved proposals: Screen.Recording.2021-10-13.at.21.38.03.mov |
Normal user: |
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.
UX looks good. Found two bugs with the requests though.
- The billing status changes request is being sent on initial load of the
Under Review
tab. It's requesting the approved proposals, but it's being sent on the wrong tab. - On the proposal details page, the
comments/v1/votes
request is being sent twice. This duplicate request behavior is not present on master.
Addressed. |
|
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.
which is already does the pagination
@lukebp addressed by moving the fetching from |
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.
tACK on firefox
Needs e2e tests and code approval from @tiagoalvesdulce.
@lukebp @tiagoalvesdulce here are the e2e results of ccaa60c:
|
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.
Code LGTM. You left a few console.logs
.
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.
ACK
This diff consumes the new pi API setting
billingstatuschangesmax
,which should be used as the maximum allowed billing status changes.
After this commit all approved proposals with less billing status
changes than the maximum will have the
Set Biiling Status
button enabled for admins.
This also uses the batched
/billingstatuschanges
route by usinguseMachineFetch
inuseBillingStatusChanges
to fetch thebilling status changes of all approved proposals page by page.
Closes #2627.