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.

Sign up for GitHub

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

Improve Preparation form links to Interactions #6110

Open
wants to merge 17 commits into
base: production
Choose a base branch
from
+216 −159
Open

Improve Preparation form links to Interactions #6110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
d76789b
Update Interaction localization
grantfitzsimmons Jan 17, 2025
daa990c
Add Disposal to Interactions dialog
grantfitzsimmons Jan 17, 2025
4800238
Update interactions.ts
grantfitzsimmons Jan 17, 2025
eaf9a6c
Hide interactions when none are available
grantfitzsimmons Jan 17, 2025
0c27b37
Remove unused text
grantfitzsimmons Jan 17, 2025
81067b5
Use localized Preparation table name
grantfitzsimmons Jan 18, 2025
416dfb2
Lint code with ESLint and Prettier
grantfitzsimmons Jan 18, 2025
702e118
Improve localization for Preparations
grantfitzsimmons Jan 19, 2025
33caf9b
Resolve conflicts
grantfitzsimmons Jan 19, 2025
aa4dfda
Update interactions.ts
grantfitzsimmons Jan 19, 2025
268110e
Fix no interactions label
grantfitzsimmons Jan 19, 2025
634aef1
Improve Preparation localization further
grantfitzsimmons Jan 19, 2025
b7f1896
Finish localizing preparations
grantfitzsimmons Jan 19, 2025
bd0efb5
Merge branch 'production' into issue-6108
grantfitzsimmons Jan 19, 2025
59802e1
Add tables import
grantfitzsimmons Jan 19, 2025
0371f02
Lint code with ESLint and Prettier
grantfitzsimmons Jan 19, 2025
34b0e89
Update specifyweb/frontend/js_src/lib/components/FormCommands/index.tsx
grantfitzsimmons Jan 21, 2025
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
169 changes: 122 additions & 47 deletions specifyweb/frontend/js_src/lib/components/FormCommands/ShowTransactions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,74 +100,149 @@ export function ShowLoansCommand({
domainFilter: false,
}).then(({ records }) => records.map(deserializeResource))
: undefined,
exchanges: hasTablePermission('ExchangeOutPrep', 'read')
exchangeOuts: hasTablePermission('ExchangeOutPrep', 'read')
? fetchCollection('ExchangeOutPrep', {
limit: DEFAULT_FETCH_LIMIT,
preparation: preparation.get('id'),
domainFilter: false,
}).then(({ records }) => records.map(deserializeResource))
: undefined,
exchangeIns: hasTablePermission('ExchangeInPrep', 'read')
? fetchCollection('ExchangeInPrep', {
limit: DEFAULT_FETCH_LIMIT,
preparation: preparation.get('id'),
domainFilter: false,
}).then(({ records }) => records.map(deserializeResource))
: undefined,
Comment on lines +110 to +116
Copy link
Contributor

@melton-jason melton-jason Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

While probably not in the scope of the PR, there are a few things I noticed about the implementation of the ShowLoansCommand feature.

Conceptually, we are fetching the first 20 InteractionPreparation records associated with the Preparation (e.g., we're fetching al ExchangeInPrep records related to the Preparation in the highlighted section of code), and then we are iterating over each of the InteractionPreparation records and fetching their related Interactions:

specify7/specifyweb/frontend/js_src/lib/components/FormCommands/ShowTransactions.tsx

Lines 34 to 42 in 36f7a7e

const interactions: RA<SpecifyResource<AnySchema>> = await Promise.all(
resources.map(async (resource) => resource.rgetPromise(fieldName))
);
return interactions
.map((resource) => ({
label: resource.get(displayFieldName),
resource,
}))
.sort(sortFunction(({ label }) => label));

(With the highlighted code of this comment, this would be iterating over all of the ExchangeInPrep records and fetching their related ExchangeIn record).

The problem with approach becomes apparent because we do not omit "duplicate" Interaction records in the list of Interactions for a Preparation.
Put more precisely, the Show Transactions Dialog can repeat the same resource multiple times if there are multiple InteractionPreps

Here is a setup demonstrating the Issue: there are 20 Preparations included in ExchangeOut #1, and 1 Preparation included in ExchangeOut #2. Specify fetches the first 20 ExchangeOutPreps related to the Preparation (i.e., all of the ExchangeOutPreps in ExchangeOut #1), then fetches the ExchangeOut related to each of those ExchangeOutPreps and uses that resource to populate the Dialog

fetching_issues.mov

I would assume the purpose of this Dialog is to show related Interaction records? In reality it is showing the InteractionPreparations which all link to the Interaction record.

Besides not showing identical Interaction records, I also think the fetching logic here can be improved!

Currently, the method of fetching the Interaction after fetching the InteractionPreparations can be expensive if each InteractionPreparation exists for a different Interaction: we need a separate network request for each Interaction (this is after the static network request to fetch the InteractionPreparations):

network_requests.mov

We can actually directly use the API to fetch the related Interaction records for a table in a single network request.
Currently, we are constructing a query like /api/specify/interactionprep/?preparation=<PREP_ID>, then iterating over the returned records and constructing queries for /api/specify/interaction/<INTERACTION_ID>/ from the relationship name to the Interaction.

But we can use a query like /api/specify/interaction/?relationshipToInteractionPrep__preparation=<PREP_ID> (e.g., /api/specify/loan/?loanpreparations__preparation=<PREP_ID>) to directly return the collection of Interaction records given a specific preparation id.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have actually worked on a branch - issue-6108-b - to address this problem. It is functionally a complete reimplementation of the Show Transactions Dialog: take a look through the branch and see if there's any ideas you'd like to carry over to this implementation!

Here are some videos comparing production, this branch, and issue-6108-b:

production

production.mov

The error here is a bug in production which occurs whenever there are ExchangeOut records associated with a Preparation

issue-6108

issue-6108.mov

issue-6108-b

issue-6108-b.mov

disposals: hasTablePermission('Disposal', 'read')
? fetchCollection('DisposalPreparation', {
limit: DEFAULT_FETCH_LIMIT,
preparation: preparation.get('id'),
domainFilter: false,
}).then(({ records }) => records.map(deserializeResource))
: undefined,
}),
[preparation]
),
true
);

const hasAnyInteractions = data && [
data.openLoans,
data.resolvedLoans,
data.gifts,
data.exchangeOuts,
data.exchangeIns,
data.disposals,
].some(interactions => Array.isArray(interactions) && interactions.length > 0);

return typeof data === 'object' ? (
<Dialog
buttons={commonText.close()}
header={interactionsText.interactions()}
icon={icons.chat}
onClose={handleClose}
>
<H3 className="flex items-center gap-2">
<TableIcon label name={tables.Loan.name} />
{interactionsText.openLoans({
loanTable: tables.Loan.label,
})}
</H3>
<List
displayFieldName="loanNumber"
fieldName="loan"
resources={data.openLoans ?? []}
/>
<H3 className="flex items-center gap-2">
<TableIcon label name={tables.Loan.name} />
{interactionsText.resolvedLoans({
loanTable: tables.Loan.label,
})}
</H3>
<List
displayFieldName="loanNumber"
fieldName="loan"
resources={data.resolvedLoans ?? []}
/>
<H3 className="flex items-center gap-2">
<TableIcon label name={tables.Gift.name} />
{interactionsText.gifts({
giftTable: tables.Gift.label,
})}
</H3>
<List
displayFieldName="giftNumber"
fieldName="gift"
resources={data.gifts ?? []}
/>
{Array.isArray(data.exchanges) && data.exchanges.length > 0 && (
{!hasAnyInteractions ? (
<>
<H3>
{interactionsText.exchanges({
exhangeInTable: tables.ExchangeIn.label,
exhangeOutTable: tables.ExchangeOut.label,
})}
</H3>
<List
displayFieldName="exchangeOutNumber"
fieldName="exchange"
resources={data.exchanges}
/>
{interactionsText.noInteractions({
preparationTable: String(tables.Preparation.label).toLowerCase(),
})}
</>
) : (
<>
{Array.isArray(data.openLoans) && data.openLoans.length > 0 && (
Copy link
Contributor

Choose a reason for hiding this comment

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

data.openLoans.length > 0

With this assertion, there is actually a slight change in UX compared to the previous implementation, was this intentional?

An empty array is a valid prop to pass to the List component, and it used to show No Results under the table icon.

specify7/specifyweb/frontend/js_src/lib/components/FormCommands/ShowTransactions.tsx

Lines 47 to 49 in 36f7a7e

return resources.length === 0 ? (
<>{commonText.noResults()}</>
) : Array.isArray(entries) ? (

Now, if no results are returned, the Interaction is not shown at all.
I'm all for this change if it was intentional: it cuts down on potentially unneeded information and thus reduces visual clutter!

<>
<H3 className="flex items-center gap-2">
<TableIcon label name={tables.Loan.name} />
{interactionsText.openLoans({
loanTable: tables.Loan.label,
})}
</H3>
<List
displayFieldName="loanNumber"
fieldName="loan"
resources={data.openLoans}
/>
</>
)}
{Array.isArray(data.resolvedLoans) && data.resolvedLoans.length > 0 && (
<>
<H3 className="flex items-center gap-2">
<TableIcon label name={tables.Loan.name} />
{interactionsText.resolvedLoans({
loanTable: tables.Loan.label,
})}
</H3>
<List
displayFieldName="loanNumber"
fieldName="loan"
resources={data.resolvedLoans}
/>
</>
)}
{Array.isArray(data.gifts) && data.gifts.length > 0 && (
<>
<H3 className="flex items-center gap-2">
<TableIcon label name={tables.Gift.name} />
{interactionsText.gifts({
giftTable: tables.Gift.label,
})}
</H3>
<List
displayFieldName="giftNumber"
fieldName="gift"
resources={data.gifts}
/>
</>
)}
{Array.isArray(data.disposals) && data.disposals.length > 0 && (
<>
<H3 className="flex items-center gap-2">
<TableIcon label name={tables.Disposal.name} />
{interactionsText.disposals({
disposalTable: tables.Disposal.label,
})}
</H3>
<List
displayFieldName="disposalNumber"
fieldName="disposal"
resources={data.disposals}
/>
</>
)}
{Array.isArray(data.exchangeOuts) && data.exchangeOuts.length > 0 && (
<>
<H3 className="flex items-center gap-2">
<TableIcon label name={tables.ExchangeOut.name} />
{interactionsText.exchangeOut({
exchangeOutTable: tables.ExchangeOut.label,
})}
</H3>
<List
displayFieldName="exchangeOutNumber"
fieldName="exchangeOut"
resources={data.exchangeOuts}
/>
</>
)}
{Array.isArray(data.exchangeIns) && data.exchangeIns.length > 0 && (
<>
<H3 className="flex items-center gap-2">
<TableIcon label name={tables.ExchangeIn.name} />
{interactionsText.exchangeIn({
exchangeInTable: tables.ExchangeIn.label,
})}
</H3>
<List
displayFieldName="exchangeInNumber"
fieldName="exchangeIn"
resources={data.exchangeIns}
/>
</>
)}
</>
)}
</Dialog>
) : null;
}
}
5 changes: 4 additions & 1 deletion specifyweb/frontend/js_src/lib/components/FormCommands/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { formatDisjunction } from '../Atoms/Internationalization';
import { toTable } from '../DataModel/helpers';
import type { AnySchema } from '../DataModel/helperTypes';
import type { SpecifyResource } from '../DataModel/legacyTypes';
import { tables } from '../DataModel/tables';
import { ErrorBoundary } from '../Errors/ErrorBoundary';
import type { UiCommands } from '../FormParse/commands';
import { LoanReturn } from '../Interactions/LoanReturn';
Expand Down Expand Up @@ -98,7 +99,9 @@ const commandRenderers: {
header={label}
onClose={handleHide}
>
{interactionsText.preparationsCanNotBeReturned()}
{interactionsText.preparationsCanNotBeReturned({
preparationTable: tables.Preparation.label.toLowerCase(),
})}
</Dialog>
) : (
<LoanReturn resource={loan} onClose={handleHide} />
Expand Down
28 changes: 22 additions & 6 deletions specifyweb/frontend/js_src/lib/components/Interactions/InteractionDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,11 +199,15 @@ export function InteractionDialog({
handleClose();
}}
>
{interactionsText.continueWithoutPreparations()}
{interactionsText.continueWithoutPreparations({
preparationTable: String(tables.Preparation.label).toLowerCase(),
})}
</Button.Info>
) : (
<Link.Info href={getResourceViewUrl(actionTable.name)}>
{interactionsText.continueWithoutPreparations()}
{interactionsText.continueWithoutPreparations({
preparationTable: String(tables.Preparation.label).toLowerCase(),
})}
</Link.Info>
)}
{}
Expand All @@ -214,7 +218,9 @@ export function InteractionDialog({
})}
onClose={handleClose}
>
{interactionsText.noPreparationsWarning()}
{interactionsText.noPreparationsWarning({
preparationTable: String(tables.Preparation.label).toLowerCase(),
})}
</Dialog>
)
) : (
Expand Down Expand Up @@ -245,7 +251,9 @@ export function InteractionDialog({
</Button.Secondary>
) : interactionsWithPrepTables.includes(actionTable.name) ? (
<Link.Secondary href={getResourceViewUrl(actionTable.name)}>
{interactionsText.withoutPreparations()}
{interactionsText.withoutPreparations({
preparationTable: String(tables.Preparation.label).toLowerCase(),
})}
</Link.Secondary>
) : undefined}
<span className="-ml-2 flex-1" />
Expand Down Expand Up @@ -419,15 +427,23 @@ function InteractionTextEntry({
<>
{state.missing.length > 0 && (
<>
<H3>{interactionsText.preparationsNotFoundFor()}</H3>
<H3>
{interactionsText.preparationsNotFoundFor({
preparationTable: String(tables.Preparation.label).toLowerCase(),
})}
</H3>
{state.missing.map((problem, index) => (
<p key={index}>{problem}</p>
))}
</>
)}
{state.unavailable.length > 0 && (
<>
<H3>{interactionsText.preparationsNotAvailableFor()}</H3>
<H3>
{interactionsText.preparationsNotAvailableFor({
preparationTable: String(tables.Preparation.label).toLowerCase(),
})}
</H3>
{state.unavailable.map((problem, index) => (
<p key={index}>{problem}</p>
))}
Expand Down
14 changes: 11 additions & 3 deletions specifyweb/frontend/js_src/lib/components/Interactions/LoanReturn.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ export function LoanReturn({
header={tables.LoanPreparation.label}
onClose={handleClose}
>
{interactionsText.noUnresolvedPreparations()}
{interactionsText.noUnresolvedPreparations({
loanPreparationsLabel: String(getField(tables.Loan, 'loanPreparations').label).toLowerCase()
})}
</Dialog>
) : (
<PreparationReturn preparations={preparations} onClose={handleClose} />
Expand Down Expand Up @@ -161,7 +163,10 @@ function PreparationReturn({
<Button.DialogClose>{commonText.cancel()}</Button.DialogClose>
<Button.Info
disabled={!canSelectAll}
title={interactionsText.returnAllPreparations()}
title=
{interactionsText.returnAllPreparations({
preparationTable: String(tables.Preparation.label).toLowerCase(),
})}
onClick={(): void =>
setState(
state.map(({ unresolved, remarks }) => ({
Expand Down Expand Up @@ -192,7 +197,10 @@ function PreparationReturn({
</Button.Info>
<Submit.Success
form={id('form')}
title={interactionsText.returnSelectedPreparations()}
title=
{interactionsText.returnSelectedPreparations({
preparationTable: String(tables.Preparation.label).toLowerCase(),
})}
>
{commonText.apply()}
</Submit.Success>
Expand Down
10 changes: 8 additions & 2 deletions specifyweb/frontend/js_src/lib/components/Interactions/PrepDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,10 @@ export function PrepDialog({
<Button.DialogClose>{commonText.cancel()}</Button.DialogClose>
<Button.Info
disabled={!canSelectAll}
title={interactionsText.selectAllAvailablePreparations()}
title=
{interactionsText.selectAllAvailablePreparations({
preparationTable: String(tables.Preparation.label).toLowerCase(),
})}
onClick={(): void =>
setSelected(preparations.map(({ available }) => available))
}
Expand Down Expand Up @@ -133,7 +136,10 @@ export function PrepDialog({
</>
)
}
header={interactionsText.preparations()}
header=
{interactionsText.preparations({
preparationTable: tables.Preparation.label,
})}
Comment on lines +140 to +142
Copy link
Member

Choose a reason for hiding this comment

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

interesting that prettier did not reformat this file in 0371f02

part of #6051 I guess

onClose={handleClose}
>
<Label.Inline className="gap-2">
Expand Down
5 changes: 4 additions & 1 deletion specifyweb/frontend/js_src/lib/components/QueryBuilder/LoanReturn.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,10 @@ export function QueryLoanReturn({
<Button.DialogClose>{commonText.cancel()}</Button.DialogClose>
<Submit.Success
form={id('form')}
title={interactionsText.returnSelectedPreparations()}
title=
{interactionsText.returnSelectedPreparations({
preparationTable: String(tables.Preparation.label).toLowerCase(),
})}
>
{interactionsText.return()}
</Submit.Success>
Expand Down
Loading
Loading