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

feat: abstract DB Adapter #112

Open
wants to merge 40 commits into
base: master
Choose a base branch
from
Open

feat: abstract DB Adapter #112

wants to merge 40 commits into from

Conversation

AVVS
Copy link
Member

@AVVS AVVS commented Jun 3, 2016

No description provided.

@codecov-io
Copy link

codecov-io commented Jun 3, 2016

Current coverage is 88.45% (diff: 90.21%)

Merging #112 into master will decrease coverage by 0.65%

@@             master       #112   diff @@
==========================================
  Files            36         38     +2   
  Lines           744        823    +79   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            663        728    +65   
- Misses           81         95    +14   
  Partials          0          0          

Powered by Codecov. Last update 6b0a659...98b5770

@AVVS
Copy link
Member Author

AVVS commented Jun 20, 2016

/home/runner/ms-users/src/actions/register.js
  12:23  error  Unable to resolve path to module '../db/adapter'  import/no-unresolved

/home/runner/ms-users/src/model/storages/redisstorage.js
  42:25  error  'captchaConfig' is defined but never used           no-unused-vars
  43:9   error  'deleteInactiveAccounts' is defined but never used  no-unused-vars

ну да, всему виной actions/register, а он пока на операционном столе

@AVVS
Copy link
Member Author

AVVS commented Jun 20, 2016

@stainwoortsel относительно ошибок ремаппинг мы будем делать не каждый раз после каждого метода, а 1 раз во время ответа, есть возможность добавить функцию onComplete(err, data, actionName, actions), где err - ошибка, data - когда собственно ее нет, actionName - название события, actions - чаще всего undefined, а когда в AMQP QOS включен, то там ack/reject/retry

https://github.com/makeomatic/mservice/blob/master/src/plugins/amqp.js#L84-L94

}

module.exports.User = new UserModel(storage.User);
module.exports.Attempts = new AttemptsHelper(storage.Attempts);
module.exports.Tokens = new TokensHelper(storage.Tokens);
Copy link
Member Author

Choose a reason for hiding this comment

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

можно сократить и просто писать

exports.User = ...
exports.Attempts = ...

Copy link

@ghost ghost Jun 20, 2016

Choose a reason for hiding this comment

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

То есть Attempts и Tokens в классы не оборачивать?
Можно, но этж типа абстракция ) мало ли чего мы захотим вписать в методы по дефолту
Плюс оно обеспечивает требование по наличию в "стороже" нужных методов на этапе (типа) компиляции.
...ну не компиляции, но ошибка сразу найдется -- я к чему ) конечно, тесты -- дело святое, но так на ошибку наткнешься сразу и носом, еще на уровне линтера

Copy link
Member Author

@AVVS AVVS Jun 20, 2016

Choose a reason for hiding this comment

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

да не, я имею ввиду не нужно писать module.exports.SMTH =
пиши просто exports.SMTH =

-module.exports.User = new UserModel(storage.User);
+exports.User = new UserModel(storage.User);

Copy link

Choose a reason for hiding this comment

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

onComplete -- это, я так понимаю, в настройках defaults.js, в разделе amqp?

Copy link
Member Author

Choose a reason for hiding this comment

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

Все верно. Либо можем добавить хуки специальные в mservice по аналогии с hapi.js - там неплохо сделано, но это уже позже

Copy link

Choose a reason for hiding this comment

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

да не, я имею ввиду не нужно писать module.exports.SMTH =
пиши просто exports.SMTH =

а... ок )

@ghost
Copy link

ghost commented Jun 21, 2016

Так.. вижу branch conflict, но ветки я пока не мёржу.
Расскажу в общих чертах об изменениях:

  1. да, класс с регистрацией сделал
  2. перенес гугл-капчу в отдельную утилиту, тот код капчи что зависит от редиски -- в адаптере, в Utils. Гугл передается методу как функция-next) Utils.checkCaptcha.call(this, username, captcha, verifyGoogleCaptcha)`
  3. Помимо register были правки в send-email, он оставался единственным где был redis, не уверен что сделал его чисто

chmod +x $DIR/.bin/docker-compose
COMPOSE=$(which docker-compose)
# COMPOSE=$(which docker-compose)
COMPOSE="c:/dev/docker/docker-compose.exe"
Copy link
Member Author

Choose a reason for hiding this comment

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

ну да, так тесты и пройдут)

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

Successfully merging this pull request may close these issues.

3 participants