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

Improving prime Datatable #862

Merged
merged 19 commits into from
Sep 24, 2024
Merged

Improving prime Datatable #862

merged 19 commits into from
Sep 24, 2024

Conversation

BenjaminCharmes
Copy link
Contributor

@BenjaminCharmes BenjaminCharmes commented Aug 30, 2024

PrimeDataTable (Dynamic DataTable) becomes the default version for Samples, Collections, Inventory and Equipment.

Keep the old SampleTable and EquipmentTable components so that the cypress e2e test still works on the /old-sample and /old-equipment routes long enough to write the new tests on the new table.

And lots of cleanup

Closes #877 & #878

Copy link

codecov bot commented Aug 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.22%. Comparing base (94e1f46) to head (ab1b98e).
Report is 20 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #862   +/-   ##
=======================================
  Coverage   68.22%   68.22%           
=======================================
  Files          62       62           
  Lines        3909     3909           
=======================================
  Hits         2667     2667           
  Misses       1242     1242           
Files with missing lines Coverage Δ
pydatalab/pydatalab/routes/v0_1/collections.py 69.53% <ø> (ø)
pydatalab/pydatalab/routes/v0_1/items.py 82.65% <100.00%> (ø)

Copy link

cypress bot commented Aug 30, 2024

datalab    Run #2417

Run Properties:  status check passed Passed #2417  •  git commit 4f2ae35f2e ℹ️: Merge ab1b98e1d33e5224b9073f611bed2446efb79ac8 into 94e1f465839508303668d72e996a...
Project datalab
Branch Review bc/next-improvement
Run status status check passed Passed #2417
Run duration 05m 48s
Commit git commit 4f2ae35f2e ℹ️: Merge ab1b98e1d33e5224b9073f611bed2446efb79ac8 into 94e1f465839508303668d72e996a...
Committer Benjamin Charmes
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 131
View all changes introduced in this branch ↗︎

@BenjaminCharmes BenjaminCharmes force-pushed the bc/next-improvement branch 2 times, most recently from fd81b56 to 9e3c180 Compare September 13, 2024 11:06
@BenjaminCharmes BenjaminCharmes linked an issue Sep 13, 2024 that may be closed by this pull request
…ry and Equipment

Fix global-filter-fields for creators and collections

Add dynamic prime table for starting materials + some cleaning

Add dynamic prime table for equipments

New ButtonPrimeTable component

Fix deleteCollection

Fix button still clickable after deleting a sample

Add API tests for collections/<collection_id> POST route
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

Looking really good @BenjaminCharmes! Great to see how much old code we can get rid of without breaking anything!

Minor initial comments: I think we should retain the old component names just for consistency (of the git history more than anything). Plus, the name equipment should be kept throughout to match the underlying model.

Sadly I'm still having issues with my local set up where the components within the table are not being used -- I'm still using node 18 locally, so perhaps this is just an a "me problem" but I'd like to at least find the reason before we merge this. Perhaps the minimum node version needs to be updated in package.json too.

I'll give this a more thorough review tomorrow and we can discuss on Friday.

webapp/cypress/component/NavbarTest.cy.jsx Outdated Show resolved Hide resolved
webapp/src/components/AddToCollectionModal.vue Outdated Show resolved Hide resolved
webapp/src/components/CollectionsTable.vue Outdated Show resolved Hide resolved
webapp/src/components/DynamicButtonDataTable.vue Outdated Show resolved Hide resolved
Copy link
Member

@jdbocarsly jdbocarsly left a comment

Choose a reason for hiding this comment

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

A few notes from today's chat:

  • The "select all" button is slow with lots of samples/inventory. Can we change the select button to only select the shown samples as a workaround?
  • Each individual checkbox has to be clicked precisely to be clicked. Can we change the @ click even to be on the cell, rather than on the checkbox
  • Tests for checking whether components are rendered in the table
  • Test for adding to inventory when editable_inventory is turned on etc
  • Check whether add to collection select on create new item modal still works
  • Consider future refactor around how components are specified in the table, i.e., whether they can be mapped from the datalab server schema
  1. Is there a way to make the filter symbol filled in when a filter is on?
  2. "Add a collection" from the CollectionSelect is great, but the spinner seems to be broken as it never goes away even once the collection is created
  3. When you insert something into a collection, and then search for the same collection again, it gives you the option to create the collection again, and then causes an error.
  4. seems to be possible to "double add" something to a collection that was already in that collection, which causes an error when you try to access the page "relationships and collection mismatch"

@ml-evs ml-evs self-requested a review September 24, 2024 13:31
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

🪨 🚀

Copy link
Member

@jdbocarsly jdbocarsly left a comment

Choose a reason for hiding this comment

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

looks great!

@BenjaminCharmes BenjaminCharmes merged commit f7c5550 into main Sep 24, 2024
20 checks passed
@BenjaminCharmes BenjaminCharmes deleted the bc/next-improvement branch September 24, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants