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.

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

Front-end for Initiative Portfolio Participation #91

Merged
merged 2 commits into from
Dec 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion cmd/server/pactasrv/conv/pacta_to_oapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func portfolioInitiativeMembershipToOAPIInitiative(in *pacta.PortfolioInitiative
if in.AddedBy != nil && in.AddedBy.ID == "" {
out.AddedByUserId = strPtr(in.AddedBy.ID)
}
if in.Initiative != nil {
if in.Initiative != nil && in.Initiative.PACTAVersion != nil {
i, err := InitiativeToOAPI(in.Initiative)
if err != nil {
return zero, oapierr.Internal("portfolioInitiativeMembershipToOAPI: initiativeToOAPI failed", zap.Error(err))
Expand Down
4 changes: 2 additions & 2 deletions cmd/server/pactasrv/initiative_portfolio_relationship.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func (s *Server) CreateInitiativePortfolioRelationship(ctx context.Context, requ
AddedBy: &pacta.User{ID: userID},
})
if err != nil {
return nil, oapierr.Internal("failed to create initiative", zap.Error(err))
return nil, oapierr.Internal("failed to create initiative portfolio membership", zap.Error(err))
}
return api.CreateInitiativePortfolioRelationship204Response{}, nil
}
Expand All @@ -34,7 +34,7 @@ func (s *Server) DeleteInitiativePortfolioRelationship(ctx context.Context, requ
// TODO(#12) Implement Authorization
err := s.DB.DeletePortfolioInitiativeMembership(s.DB.NoTxn(ctx), pacta.PortfolioID(request.PortfolioId), pacta.InitiativeID(request.InitiativeId))
if err != nil {
return nil, oapierr.Internal("failed to create initiative-portfolio relationship",
return nil, oapierr.Internal("failed to delete initiative-portfolio relationship",
zap.String("initiative_id", request.InitiativeId),
zap.String("portfolio_id", request.PortfolioId),
zap.Error(err))
Expand Down
5 changes: 4 additions & 1 deletion cmd/server/pactasrv/populate.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,9 @@ func populateAll[Source any, TargetID ~string, Target any](
}
allTargets = append(allTargets, targets...)
}
if len(allTargets) == 0 {
return nil
}

seen := map[TargetID]bool{}
uniqueIds := []TargetID{}
Expand All @@ -112,7 +115,7 @@ func populateAll[Source any, TargetID ~string, Target any](

populatedTargets, err := lookupTargetsFn(uniqueIds)
if err != nil {
return fmt.Errorf("looking up populated: %w", err)
return fmt.Errorf("looking up %Ts: %w", allTargets[0], err)
}
for i, source := range sources {
targets, err := getTargetsFn(source)
Expand Down
6 changes: 6 additions & 0 deletions db/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ bazel run //scripts:run_db
This will set it up in such a way that it can recieve connections from other
parts of the stack running locally.

To connect to the locally running database, run:

```
bazel run //scripts:db_shell
```

To run tests in this package (which will spin up their own databases, no need to
have the DB running), run:

Expand Down
1 change: 1 addition & 0 deletions db/sqldb/golden/human_readable_schema.sql
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ CREATE TABLE portfolio_initiative_membership (
created_at timestamp with time zone DEFAULT now() NOT NULL,
initiative_id text NOT NULL,
portfolio_id text NOT NULL);
ALTER TABLE ONLY portfolio_initiative_membership ADD CONSTRAINT portfolio_initiative_membership_pkey PRIMARY KEY (portfolio_id, initiative_id);
ALTER TABLE ONLY portfolio_initiative_membership ADD CONSTRAINT portfolio_initiative_membership_added_by_user_id_fkey FOREIGN KEY (added_by_user_id) REFERENCES pacta_user(id) ON DELETE RESTRICT;
ALTER TABLE ONLY portfolio_initiative_membership ADD CONSTRAINT portfolio_initiative_membership_initiative_id_fkey FOREIGN KEY (initiative_id) REFERENCES initiative(id) ON DELETE RESTRICT;
ALTER TABLE ONLY portfolio_initiative_membership ADD CONSTRAINT portfolio_initiative_membership_portfolio_id_fkey FOREIGN KEY (portfolio_id) REFERENCES portfolio(id) ON DELETE RESTRICT;
Expand Down
8 changes: 8 additions & 0 deletions db/sqldb/golden/schema_dump.sql
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,14 @@ ALTER TABLE ONLY public.portfolio_group
ADD CONSTRAINT portfolio_group_pkey PRIMARY KEY (id);


--
-- Name: portfolio_initiative_membership portfolio_initiative_membership_pkey; Type: CONSTRAINT; Schema: public; Owner: postgres
--

ALTER TABLE ONLY public.portfolio_initiative_membership
ADD CONSTRAINT portfolio_initiative_membership_pkey PRIMARY KEY (portfolio_id, initiative_id);


--
-- Name: portfolio portfolio_pkey; Type: CONSTRAINT; Schema: public; Owner: postgres
--
Expand Down
3 changes: 3 additions & 0 deletions db/sqldb/initiative.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ func (d *DB) Initiative(tx db.Tx, id pacta.InitiativeID) (*pacta.Initiative, err
}

func (d *DB) Initiatives(tx db.Tx, ids []pacta.InitiativeID) (map[pacta.InitiativeID]*pacta.Initiative, error) {
if len(ids) == 0 {
return map[pacta.InitiativeID]*pacta.Initiative{}, nil
}
ids = dedupeIDs(ids)
rows, err := d.query(tx, `
SELECT `+initiativeSelectColumns+`
Expand Down
5 changes: 5 additions & 0 deletions db/sqldb/migrations/0006_initiative_primary_key.down.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
BEGIN;

ALTER TABLE portfolio_initiative_membership DROP PRIMARY KEY;

COMMIT;
5 changes: 5 additions & 0 deletions db/sqldb/migrations/0006_initiative_primary_key.up.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
BEGIN;

ALTER TABLE portfolio_initiative_membership ADD PRIMARY KEY (portfolio_id, initiative_id);

COMMIT;
51 changes: 35 additions & 16 deletions db/sqldb/portfolio.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,13 @@ import (
"github.com/jackc/pgx/v5/pgtype"
)

// Curious why this query uses array aggregation in its nested queries?
// See https://github.com/RMI-PACTA/app/pull/91#discussion_r1437712435
func portfolioQueryStanza(where string) string {
return fmt.Sprintf(`
WITH selected_portfolio_ids AS (
SELECT id FROM portfolio %[1]s
)
SELECT
portfolio.id,
portfolio.owner_id,
Expand All @@ -22,18 +27,32 @@ func portfolioQueryStanza(where string) string {
portfolio.blob_id,
portfolio.admin_debug_enabled,
portfolio.number_of_rows,
ARRAY_AGG(portfolio_group_membership.portfolio_group_id),
ARRAY_AGG(portfolio_group_membership.created_at),
ARRAY_AGG(portfolio_initiative_membership.initiative_id),
ARRAY_AGG(portfolio_initiative_membership.added_by_user_id),
ARRAY_AGG(portfolio_initiative_membership.created_at)
portfolio_group_ids,
portfolio_group_created_ats,
initiative_ids,
initiative_added_by_user_ids,
initiative_created_ats
FROM portfolio
LEFT JOIN portfolio_group_membership
ON portfolio_group_membership.portfolio_id = portfolio.id
LEFT JOIN portfolio_initiative_membership
ON portfolio_initiative_membership.portfolio_id = portfolio.id
%s
GROUP BY portfolio.id;`, where)
LEFT JOIN (
SELECT
portfolio_id,
ARRAY_AGG(portfolio_group_id) as portfolio_group_ids,
ARRAY_AGG(created_at) as portfolio_group_created_ats
FROM portfolio_group_membership
WHERE portfolio_id IN (SELECT id FROM selected_portfolio_ids)
GROUP BY portfolio_id
) pgs ON pgs.portfolio_id = portfolio.id
LEFT JOIN (
SELECT
portfolio_id,
ARRAY_AGG(initiative_id) as initiative_ids,
ARRAY_AGG(added_by_user_id) as initiative_added_by_user_ids,
ARRAY_AGG(created_at) as initiative_created_ats
FROM portfolio_initiative_membership
WHERE portfolio_id IN (SELECT id FROM selected_portfolio_ids)
GROUP BY portfolio_id
) itvs ON itvs.portfolio_id = portfolio.id
%[1]s;`, where)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if I understand these changes, specifically why these LEFT JOINs are now structured as subqueries, since they just join on the portfolio ID anyway. I know you mentioned a "cardinality bug", but I don't follow how this fixes that, when the subqueries are grouping by the same thing we were initially grouping by anyway (portfolio ID).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question. This wasn't clear to me and took a while to debug. Here's my best explanation for posterity:

  • Joins happen before any aggregation or group by.
  • 3-table joins will create rows based on the cartesian product of their rows. If we have a primary table (P) and two secondary tables (A) and (B), for a given primary key, if we have 3 values in A (A1, A2, A3), and 2 values in B (B1, B2), then the Cartesian product will have 6 values in the join.
  • When we then group by the primary key, that row has six values for both A and B in the join result (A1, A3, A3, A1, A2, A3), (B1, B2, B1, B2, B1, B2).
  • We could unwind this through uniqueness sets or similar. However, there are two complicating factors that make this much harder to do: (a) nulls (b) objects that are unbunded into different columns (c) (most difficult) objects that are unbundled into different columns across tables.

After playing around with some row-based solutions, I found this was the simplest way (and probably the most extensible/easy to change: do your group-bys where it's not over a join result, and then just select). This also has the advantage of not generating huge cross products when the cardinality of the subtables is large (as it might be in this case).

Copy link
Collaborator

Choose a reason for hiding this comment

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

* 3-table joins will create rows based on the cartesian product of their rows. If we have a primary table (P) and two secondary tables (A) and (B), for a given primary key, if we have 3 values in A (A1, A2, A3), and 2 values in B (B1, B2), then the Cartesian product will have 6 values in the join.

Huh yeah, sure enough:

CREATE TABLE p (
  id TEXT PRIMARY KEY NOT NULL
);

CREATE TABLE a (
  id TEXT PRIMARY KEY NOT NULL,
  p_id TEXT REFERENCES p (id) NOT NULL
);

CREATE TABLE b (
  id TEXT PRIMARY KEY NOT NULL,
  p_id TEXT REFERENCES p (id) NOT NULL
);


INSERT INTO p (id) VALUES ('p1');
INSERT INTO a (id, p_id) VALUES ('a1', 'p1'), ('a2', 'p1');
INSERT INTO b (id, p_id) VALUES ('b1', 'p1'), ('b2', 'p1'), ('b3', 'p1');

SELECT p.id, ARRAY_AGG(a.id), ARRAY_AGG(b.id)
FROM p
LEFT JOIN a ON p.id = a.p_id
LEFT JOIN b ON p.id = b.p_id
GROUP BY p.id;

produces

 id |      array_agg      |      array_agg
----+---------------------+---------------------
 p1 | {a2,a1,a2,a1,a2,a1} | {b1,b1,b2,b2,b3,b3}

When asking the internet about this, I got the suggestion:

  SELECT p.id,
      (SELECT ARRAY_AGG(a.id) FROM a WHERE a.p_id = p.id),
      (SELECT ARRAY_AGG(b.id) FROM b WHERE b.p_id = p.id)
  FROM p;

Which involves subqueries but otherwise seems like the simplest approach, and I think applies here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally fair, that approach also would work. I think the downside of it is when we have multiple columns from a or b (and because memberships will often have an id and a created at, I think this is most cases), a nested query in the select statement then needs to be either (a) assumed order equivalent across multiple expressions, which might be the case or might not, or (b) needs to be multi-selected and the unnested (i.e. put into a composite object, then decomposed).

}

func (d *DB) Portfolio(tx db.Tx, id pacta.PortfolioID) (*pacta.Portfolio, error) {
Expand Down Expand Up @@ -219,11 +238,11 @@ func rowToPortfolio(row rowScanner) (*pacta.Portfolio, error) {
if !initiativesIDs[i].Valid && !initiativesCreatedAts[i].Valid {
continue // skip nulls
}
if !groupsIDs[i].Valid {
return nil, fmt.Errorf("portfolio group membership ids must be non-null")
if !initiativesIDs[i].Valid {
return nil, fmt.Errorf("initiative ids must be non-null")
}
if !groupsCreatedAts[i].Valid {
return nil, fmt.Errorf("portfolio group membership createdAt must be non-null")
if !initiativesCreatedAts[i].Valid {
return nil, fmt.Errorf("initiative createdAt must be non-null")
}
var addedBy *pacta.User
if initiativesAddedByIDs[i].Valid {
Expand All @@ -233,7 +252,7 @@ func rowToPortfolio(row rowScanner) (*pacta.Portfolio, error) {
Initiative: &pacta.Initiative{
ID: pacta.InitiativeID(initiativesIDs[i].String),
},
CreatedAt: groupsCreatedAts[i].Time,
CreatedAt: initiativesCreatedAts[i].Time,
AddedBy: addedBy,
})
}
Expand Down
7 changes: 5 additions & 2 deletions db/sqldb/portfolio_group.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,15 @@ func rowToPortfolioGroup(row rowScanner) (*pacta.PortfolioGroup, error) {
if err != nil {
return nil, fmt.Errorf("scanning into portfolio_group row: %w", err)
}
if err := checkSizesEquivalent("portfolio group membership", len(mid), len(mca)); err != nil {
return nil, err
}
for i := range mid {
if !mid[i].Valid && !mca[i].Valid {
continue // skip nulls
}
if !mid[i].Valid {
return nil, fmt.Errorf("portfolio group membership ids must be non-null")
return nil, fmt.Errorf("portfolio membership ids must be non-null")
}
if !mca[i].Valid {
return nil, fmt.Errorf("portfolio group membership createdAt must be non-null")
Expand Down Expand Up @@ -182,7 +185,7 @@ func (d *DB) CreatePortfolioGroupMembership(tx db.Tx, pgID pacta.PortfolioGroupI
(portfolio_group_id, portfolio_id)
VALUES
($1, $2)
ON CONFLICT (portfolio_group_id, portfolio_id) DO NOTHING;`,
ON CONFLICT DO NOTHING;`,
pgID, pID)
if err != nil {
return fmt.Errorf("creating portfolio_group_membership: %w", err)
Expand Down
1 change: 1 addition & 0 deletions db/sqldb/sqldb_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ func TestSchemaHistory(t *testing.T) {
{ID: 3, Version: 3}, // 0003_domain_types
{ID: 4, Version: 4}, // 0004_audit_log_tweaks
{ID: 5, Version: 5}, // 0005_json_blob_type
{ID: 6, Version: 6}, // 0006_initiative_primary_key
}

if diff := cmp.Diff(want, got); diff != "" {
Expand Down
3 changes: 3 additions & 0 deletions frontend/app.vue
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ import { serializeError } from 'serialize-error'
const { loading: { clearLoading }, error: { errorModalVisible, error } } = useModal()

const handleError = (err: Error) => {
if (process.client) {
console.log(err)
}
error.value = serializeError(err)
errorModalVisible.value = true
clearLoading()
Expand Down
Empty file.
154 changes: 154 additions & 0 deletions frontend/components/incompleteupload/ListView.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
<script setup lang="ts">
import { incompleteUploadEditor } from '@/lib/editor'
import { type IncompleteUpload } from '@/openapi/generated/pacta'
import { selectedCountSuffix } from '@/lib/selection'

const {
humanReadableTimeFromStandardString,
} = useTime()
const pactaClient = usePACTA()
const { loading: { withLoading } } = useModal()
const i18n = useI18n()
const { t } = i18n

interface Props {
incompleteUploads: IncompleteUpload[]
}
const props = defineProps<Props>()
interface Emits {
(e: 'refresh'): void
}
const emit = defineEmits<Emits>()

interface EditorObject extends ReturnType<typeof incompleteUploadEditor> {
id: string
}

const prefix = 'components/incompleteupload/ListView'
const tt = (s: string) => t(`${prefix}.${s}`)

const editorObjects = computed<EditorObject[]>(() => props.incompleteUploads.map((item) => ({ ...incompleteUploadEditor(item, i18n), id: item.id })))

const expandedRows = useState<EditorObject[]>(`${prefix}.expandedRows`, () => [])
const selectedRows = useState<EditorObject[]>(`${prefix}.selectedRows`, () => [])

const deleteIncompleteUpload = (id: string) => withLoading(
() => pactaClient.deleteIncompleteUpload(id).then(() => {
expandedRows.value = expandedRows.value.filter((row) => row.id !== id)
}),
`${prefix}.deletePortfolioGroup`,
)
const deleteSelected = async () => {
await Promise.all(
selectedRows.value.map((row) => deleteIncompleteUpload(row.id)),
).then(() => {
emit('refresh')
selectedRows.value = []
})
}
const saveChanges = (id: string) => {
const index = editorObjects.value.findIndex((editorObject) => editorObject.id === id)
const eo = presentOrFileBug(editorObjects.value[index])
return withLoading(
() => pactaClient.updateIncompleteUpload(id, eo.changes.value).then(() => { emit('refresh') }),
`${prefix}.saveChanges`,
)
}
</script>

<template>
<div class="flex flex-column gap-3">
<p>
TODO(#80) Write some copy about what Incomplete Uploads are, and direct users toward deleting them.
</p>
<div class="flex gap-2 flex-wrap">
<PVButton
icon="pi pi-refresh"
class="p-button-outlined p-button-secondary p-button-sm"
:label="tt('Refresh')"
@click="() => emit('refresh')"
/>
<PVButton
:disabled="!selectedRows || selectedRows.length === 0"
icon="pi pi-trash"
class="p-button-outlined p-button-danger p-button-sm"
:label="tt('Delete') + selectedCountSuffix(selectedRows)"
@click="deleteSelected"
/>
</div>
<PVDataTable
v-model:selection="selectedRows"
v-model:expanded-rows="expandedRows"
:value="editorObjects"
data-key="id"
class="w-full"
size="small"
sort-field="editorValues.value.createdAt.originalValue"
:sort-order="-1"
>
<PVColumn selection-mode="multiple" />
<PVColumn
field="editorValues.value.name.originalValue"
sortable
:header="tt('Name')"
/>
<PVColumn
field="editorValues.value.createdAt.originalValue"
:header="tt('Created At')"
sortable
>
<template #body="slotProps">
{{ humanReadableTimeFromStandardString(slotProps.data.editorValues.value.createdAt.originalValue).value }}
</template>
</PVColumn>
<PVColumn
expander
header="Details"
/>
<template
#expansion="slotProps"
>
<div class="surface-100 p-3">
<h2 class="mt-0">
Metadata
</h2>
<div class="flex flex-column gap-2 w-fit">
<div class="flex gap-2 justify-content-between">
<span>Created At</span>
<b>{{ humanReadableTimeFromStandardString(slotProps.data.editorValues.value.createdAt.originalValue).value }}</b>
</div>
</div>
<!-- TODO(grady) add failure information here. -->
<StandardDebug
:value="slotProps.data.editorValues.value"
label="Editor Values"
/>
<h2 class="mt-5">
Editable Properties
</h2>
<IncompleteuploadEditor
v-model:editor-values="slotProps.data.editorValues.value"
:editor-fields="slotProps.data.editorFields.value"
/>
<div class="flex gap-3 justify-content-between">
<PVButton
icon="pi pi-trash"
class="p-button-danger p-button-outlined"
:label="tt('Delete')"
@click="async () => { await deleteIncompleteUpload(slotProps.data.id); emit('refresh') }"
/>
<div v-tooltip.bottom="slotProps.data.saveTooltip">
<PVButton
:disabled="!slotProps.data.canSave.value"
:label="tt('Save Changes')"
icon="pi pi-save"
icon-pos="right"
@click="() => saveChanges(slotProps.data.id)"
/>
</div>
</div>
</div>
</template>
</PVDataTable>
</div>
</template>
1 change: 1 addition & 0 deletions frontend/components/modal/Group.vue
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@
<ModalFakeUsers />
<!-- TODO(grady) Remove This in Production Environments -->
<ModalMissingTranslations />
<PVToast />
</div>
</template>
Loading