Improve Preparation form links to Interactions #6110

grantfitzsimmons · 2025-01-17T16:01:09Z

Fixes #6112

Checklist

Self-review the PR after opening it to make sure the changes look good and
self-explanatory (or properly documented)
Add relevant documentation (Tester - Dev)

Before

You could not see any linked disposals or exchanges. Interactions that were empty still displayed with no results.

After

Interactions are only shown when they are linked to a Preparation
- If none are linked, it shows "There are no interactions linked to this preparation"
When clicking on the "Show Interactions" button, the user should see all associated
- Resolved Loans
- Unresolved Loans
- Gifts
- Disposals
- Exchange Out
- Exchange In

Screen.Recording.2025-01-17.at.11.06.02.AM.mov

Testing instructions

You will need to make sure you have the "Show Interactions" / "Show Loans" button on the Preparation form to test this properly. If you do not already, here is the XML to add:

 <cell type="command" id="ShowLoansBtn" name="ShowLoansBtn" label="SHOW_INTERACTIONS" ignore="true"/>

Test this by using a combination of various interactions and preparations. Involve the preparation in multiple interactions of the same type. Verify that existing preparations linked to interactions appear as expected. Compare the behavior to production and let me know if this is undesirable.

OK I did it

If anyone is interested in updating the documentation for this, I would appreciate it. Thank you!

Triggered by 81067b5 on branch refs/heads/issue-6108

Triggered by 59802e1 on branch refs/heads/issue-6108

specifyweb/frontend/js_src/lib/components/FormCommands/index.tsx

maxpatiiuk · 2025-01-20T03:20:09Z

specifyweb/frontend/js_src/lib/components/Interactions/PrepDialog.tsx

+          {interactionsText.preparations({
+            preparationTable: tables.Preparation.label,
+        })}


interesting that prettier did not reformat this file in 0371f02

part of #6051 I guess

Alternative implementation of #6110 for #6108

melton-jason · 2025-01-20T06:10:08Z

specifyweb/frontend/js_src/lib/localization/interactions.ts

+  disposals: {
+    comment: 'Example: Disposal records',
+    'en-us': '{disposalTable:string} records',
+    'es-es': '{disposalTable:string} registros',
+    'fr-fr': '{disposalTable:string} enregistrements',
+    'ru-ru': '{disposalTable:string} записи',
+    'uk-ua': '{disposalTable:string} записи',
+    'de-ch': '{disposalTable:string} Datensätze',
+  },


Looks like there's multiple ${InteractionTable} records localization strings. If these are used in the same context, can they merged into a single localization string?

For example:

Suggested change

disposals: {

comment: 'Example: Disposal records',

'en-us': '{disposalTable:string} records',

'es-es': '{disposalTable:string} registros',

'fr-fr': '{disposalTable:string} enregistrements',

'ru-ru': '{disposalTable:string} записи',

'uk-ua': '{disposalTable:string} записи',

'de-ch': '{disposalTable:string} Datensätze',

},

interactionRecords: {

comment: 'Example: Loan records',

'en-us': '{interactionTableLabel:string} records',

},

Which can be used like:

// Loan records interactionsText.interactionRecords({ interactionTableLabel: tables.Loan.label, }); // Preparation records interactionsText.interactionRecords({ interactionTableLabel: tables.Preparation.label, }); // ...

melton-jason · 2025-01-20T06:15:08Z

specifyweb/frontend/js_src/lib/localization/interactions.ts

@@ -17,6 +17,11 @@ export const interactionsText = createDictionary({
    'uk-ua': 'Взаємодії',
    'de-ch': 'Interaktionen',
  },
+  noInteractions: {
+    comment: 'Example: There are no interactions linked to this {preparation}',


Should explicit examples include a placeholder like this? Personally, I think examples should be one or more values the placeholder can take: there's hopefully less confusion for the localizer this way.

If you need to provide additional context for how this might be used or appear in the application, you can do so outside of the example; like in the following example:

specify7/specifyweb/frontend/js_src/lib/localization/common.ts

Lines 639 to 642 in 36f7a7e

comment: `

Example usage: "Choose collection:". Used only if there is nothing else on

this line after the colon heading

`,

melton-jason · 2025-01-20T06:31:36Z

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

+        </>
+      ) : (
+        <>
+          {Array.isArray(data.openLoans) && data.openLoans.length > 0 && (


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!

melton-jason · 2025-01-20T09:37:30Z

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

+          exchangeIns: hasTablePermission('ExchangeInPrep', 'read')
+            ? fetchCollection('ExchangeInPrep', {
+                limit: DEFAULT_FETCH_LIMIT,
+                preparation: preparation.get('id'),
+                domainFilter: false,
+              }).then(({ records }) => records.map(deserializeResource))
+            : undefined,


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.

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

Co-authored-by: Max Patiiuk <[email protected]>

grantfitzsimmons added 2 commits January 17, 2025 10:00

Update Interaction localization

d76789b

Add Disposal to Interactions dialog

daa990c

grantfitzsimmons added this to the Grant's issue list milestone Jan 17, 2025

grantfitzsimmons linked an issue Jan 17, 2025 that may be closed by this pull request

Extend "Show Interactions" dialog to Disposal, Exchange In, and Exchange Out #6112

Open

grantfitzsimmons added 2 commits January 17, 2025 11:06

Update interactions.ts

4800238

Hide interactions when none are available

eaf9a6c

grantfitzsimmons requested a review from maxpatiiuk January 17, 2025 17:11

Remove unused text

0c27b37

grantfitzsimmons marked this pull request as ready for review January 17, 2025 17:21

grantfitzsimmons added 4 commits January 18, 2025 17:37

Use localized Preparation table name

81067b5

Lint code with ESLint and Prettier

416dfb2

Triggered by 81067b5 on branch refs/heads/issue-6108

Improve localization for Preparations

702e118

Resolve conflicts

33caf9b

grantfitzsimmons marked this pull request as draft January 19, 2025 00:18

grantfitzsimmons added 7 commits January 18, 2025 18:20

Update interactions.ts

aa4dfda

Fix no interactions label

268110e

Improve Preparation localization further

634aef1

Finish localizing preparations

b7f1896

Merge branch 'production' into issue-6108

bd0efb5

Add tables import

59802e1

Lint code with ESLint and Prettier

0371f02

Triggered by 59802e1 on branch refs/heads/issue-6108

grantfitzsimmons marked this pull request as ready for review January 19, 2025 21:24

grantfitzsimmons modified the milestones: Grant's issue list, 7.9.11.1 Jan 19, 2025

melton-jason self-requested a review January 19, 2025 21:31

maxpatiiuk approved these changes Jan 20, 2025

View reviewed changes

melton-jason added a commit that referenced this pull request Jan 20, 2025

Show all related interactions in ShowLoans dialog

6c6d365

Alternative implementation of #6110 for #6108

melton-jason approved these changes Jan 20, 2025

View reviewed changes

Update specifyweb/frontend/js_src/lib/components/FormCommands/index.tsx

34b0e89

Co-authored-by: Max Patiiuk <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Preparation form links to Interactions #6110

Improve Preparation form links to Interactions #6110

grantfitzsimmons commented Jan 17, 2025 •

edited

Loading

maxpatiiuk Jan 20, 2025

melton-jason Jan 20, 2025

melton-jason Jan 20, 2025

melton-jason Jan 20, 2025

melton-jason Jan 20, 2025 •

edited

Loading

melton-jason Jan 20, 2025

	comment: `
	Example usage: "Choose collection:". Used only if there is nothing else on
	this line after the colon heading
	`,

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

	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));

Improve Preparation form links to Interactions #6110

Are you sure you want to change the base?

Improve Preparation form links to Interactions #6110

Conversation

grantfitzsimmons commented Jan 17, 2025 • edited Loading

Checklist

Before

After

Testing instructions

maxpatiiuk Jan 20, 2025

Choose a reason for hiding this comment

melton-jason Jan 20, 2025

Choose a reason for hiding this comment

melton-jason Jan 20, 2025

Choose a reason for hiding this comment

melton-jason Jan 20, 2025

Choose a reason for hiding this comment

melton-jason Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

melton-jason Jan 20, 2025

Choose a reason for hiding this comment

grantfitzsimmons commented Jan 17, 2025 •

edited

Loading

melton-jason Jan 20, 2025 •

edited

Loading