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

Recordedit with association picker when adding rows using prefill #2490

Merged
merged 55 commits into from
Oct 3, 2024

Conversation

jrchudy
Copy link
Member

@jrchudy jrchudy commented Jul 16, 2024

This pull request will change how add records on record app works for adding rows to certain related tables (outlined in issue #2416 ). This is ready to review so we can discuss which functions to add to ermrestJS to make support for this simpler in chaise.

The changes to support this new workflow are as follows:

  • recordedit component changed to check if there is a prefill and if we have the leaf column for the association visible in create mode of recordedit before showing the association modal on load
    • on submission of association modal, the first form is updated with the row to associate and more forms are added if there were multiple selections
    • "add more" button added to reopen the association modal to selected more rows to associate and add more forms in recordedit on submission
  • recordedit provider keeps track of the prefill object from cookie storage and the selected rows when the association is unique
  • attach foreignKeyCallbacks when generating the foreign key fields if the column for the foreign key field is the one used for the leaf column in the association
  • foreignkey-field changed to update selected rows used for disabling when each input is changed or cleared
  • removeForm updated to remove the selected row(s) associated with the removed form

Other changes:

  • foreignkey-dropdown-field also works with disabling rows and updating which rows are selected
    • if the selected rows in the form are changed in any other dropdown or more rows are added using "add more", the foreign key dropdowns will have their disabled rows updated
  • hide multi set button for the leaf column foreign key field
  • new RecordsetDisplayMode added for customizing bulk foreign key picker title and "continue" button
  • disabled tuples promise moved to recordedit-utils so the generated promise function can be reused
  • chaise-input-group-text-sm rule was not using btn-padding-sm-* variables
  • addTooManyFormsAlert added for when trying to pick more than 200 foreign key values to add as recordedit forms

Testing:

Since this applies to both foreign key picker popups and dropdowns, this should be tested for both cases. The ermrestJS branch association-display should be checked and installed alongside this branch.

To test the modal picker, navigate to this Protocol and click "Add Records" for the "Protocol Author" related table dev.atlas-d2k is unavailable for testing, see comment below for testing modal picker.

To test the foreignkey dropdown, this catalog is using the foreign key dropdown for the recordedit inputs. If you can't test with this one, this was created using createAssociationEdit script in the testScripts folder of ErmrestDataUtils. This schema could also be used to test the modal popup too if the table-display annotation on the table named "leaf" is modified.

@jrchudy jrchudy self-assigned this Jul 16, 2024
@jrchudy jrchudy linked an issue Jul 16, 2024 that may be closed by this pull request
@jrchudy jrchudy requested a review from RFSH July 19, 2024 22:02
src/providers/recordedit.tsx Outdated Show resolved Hide resolved
src/providers/recordset.tsx Outdated Show resolved Hide resolved
@jrchudy jrchudy requested a review from RFSH September 19, 2024 23:17
@jrchudy
Copy link
Member Author

jrchudy commented Sep 19, 2024

@RFSH This is ready for review now with the changes to ermrestJS that are ready for review as well. There are few things that we should discuss together about this PR:

  • How should numberForms be made available in recordset provider when it is used as a modal in recordedit app?
    • I'll update this comment later after I think about this a bit more
    • most likely needs to be a prop for recordset modal, like another variable attached to foreign key callbacks similar to bulkForeignKeySelectedRows
  • should prefill object be kept as a variable in recordedit provider or moved to the BulkCreateForeignKeyObject calculated in ermrestJS?
  • I noticed 2 things in the test code that we should think about:
    • our recordedit selectors inconsistently use 0 or 1 index
      • getForeignKeyInputButton - uses 1 indexing
      • getDeleteRowButton - uses 0 indexing
      • we should do one or the other for all selectors in recordedit app
    • RecordsetRowValue is a type that is an array of “column” values
      • should the “type” be called RecordsetRowValues (plural) since it’s intended to be an array of individual column values?
      • this is often used as a type for arrays so we can have an array of arrays

// user closes the modal without making any selections
const closeBulkForeignKeyCB = () => {
// if the page was loaded with a modal showing and it is dismissed, update app state variable and do nothing else
if (selectionsFillFirstForm) setSelectionsFillFirstForm(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

@RFSH pointed out that there was a bug when:

  1. the recordedit app was loaded
  2. the user closed the first modal
  3. then used "Add more" to add 3 values, resulting in 4 forms displayed
  4. remove form 2
  5. open "Add more" again and the wrong row was still disabled

Copy link
Member Author

@jrchudy jrchudy Sep 24, 2024

Choose a reason for hiding this comment

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

The issue was that when the first modal is shown and closed, the first form is initialized but the code didn't initialize bulkForeignKeySelectedRows to have a null value for the first "selection". The following change to this condition will initialize that "array" with a null value as the first element representing a null value in the first form.

if (selectionsFillFirstForm) {
  setSelectionsFillFirstForm(false);
  setBulkForeignKeySelectedRows([null]);
}

*
* This means there are 2 foreign keys that are part of the same key (making the pair unique) and there are other columns that are not foreign keys
*/
test.describe('for a table that is almost pure and binary and the foreign keys are a unique key', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

From the comment above, there was a bug regarding selecting leaf values and removing forms that was fixed.

I need to update the test cases to ALSO check for the values that are disabled in the modals/dropdowns and not just the “# of disabled rows”.

I will also add a test case for the example where the initial modal is “closed” and make sure the first form remains “unfilled” for the “leaf value” since no selections were made.

src/models/recordset.ts Outdated Show resolved Hide resolved
src/models/recordset.ts Outdated Show resolved Hide resolved
src/utils/message-map.ts Outdated Show resolved Hide resolved
src/providers/alerts.tsx Show resolved Hide resolved
src/providers/recordedit.tsx Outdated Show resolved Hide resolved
src/components/recordedit/recordedit.tsx Outdated Show resolved Hide resolved
src/components/recordedit/recordedit.tsx Outdated Show resolved Hide resolved
};
}

const modalReference = domainRef.addFacets(andFilters, customFacets)
Copy link
Member

Choose a reason for hiding this comment

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

We should come up with a new context for this in ermrestjs.

Copy link
Member Author

Choose a reason for hiding this comment

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

New context added to ermrestJS called compactSelectBulkForeignKey. See ermrestJS PR for comments about those changes.

src/components/recordedit/recordedit.tsx Show resolved Hide resolved
src/components/recordedit/recordedit.tsx Outdated Show resolved Hide resolved
@jrchudy jrchudy requested a review from RFSH September 30, 2024 20:46
Copy link
Member

@RFSH RFSH left a comment

Choose a reason for hiding this comment

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

Overall everything looks and works as expected. My comments are mainly about code style or leftover code.

I left a comment about the "LogStackPath" that you introduced for this new popup and how we should change it to be more accurate. Please also update logging.md to include the new stack-path.

src/components/recordedit/form-container.tsx Outdated Show resolved Hide resolved
src/models/log.ts Outdated Show resolved Hide resolved
src/components/recordedit/recordedit.tsx Outdated Show resolved Hide resolved
src/components/recordedit/recordedit.tsx Outdated Show resolved Hide resolved
src/components/recordedit/recordedit.tsx Outdated Show resolved Hide resolved
src/providers/recordedit.tsx Outdated Show resolved Hide resolved
src/providers/recordedit.tsx Outdated Show resolved Hide resolved
@jrchudy jrchudy merged commit f4f08c1 into master Oct 3, 2024
2 checks passed
@jrchudy jrchudy deleted the recordedit-w-association-picker branch October 3, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve multi-create workflow for almost pure and binary relationships
2 participants