feat: Add reject reason #1295
feat: Add reject reason #1295
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1295 +/- ##
==========================================
+ Coverage 81.00% 81.66% +0.66%
==========================================
Files 66 66
Lines 4500 4522 +22
Branches 776 786 +10
==========================================
+ Hits 3645 3693 +48
+ Misses 840 814 -26
Partials 15 15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# Conflicts: # src/ui/views/PushDetails/PushDetails.tsx # test/plugin/plugin.test.js # test/testPush.test.js
jescalada
left a comment
jescalada
left a comment
There was a problem hiding this comment.
Thanks for the contribution, @andypols! The main flow works good so far 🚀
Just wondering if we could keep the UI services refactor for another PR - just so that we don't remove error handling and setAuth without setting up an alternative.
I'm also having issues rendering the Push Details for pushes that errored out (which might have missing parameters):
I'm also wondering about the default ordering for the pushes. I think these are coming from the backend without any particular ordering - would be nice to have the newest or oldest at the top for all tabs:
Since the changes are rather significant (and we will likely follow up with the ui/services refactor and other UI improvements), we're probably going to release this in v2.1. Does that work for you? 🤔
Pinging @kriswest to check this out since we don't make heavy use of the UI.
| }; | ||
|
|
||
| export const reject = async (id: string, attestation: any): Promise<{ message: string }> => { | ||
| export const reject = async (id: string, rejection: any): Promise<{ message: string }> => { |
There was a problem hiding this comment.
Might want to add the Rejection type here!
| }; | ||
|
|
||
| export const authorise = async (id: string, attestation: any): Promise<{ message: string }> => { | ||
| export const authorise = async (id: string, rejection: any): Promise<{ message: string }> => { |
There was a problem hiding this comment.
Same as above, except I'm wondering if it makes sense to include the rejection string here - since we're authorising (and the authorisation "reason" would be implicit in the attestation.questions).
Would be great to have an Approval object here instead (formerly Attestation type)
| }; | ||
| timestamp: string | Date; | ||
| reason: string; | ||
| }; |
There was a problem hiding this comment.
I understand a bit better now - perhaps we could have a Review base object, rename Attestation to Approval and keep Rejection? Then the database function and API routes only need one of them.
| setPushes: (pushes: PushActionView[]) => void, | ||
| setAuth: (auth: boolean) => void, | ||
| setIsError: (isError: boolean) => void, | ||
| setErrorMessage: (errorMessage: string) => void, |
There was a problem hiding this comment.
Wondering if it's okay to remove the error handling here - I know it's not the cleanest right now, but it'd be best to refactor all the src/ui/services systematically and in a single PR.
I'm inclined to keep the error handling for now until we start overhauling the whole src/ui/services module into something cleaner.
There's an issue to track this: #1188
There was a problem hiding this comment.
I'm not really removing error handling, It's more removing unused code. The error/auth value are never used. Add back code like this really hurts :)
const [, setIsError] = useState(false);
const navigate = useNavigate();
const [, setAuth] = useState(true);
|
@jescalada Thanks for reviewing
I will look into this. Can you give me the scenario so I can fix it?
Yes that is true. I was worried this was already too large, so had left that out.
We’ve already made the changes on our side and don’t depend on an upstream release, so the target version isn’t critical for us. That said, the current UI is borderline unusable in practice for people who rely on it day-to-day — @kriswest raised some of this at the last meeting, and I suspect it reflects that many in the community don’t use the UI much. If most users aren’t using it, then waiting is fine; but if we do want people to use the UI, I think v2.1 would really benefit from including these improvements (or similar ones). I’m happy to go with whatever release plan works best for you and the project — I care more about forward progress and usability than the specific version number. |
2 participants
Summary
This PR addresses #1030. Previously, push rejections were recorded without any context. Reviewers now must provide an explanation when rejecting a push, and that explanation is stored and displayed so contributors can understand why their changes were not approved.
Solution
Backend
db.reject(file and Mongo) to accept a rejection object.Actionvia a newrejectionproperty./push/:id/rejectto require a rejection reason.UI
PushDetailsinto smaller components for clarity and easier maintenance:PushStatusHeaderStatusIconAttestationRejectionApprovalBadgeRejectionBadgeRejectionBadgeandApprovalBadgecomponents to display:Screenshot
Reject button is disabled by default.
Button enabled with a reason is given.