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

Add auth api methods #185

Merged
merged 17 commits into from
Feb 3, 2021
Merged

Add auth api methods #185

merged 17 commits into from
Feb 3, 2021

Conversation

svyborov
Copy link
Collaborator

Close #166

Изменения предложенные в пул реквесте:

  • Добавлен маршрут для колбека после авторизации в телеграмме
  • Добавлен маршрут для логаута

@qelphybox посмотри, пожалуйста

svyborov and others added 8 commits January 13, 2021 22:05
Bumps [webpack](https://github.com/webpack/webpack) from 5.11.1 to 5.13.0.
- [Release notes](https://github.com/webpack/webpack/releases)
- [Commits](webpack/webpack@v5.11.1...v5.13.0)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Bumps [sass-loader](https://github.com/webpack-contrib/sass-loader) from 10.1.0 to 10.1.1.
- [Release notes](https://github.com/webpack-contrib/sass-loader/releases)
- [Changelog](https://github.com/webpack-contrib/sass-loader/blob/master/CHANGELOG.md)
- [Commits](webpack-contrib/sass-loader@v10.1.0...v10.1.1)

Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Kirill Bobykin <qelphybox@gmail.com>
Copy link
Owner

@qelphybox qelphybox left a comment

Choose a reason for hiding this comment

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

Круто! Вижу ты выучил новые паттерны и практики. И код выглядит чище чем раньше, все абстракции красиво порезаны и не текут. Чисто пару комментов по коду есть, но это скорее мои предложения чем замечания.

src/api/controllers/test-auth-pages/home.html Outdated Show resolved Hide resolved
src/api/controllers/test-auth-pages/home.html Outdated Show resolved Hide resolved
src/api/services/auth.service.js Outdated Show resolved Hide resolved
src/api/services/auth.service.js Outdated Show resolved Hide resolved
.jest/setEnvVars.js Outdated Show resolved Hide resolved
Copy link
Owner

@qelphybox qelphybox left a comment

Choose a reason for hiding this comment

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

Теперь ревью того что действительно не хватает. Нехватает основного теста, который надо написать это e2e тест, то есть полностью сконфигурировать приложение, сделать тестовый запрос с параметрами и проверить что за время запроса происходит все что нужно. Вот статья, примерно о таких тестах я говорю.

тут сразу тестируется и роут, и параметры и что он возвращает. Поэтому тесту фронтенд сразу поймет, что происходит на запрос. И как он будет на реагировать в каких ситуациях. И код контроллера тоже будет полностью покрыт без моков.

const request = require('supertest')
const app = require('../server')
describe('Post Endpoints', () => {
  it('should create a new post', async () => {
    const res = await request(app)
      .post('/api/posts')
      .send({
        userId: 1,
        title: 'test is cool',
      })
    expect(res.statusCode).toEqual(201)
    expect(res.body).toHaveProperty('post')
  })
})

Поэтому это самый главный тест который надо организовать, я считаю

src/api/__tests__/controllers/auth.controller.test.js Outdated Show resolved Hide resolved
Copy link
Owner

@qelphybox qelphybox left a comment

Choose a reason for hiding this comment

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

Тесты отличные! 🔥 Вот тут уже все видно хорошо, как мне кажется.
У меня еще есть пара предложений на обсуждение, хочется закрыть вопрос с env переменными и токеном, этим потом ребята будут пользоваться, хочется выточить хорошую практику. Давай созвонимся обсудим.

src/api/services/auth.service.js Outdated Show resolved Hide resolved
src/api/__tests__/e2e/auth.e2e.test.js Outdated Show resolved Hide resolved
@qelphybox
Copy link
Owner

Сделай ещё утилку пожалуйста типа такой:

const generateUserSession = (tgToken, userData) => {
  // ...
  return { hash, ... userData }
}

Copy link
Owner

@qelphybox qelphybox left a comment

Choose a reason for hiding this comment

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

Круто, спасибо

@qelphybox qelphybox merged commit cfe0ccd into master Feb 3, 2021
@qelphybox qelphybox deleted the auth branch February 3, 2021 20:59
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.

Реализовать авторизацию на бэке
2 participants