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

Inventory #4

Closed
wants to merge 10 commits into from
Closed

Inventory #4

wants to merge 10 commits into from

Conversation

gcrt0701
Copy link

@gcrt0701 gcrt0701 commented Jan 3, 2021

Fixes HospitalRun#2486.

Changes proposed in this pull request:

  • Created Inventory Model
  • Can now view a table of different inventory items
  • Can add inventory items
  • Can view, edit, and delete individual inventory items

Note: pull requests without proper descriptions may simply be closed without further discussion. We appreciate your contributions, but need to know what you are offering in clearly described format. Provide tests for all code that you add/modify. If you add/modify any components update the storybook. Thanks! (you can delete this text)

@@ -63,6 +63,11 @@ export const schema = [
plural: 'medications',
relations: { patient: { belongsTo: 'patient' } },
},
{
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if this is necessary given that we don't know what pouchdb.ts will look like with the new migration.

Copy link
Author

Choose a reason for hiding this comment

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

Deleting it prevented the search function from working in InventoryRepository. I'm guessing it's because the file imports relationalDb from puchdb.ts which requires the use of the schema. I'm not sure what other way to do it since InventoryRepository requires some sort of database either way. The rest of the repositories seem to do it this way so I figure it's safe to do so as well.

rank: '',
type: '',
crossReference: '',
reorderPoint: ('' as unknown) as number,
Copy link
Member

Choose a reason for hiding this comment

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

This casting feels strange to me. Why not start with a number?

Copy link
Author

Choose a reason for hiding this comment

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

Since the value is set through a text input, it's a string. However, strings and numbers don't appear to overlap enough to simply cast from one to the other. Turns out you have to make it unknown first. I would try to change the input to a numerical one but I didn't see any templates on the inputs folder so I decided to leave it as is.

Copy link
Member

@DrewGregory DrewGregory left a comment

Choose a reason for hiding this comment

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

Great work @gcrt0701 ! I'm quite impressed -- this is a hefty PR that uses a ton of cool techniques I intend to use in my future React development.

I would also appreciate if you could resolve the merge conflicts with src/shared/components/navbar/pageMap.tsx. If you would like help with that, let me know.

Copy link
Member

@DrewGregory DrewGregory 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!

Copy link

@cpondoc cpondoc left a comment

Choose a reason for hiding this comment

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

This looks great, @gcrt0701! Great work on all of this!

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