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

Prevent from carrying preps in interaction during cloning #4905

Draft
wants to merge 2 commits into
base: production
Choose a base branch
from

Conversation

CarolineDenis
Copy link
Contributor

Fixes #4012

Checklist

  • Self-review the PR after opening it to make sure the changes look good
    and self-explanatory (or properly documented)
  • Add relevant issue to release milestone

Testing instructions

  • Create a loan with the maximum number of preps an object has
  • Clone the loan
  • verify that the preparations were not carried over to the new 'cloned' loan
  • verify the same for other interactions

Comment on lines +228 to +244
const interactionTablesWithPreps = [
'Disposal',
'Loan',
'Gift',
'ExchangeIn',
'ExchangeOut',
'Borrow',
] as const;
type InteractionTable = typeof interactionTablesWithPreps[number];
const interactionTablesPrepsFieldName: RR<InteractionTable, string> = {
Disposal: 'disposalPreparations',
Loan: 'loanPreparations',
Gift: 'giftPreparations',
ExchangeOut: 'exchangeOutPreps',
ExchangeIn: 'exchangeInPreps',
Borrow: 'borrowPreparations',
};
Copy link
Member

Choose a reason for hiding this comment

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

do you need to hardcode this list?
imagine someone adding a new table - how would they know that this place in the code might need to be modified before the table is added or else a bug is introduced???
that is why you should avoid hardcoding things and creating edge cases where possible - instead compute things on the fly

could you please dynamically construct it on the fly (i.e if field name is a relationship to the Preparations table, then don't clone it)


the only exception to computing things on the fly is when doing so is computationally expensive - but in those cases, you should add a test case that will do the computation to make sure the hardcoded list is not out of date with the dynamically computed list
(i.e front-end stores a hardcoed list of all the permission policies that sp7 back-end defines so as not to have to fetch those from the back-end on each app load - yet, when in development, it will make sure that the hardcoded list stored on the front-end matches exactly what back-end provides)

Copy link
Contributor

Choose a reason for hiding this comment

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

Take a look at the changes made in #4805: there is code to get the main Interaction tables which have preparations:

export type InteractionWithPreps = Tables[ExtractInteraction<
AnyInteractionPreparation['tableName']
>];
export const interactionsWithPrepTables: RestrictedTuple<
InteractionWithPreps['tableName']
> = ['Disposal', 'ExchangeIn', 'ExchangeOut', 'Gift', 'Loan'];

This code relies on a dynamically generated type which already exists on the frontend called AnyInteractionPreparation:

export type AnyInteractionPreparation = Extract<
ValueOf<Tables>,
{
readonly fields: {
readonly quantity: number | null;
};
} & {
readonly toOneIndependent: {
readonly preparation: Preparation | null;
};
}
>;


#4805 also has examples of using/getting the relationship from Interaction -> InteractionPreparation:

const preparationRelationship = defined(
interaction.specifyTable.relationships.find((relationship) =>
interactionPrepTables.includes(
(relationship.relatedTable as SpecifyTable<AnyInteractionPreparation>)
.name
)
)
);

const uniqueFields = getUniqueFields(table);
if (
interactionTablesWithPreps.includes(table.name as InteractionTable) &&
cloneAll
Copy link
Member

Choose a reason for hiding this comment

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

are you sure this logic should apply only in the cloneAll cases?
it seems like it should apply in !cloneAll case too...

@CarolineDenis
Copy link
Contributor Author

NOTES:
Continue work here once #4805 is merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Dev Attention Needed
Development

Successfully merging this pull request may close these issues.

Cloning a Loan carries the Loan Preparations along
3 participants