External Supplier Quotes #377
Conversation
- updated supplier quote status to support new enum values
- added new share link and UI for digital supplier quote
- added error handling to services
jeebeez
changed the title
- Replacing Active with Draft everywhere
| DROP POLICY IF EXISTS "Employees with purchasing_view can view purchasing-related external links" ON "externalLink"; | ||
| CREATE POLICY "Employees with purchasing_view can view purchasing-related external links" ON "externalLink" | ||
| FOR SELECT | ||
| USING ( | ||
| "documentType" = 'SupplierQuote' AND | ||
| has_role('employee', "companyId") AND | ||
| has_company_permission('purchasing_view', "companyId") | ||
| ); | ||
|
|
||
| -- Insert | ||
| DROP POLICY IF EXISTS "Employees with purchasing_create can insert purchasing-related external links" ON "externalLink"; | ||
| CREATE POLICY "Employees with purchasing_create can insert purchasing-related external links" ON "externalLink" | ||
| FOR INSERT | ||
| WITH CHECK ( | ||
| "documentType" = 'SupplierQuote' AND | ||
| has_role('employee', "companyId") AND | ||
| has_company_permission('purchasing_create', "companyId") | ||
| ); | ||
|
|
||
| -- Update | ||
| DROP POLICY IF EXISTS "Employees with purchasing_update can update purchasing-related external links" ON "externalLink"; | ||
| CREATE POLICY "Employees with purchasing_update can update purchasing-related external links" ON "externalLink" | ||
| FOR UPDATE | ||
| USING ( | ||
| "documentType" = 'SupplierQuote' AND | ||
| has_role('employee', "companyId") AND | ||
| has_company_permission('purchasing_update', "companyId") | ||
| ); | ||
|
|
||
| -- Delete | ||
| DROP POLICY IF EXISTS "Employees with purchasing_delete can delete purchasing-related external links" ON "externalLink"; | ||
| CREATE POLICY "Employees with purchasing_delete can delete purchasing-related external links" ON "externalLink" | ||
| FOR DELETE | ||
| USING ( | ||
| "documentType" = 'SupplierQuote' AND | ||
| has_role('employee', "companyId") AND | ||
| has_company_permission('purchasing_delete', "companyId") | ||
| ); |
There was a problem hiding this comment.
One thing I've learned so far is that it's very non-performant to have multiple RLS policies for a single table like this. It would be great if you could consolidate all the policies for this table into a single policy for each (SELECT, INSERT, UPDATE, DELETE)
For the select policy we can do something simple like:
CREATE POLICY "SELECT" ON "externalLink"
FOR SELECT
USING (
"companyId" = ANY (
(
SELECT
get_companies_with_employee_role()
)::text[]
)
);And then for INSERT/UPDATE/DELETE, we can add in the specific documentType/permission combo:
I'm not sure exactly what the code would be but something like:
CREATE POLICY "INSERT" ON "public"."group"
FOR INSERT WITH CHECK (
"documentType" = 'SupplierQuote' AND "companyId" = ANY (
(
SELECT
get_companies_with_employee_permission('purchasing_create')
)::text[]
) OR
"documentType" = 'Quote' AND "companyId" = ANY (
(
SELECT
get_companies_with_employee_permission('sales_create')
)::text[]
)
);
| exchangeRateUpdatedAt: zfd.text(z.string().optional()), | ||
| }); | ||
|
|
||
| export const externalSupplierQuoteValidator = z.object({ |
There was a problem hiding this comment.
Looks good, but let's try to keep these alphabetized
| } | ||
|
|
||
| export async function finalizeSupplierQuote( | ||
| client: SupabaseClient<Database>, |
There was a problem hiding this comment.
Same nit, please alphabetize
| }); | ||
|
|
||
| if (externalLink.data) { | ||
| await client |
There was a problem hiding this comment.
what happens if this fails?
There was a problem hiding this comment.
Good catch, my bad will add a try catch.
There was a problem hiding this comment.
it will never error, it just returns a result that is of type: { data: T; error: null; } | { data: null; error: PostgresError; } so we can just do something like:
if (externalLink.data) {
const result = await client.suchAndSuch();
if (result.error) {
// handle the error either by returning it or something
}
}
| const { id } = params; | ||
| if (!id) throw new Error("Could not find supplier quote id"); | ||
|
|
||
| const [quote] = await Promise.all([getSupplierQuote(client, id)]); |
There was a problem hiding this comment.
Why are we doing a Promise.all on a single item?
There was a problem hiding this comment.
I'm fine with keeping these. It's a little silly in this case, but it makes it easy to rewrite to fetch more data.
There was a problem hiding this comment.
Yeah, exactly, saw the pattern everywhere else, and it makes sense. This is to ensure that subsequent loading operations are performed in a similar manner. At least, that was my interpretation 😅
|
|
||
| // Reuse existing external link or create one if it doesn't exist | ||
| const [externalLink] = await Promise.all([ | ||
| upsertExternalLink(client, { |
|
|
||
| // Reuse existing external link or create one if it doesn't exist | ||
| const [externalLink] = await Promise.all([ | ||
| upsertExternalLink(client, { |
| const { id } = params; | ||
| if (!id) throw new Error("Could not find supplier quote id"); | ||
|
|
||
| const [quote] = await Promise.all([getSupplierQuote(client, id)]); |
sidwebworks
left a comment
sidwebworks
left a comment
There was a problem hiding this comment.
LGTM, just a few comments
|
Looks really nice! Couple UI updates:
|
|
Looks great overall. Just a few things to polish up. |
Co-authored-by: Brad Barbin <[email protected]>
Co-authored-by: Brad Barbin <[email protected]>
Got it done. Just confused about this one. When you usually add a line item the default price is zero right so shouldn't we only disable the Finalize button and not the Send button |
What does this PR do?
Added digital supplier supplier quotes
Fixes Digital Supplier Quotes #300 (GitHub issue number)
Visual Demo (For contributors especially)
Video Demo (if applicable):
https://www.loom.com/share/9502f7d06a7a4cbcb3093bf25aa9e444
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?