StakeRequest model (with a help of sequelize) #73

pgev · 2019-06-19T12:10:59Z

Implements stake request model classes.

StakeRequestAttributes: Represents an interface for an input to create StakeRequest objects in StakeRequestRepository.

StakeRequest: Represents a stake request object to be used throughout facilitator.

StakeRequestModel: Represents a StakeRequest database model. Allows to initialize stake request database table's schema and in addition represents a raw in that table.

StakeRequestRepository: Allows to create and retrieve stake request object. A class should be an interface for other classes in facilitator to interact with stake request model.

Database: Creates, initializes and gives an access to different models throughout facilitator.

Fixes: #53

See: OpenST#53

- Enables syncing all database tables once. - Adds common configuration settings for all tables. - Defines stake request number fields as unsigned integer. - Adds deferred sync functionality for table creation and table operations.

deepesh-kn

This looks very clean 👍

Questions:

Why do we need StakeRequestAttributes? Is it that any class that implement the StakeRequestAttributes interface can be passed in the build and create params?
What StakeRequestRepository.build does?
What do you think if we do the following?
- StakeRequestRepository can extends Model
- StakeRequestModel can be a simple class that has all the properties (may be some functions to implement business logic). Implements/confirms to the StakeRequestAttributes interface.
- StakeRequestModel can be passed as params in StakeRequestRepository.build and StakeRequestRepository.create
- StakeRequestModel is returned from StakeRequestRepository.get function.

In this way there is clear separation of Model objects(related to facilitator) and Repository (sequelize model objects)

- Hides sequelize from Database public interface. - Updates table and model names for StakeRequestModel. - Removes StakeRequestRepository::build interface for now. - Improves StakeRequestRepository::create unit test.

pgev · 2019-06-20T07:04:34Z

This looks very clean 👍

Questions:

Why do we need StakeRequestAttributes? Is it that any class that implement the StakeRequestAttributes interface can be passed in the build and create params?

Yes. StakeRequestAttributers is an interface for an input to create StakeRequest object.

What StakeRequestRepository.build does?

build pattern in sequelize creates an object, which is not yet saved in database model and can be separately (in some other time) saved into database. However, I've decided to remove that for now. We can add it later if needed.

What do you think if we do the following?

StakeRequestRepository can extends Model

StakeRequestModel can be a simple class that has all the properties (may be some functions to implement business logic). Implements/confirms to the StakeRequestAttributes interface.

StakeRequestModel can be passed as params in StakeRequestRepository.build and StakeRequestRepository.create

StakeRequestModel is returned from StakeRequestRepository.get function.

In this way there is clear separation of Model objects(related to facilitator) and Repository (sequelize model objects)

In the current design, StakeRequestModel is a sequelize model, which in my understanding represents two things: a database schema model, its init function defines database table schema and database table raw (all find* functions returns Model object).

Currently, for abstracting away from underlying database representation there are StakeRequestAttributes, StakeRequest and StakeRequestRepository classes. I've made some changes after your comment, please, have a look and we could discuss this point one more time. Thx!

abhayks1 · 2019-06-20T09:48:01Z

Started reviewing 🎸 🚗

abhayks1

Really nice work with sequelize integration 🚀 💯
Liked the classes segregation.

Added few suggestions:

Database class can create and return singleton sequelize instance. It makes sure there are not too many sequelize db instances.
Migrations can be used for creating tables.
I feel 🤔 we shouldn't import/load all repositories at once for memory footprint and maintenance. As respositries increase this will not be maintainable . Currently Database class loads all the repositories. We can import/load only those repositories which are needed in a service/process.

package.json

src/models/Database.ts

src/models/StakeRequestRepository.ts

Removes an empty StakeRequestRepository::constructor tests file. Updates unit tests accordingly.

abhayks1 · 2019-06-21T06:17:51Z

Reviewing 🚋

tests/models/StakeRequestRepository/create.test.ts

tests/models/StakeRequestRepository/get.test.ts

src/models/Database.ts

deepesh-kn

LGTM 💯 👍

I have added a few comments.

tests/models/StakeRequestRepository/util.ts

src/models/StakeRequestRepository.ts

abhayks1

Overall looks good to me 🥇
Added few minor comments on tests.

- Improves documentation for StakeRequestRepository. - Enhances error message for StakeRequestRepository::create.

src/models/Database.ts

src/models/StakeRequestRepository.ts

deepesh-kn

LGTM

Changes are implemented.

Paruyr Gevorgyan added 19 commits June 14, 2019 10:43

Implements draft version of stake request model

19cb5e1

See: OpenST#53

Renamed DataAccessObject to DatabaseWrapper for more clarity

a6da945

Adds empty tests files for DatabaseWrapper and StakeRequestRepository

ed650b3

Adds npm scripts to run tests

d4b2c67

Removes DatabaseWrapper empty tests file

1b74714

Adds tests for StakeRequestRepository::createTable

009a2f4

Adds tests for StakeRequestRepository::create

6c1576c

Adds tests for StakeRequestRepository::get

996f16c

Adds .travis.yml and enhances package.json

ae584ea

Adds documentation to DatabaseWrapper

d86c945

Documents StakeRequestRepository

d05e635

Merge branch 'local/origin/master' into local/origin/stake-request-model

83b1ec8

Merges with master branch

d9a10a7

Changes ts target from es6 to es5

7131506

Merges with upstream/master branch

2f4ae9a

Removes the test coverage step (temporarily) from Travis

a99df19

Drafts stake request model with sequelize

f610a86

Adds a database configuration and deferred sync

b5425c4

- Enables syncing all database tables once. - Adds common configuration settings for all tables. - Defines stake request number fields as unsigned integer. - Adds deferred sync functionality for table creation and table operations.

Adds empty tests file for StakeRequestRepository

a8e5dde

deepesh-kn reviewed Jun 19, 2019

View reviewed changes

Paruyr Gevorgyan added 6 commits June 19, 2019 16:06

Adds unit tests for StakeRequestRepository::create

b0d4918

Adds unit tests for StakeRequestRepository::build

104ce73

Removes StakeRequestRepository::sync empty test

0680d3f

Adds unit test for StakeRequestRepository::get

3043888

Hides StakeRequestModel behind implementation

af4e5ea

- Hides sequelize from Database public interface. - Updates table and model names for StakeRequestModel. - Removes StakeRequestRepository::build interface for now. - Improves StakeRequestRepository::create unit test.

Syncs all models on a database creation

c040911

pgev changed the title ~~Draft implementation of stake request model with sequelize~~ Stake request model classes Jun 20, 2019

pgev changed the title ~~Stake request model classes~~ StakeRequest model (with a help of sequelize) Jun 20, 2019

abhayks1 self-assigned this Jun 20, 2019

abhayks1 suggested changes Jun 20, 2019

View reviewed changes

A single interface to create a database object.

85fc160

Removes an empty StakeRequestRepository::constructor tests file. Updates unit tests accordingly.

abhayks1 removed their assignment Jun 20, 2019

Adds sequelize and sqlite3 to dependencies.

78fa3ca

abhayks1 self-assigned this Jun 21, 2019

abhayks1 reviewed Jun 21, 2019

View reviewed changes

tests/models/StakeRequestRepository/create.test.ts Outdated Show resolved Hide resolved

tests/models/StakeRequestRepository/get.test.ts Outdated Show resolved Hide resolved

src/models/Database.ts Show resolved Hide resolved

src/models/Database.ts Outdated Show resolved Hide resolved

deepesh-kn reviewed Jun 21, 2019

View reviewed changes

tests/models/StakeRequestRepository/util.ts Show resolved Hide resolved

src/models/StakeRequestRepository.ts Show resolved Hide resolved

src/models/StakeRequestRepository.ts Show resolved Hide resolved

abhayks1 previously requested changes Jun 21, 2019

View reviewed changes

abhayks1 removed their assignment Jun 21, 2019

Paruyr Gevorgyan added 2 commits June 21, 2019 12:09

Improves StakeRequestRepository

dea7b9f

- Improves documentation for StakeRequestRepository. - Enhances error message for StakeRequestRepository::create.

StakeRequest extends StakeRequestAttributes

e044e73

pgev requested review from abhayks1 and deepesh-kn June 21, 2019 10:55

Adds StakeRequestRepository::updateMessageHash

a292167

deepesh-kn reviewed Jun 21, 2019

View reviewed changes

src/models/Database.ts Outdated Show resolved Hide resolved

deepesh-kn reviewed Jun 21, 2019

View reviewed changes

src/models/StakeRequestRepository.ts Show resolved Hide resolved

Paruyr Gevorgyan added 2 commits June 21, 2019 15:47

Enhances documentation

53a3c56

Enhances documentation

ebf3f5b

pgev requested a review from deepesh-kn June 21, 2019 13:54

deepesh-kn approved these changes Jun 21, 2019

View reviewed changes

deepesh-kn merged commit 22f646a into OpenST:master Jun 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StakeRequest model (with a help of sequelize) #73

StakeRequest model (with a help of sequelize) #73

pgev commented Jun 19, 2019 •

edited

Loading

deepesh-kn left a comment

pgev commented Jun 20, 2019

abhayks1 commented Jun 20, 2019

abhayks1 left a comment •

edited

Loading

abhayks1 commented Jun 21, 2019 •

edited

Loading

deepesh-kn left a comment

abhayks1 left a comment

deepesh-kn left a comment

StakeRequest model (with a help of sequelize) #73

StakeRequest model (with a help of sequelize) #73

Conversation

pgev commented Jun 19, 2019 • edited Loading

deepesh-kn left a comment

Choose a reason for hiding this comment

pgev commented Jun 20, 2019

abhayks1 commented Jun 20, 2019

abhayks1 left a comment • edited Loading

Choose a reason for hiding this comment

abhayks1 commented Jun 21, 2019 • edited Loading

deepesh-kn left a comment

Choose a reason for hiding this comment

abhayks1 left a comment

Choose a reason for hiding this comment

deepesh-kn left a comment

Choose a reason for hiding this comment

pgev commented Jun 19, 2019 •

edited

Loading

abhayks1 left a comment •

edited

Loading

abhayks1 commented Jun 21, 2019 •

edited

Loading