Terms ctrl refactoring and testing

dhilt commented 7 years ago

По подобию termsController.create нужно довести до соотв. вида метод update. И покрыть его тестами, написав спеку updateTermSpec.js.

Поизучай, какие данные уходят при работе с этим API через UI и напиши соотв. тесты. В тестах нужно работать с тестовым термином 'test-term', создаваемом в спеке newTermSpec.js. Работать от имени пользователя 'test-translator'. См _shared.js и как с ним работают спеки. Там есть шаренный логин и все необходимые данные.

Всякий раз при запуске тестов чистятся тестовые данные через вызов контроллерных методов на forceCleanUp. Это трогать не надо. Пользователи и термины создаются через уже существующие спеки.

dyuvzhenko commented 7 years ago

@dhilt Так, я в общем в процессе создания updateTermSpec.js: https://github.com/dhilt/dharmadict/commit/6b54db75105f9c5f352a199384ef3c8e6d9e26dc Пришлось все-таки создать локальный объект данных, который приходит с клиента:

  const testUpdateTerm = {
    termId: "chos",
    translation: {
      translatorId: "MK",
      language: "rus",
      meanings:
        [  { versions: ["a1", "b1", "c1"], comment: "comment1" },
           { versions: ["a1", "b1", "c1"], comment: null }  ]
    }
  };

И на этом моменте https://github.com/dhilt/dharmadict/commit/6b54db75105f9c5f352a199384ef3c8e6d9e26dc#diff-991282422eae7691b7a6c6b2dfd2efdaR72 происходят неожиданные явления (после этого теста выпадает ошибка), вроде как копирую отдельный объект, а выходит что копирую на самом деле уже измененный после предыдущих тестов.

dhilt commented 7 years ago

@BitDen По текущему workflow это не требуется, смотри:

1) admin создает термин (это делается в спеке newTermSpec); 2) translator добавляет перевод (это нужно делать в updateTermSpec); 3) translator редактирует перевод (там же в updateTermSpec); 4) и проверяем поиск, что найденный термин содержит последний перевод.

То есть admin не может добавлять переводы, он может только править их!

Потом спеку нужно будет дополнить добавлением перевода для того же термина второго тестового переводчика. Пока не делай этого.

dhilt commented 7 years ago

@BitDen А или ты его в тестах порождаешь этот объект? в общем, я должен посмотреть...

dyuvzhenko commented 7 years ago

@dhilt В общем-то я пока нацелился только на том, что спека принимает данные, которые должны обновить переводы от какого-то переводчика. И в этой самой спеке пока писал проверку на отсутствие данных, или неправильный тип данных.

dyuvzhenko commented 7 years ago

@dhilt И вроде бы админ может только создавать новые термины, а добавлять переводы или править - только переводчики.

dhilt commented 7 years ago

@BitDen Я надеюсь добраться сегодня до твоего кода. Тем временем я запустил http://dharmadict.ru! Там данные, полученные через db-reset.

dyuvzhenko commented 7 years ago

@dhilt Здорово! А насчет моего кода, тут видимо главная проблема в недопонимании самого js, мне попросту нужно сделать объект testUpdateTerm полностью статичным, чтобы нельзя было удалять/добавлять свойства (что можно сделать с помощью Object.freeze({}) как я выяснил). Ну и затем делать копию и прогонять в тестах. Копию коверкать конечно же.

dhilt commented 7 years ago

@BitDen Для тестовых данных используй конвенцию _shared.js (вроде https://github.com/dhilt/dharmadict/blob/master/test/_shared.js#L33). Там, где тебе нужен клон объекта, используй Object.create({}, sourceObj)

dyuvzhenko commented 7 years ago

@dhilt testTerm совсем не годится. Нужно около 10 тестов сделать именно на приходящий на сервер объект данных:

  const testUpdateTerm = {
    termId: "chos",
    translation: {
      translatorId: "MK",
      language: "rus",
      meanings:
        [  { versions: ["a1", "b1", "c1"], comment: "comment1" },
           { versions: ["a1", "b1", "c1"], comment: null }  ]
    }
  };

dyuvzhenko commented 7 years ago

@dhilt А, termId надо тестовым значением из _shared.js заменить, точно. И потом в тестировании на само добавление переводы добавлять к тестовому термину конечно же.

dhilt commented 7 years ago

@BitDen Да, этот объект будет фигурировать в нескольких спеках, он должен быть максимально удобно расшарен. И _shared.js – первая пришедшая мне в голову концепция на этот счет.

dyuvzhenko commented 7 years ago

В общем пока что добавил самопальную функцию для копирования объекта и продолжаю доделывать спеку: https://github.com/dhilt/dharmadict/commit/619a726b652cb3c59217be6799f9162637d5ed19

dyuvzhenko commented 7 years ago

@dhilt Добавил предпоследний тест: https://github.com/dhilt/dharmadict/commit/0955eebe908776f114a5e8ad78b820b4f88b4610

Но он не работает, самопальная функция дала сбой. Ожидается объект вида

const testUpdateTerm = {
    termId: "chos",
    translation: {
      translatorId: "MK",
      language: "rus",
      meanings:
        [  { versions: ["a1", "b1", "c1"], comment: "comment1" },
           { versions: ["a1", "b1", "c1"], comment: null }  ]
    }
  };

а получаем

const testUpdateTerm = {
    termId: "chos",
    translation: {
      translatorId: "MK",
      language: "rus",
      meanings:
        [  { versions: ["a1", "b1", "c1"], comment: "comment1" },
           { versions: ["a1", "b1", "c1"], comment: {} }  ]
    }
  };

где comment по задумке может быть либо строкой, либо же null.

dyuvzhenko commented 7 years ago

@dhilt И остается вроде как обработать следующую ошибку вида:

if (user.role !== 'translator') {
    throw new ApiError('Update term can only translator.', 302)
  }

Но вот как именно достать объект user, не до конца понимаю пока.

dhilt commented 7 years ago

@BitDen Сливай все в бранч, я когда доберусь, внесу правки, возможно дам комментарии. Отвечая на последний вопрос, посмотри, как сделана isAdmin на usersController. Можно обобщить эту функцию до hasRole.

dyuvzhenko commented 7 years ago

@dhilt Сливать все в 'master'? Сейчас там в 'dev' один из тестов не проходит, который я описал предпоследним комментарием.

dhilt commented 7 years ago

@BitDen Нет, мастер трогать не надо. Я должен посмотреть.

dyuvzhenko commented 7 years ago

@dhilt Ну вот и я так подумал. А так, все что успел сделать, все это есть в 'dev'.

dhilt commented 7 years ago

@BitDen Да, я уже начал смотреть.

dhilt commented 7 years ago

@BitDen Я довел updateTermSpec до момента Not found. Дальше надо

обновить-таки термин, добавив перевод testTermTranslation (убедиться, что перевод добавился)
добавить к этому переводу еще один meaning, допустим уже с одним versions и без комментариев, просто посылаешь более сложный объект на update, включающий в себя testTermTranslation (убедиться, что перевод добавился) (meanings должно стать 2!)
удалить 2 meaning из предыд. пункта (убедиться, что перевод удалился и у нас снова 1 meaning)
удалить исходный meaning (убедиться, что перевод пустой)

Таким образом мы покроем все сценарии update термина одним переводчиком.

Что значит "убедиться". Можно пойти двумя путями: запрашивать термин через тесты отдельным запросом (по сути отдельный проверочный тест) или полагаться на результат, возвращаемый контроллером. Поскольку мы еще не занимались тестированием GET (и есть еще ряд причин, но эта является критичной на данный момент), я предлагаю остановиться на втором варианте.

Во втором варианте меня смущает, что у нас там нечестный Promise.resolve(term), где term - просто аргумент вызова update, а не реальный результат. Посмотри, нельзя ли заставить эластик на апдейт отдавать реальный результат обновления, а не просто статус, что все ок. И если нет, то допиши контроллер так, чтобы после обновления был бы еще запрос на термин, используй связку findById и findTranslations для сужения выборки (ведь переводов может быть много, а нам нужен только тот, который соотв. переводчику, запускавшему update).

dyuvzhenko commented 7 years ago

@dhilt С возвращением term именно с базы данных, а не в качестве пришедшей переменной пока ничего не выходит. Дописал еще один then на методе update, но данные не приходят. Точнее переводы не приходят, только вот это { wylie: 'test term', translations: [], id: 'test_term' }. Коммит: https://github.com/dhilt/dharmadict/commit/4f41cc437d0dd1be4967e176afd0b745d05fba5b

dyuvzhenko commented 7 years ago

Пока что подготовлю оставшиеся три теста.

dyuvzhenko commented 7 years ago

@dhilt Три теста готовы - https://github.com/dhilt/dharmadict/commit/ae8aca38caa4e2e0265e5f249cb61bac74b0131f. Далее буду думать над возвращением переводов с базы данных...

dyuvzhenko commented 7 years ago

Пока что оставил метку, откуда стартует ошибка в виде отсутствующих переводов, и перейду к изучению вопроса русскоязычного поиска. Коммит: https://github.com/dhilt/dharmadict/commit/e732b37cef65cc7bc23c9b52559a4acc4fbd7165

dhilt commented 7 years ago

@BitDen Я сделал пару комитов, они решают проблему синхронизации апдейта БД, также я дописал тест "should update term". Можно продолжать писать тесты по описанному выше сценарию.

dyuvzhenko commented 7 years ago

@dhilt Здорово, сейчас буду дописывать.

dyuvzhenko commented 7 years ago

@dhilt Тесты дописал: https://github.com/dhilt/dharmadict/commit/1334948ab350c69c1afc312a87c350211b9b96e9

dyuvzhenko commented 7 years ago

@dhilt Меня все терзает небольшой вопрос насчет токена, очень странный наверное. Мы запихиваем в токен захешированный пароль, но при этом ни разу его не используем. Насколько это вообще правильно или разумно так делать, оставлять пароль в токене?

dhilt commented 7 years ago

@BitDen Ты, прав, я заменил токен на userInfo.

dhilt commented 7 years ago

@BitDen Я внес некоторое изменение, в связи с этим новое задание.

Суперадмин может редактировать чужие переводы. Я уже разрешил это на уровне валидации входных данных. Но второй then выдает пустоту. Нужно написать тест, в котором админ осуществляет редактирование перевода (заменяет первый meaning, например, или изменяет комментарий, максимально простая операция, поскольку редактирование по существу мы уже покрыли тестами) и под этот тест внести правки в метод update (я полагаю, что это 2й then).
Добавить 2го переводчика в тесты полностью аналогичного первому. Все то же самое, но с индексом 2 (_shared, добавление, удаление). В update спеке написать тест, в котором 2 переводчик пытается отредактировать перевод тестового термина 1 переводчика и получает отлуп Unpermitted access. Я уже начал писать этот тест, тебе нужно его закончить (подмени id переводчика в testTermTranslation.translatorId).
Кроме этого напиши тест, добавляющий новый перевод от лица 2 переводчика. Пока мы не можем проверить, что теперь у термина 2 перевода, просто оставь его висеть в БД. Это будет последний тест этой спеки. Потом будем покрывать search и это нам понадобится. Используй функцию addTestTranslation, добавь ей параметров, определяющих термин и переводчика.

dyuvzhenko commented 7 years ago

@dhilt В первом пункте немного "грязновато" сделал, переменной user, которая спускается с самого первого промиса присвоил id переводчика, даже если user это админ. Пойдет ли такой вариант? Коммит: https://github.com/dhilt/dharmadict/commit/b86d7ae5af149096ac2757a88f079575eaef4df6 2 пункт: https://github.com/dhilt/dharmadict/commit/06f4bc2c9962208bfb2c8ed6035e9c78f1bab17c 3 пункт: https://github.com/dhilt/dharmadict/commit/40710cf5771cfb1bda2e8ba130e79e1119739257 В 3-ем пункте применить функцию не удалось, ибо нужен token, id от другого переводчика translator_2. Но можно пожалуй переосмыслить эту функцию, и передавать ей второй аргумент в виде переводчика. И даже третий аргумент в виде testTermTranslation.

dhilt commented 7 years ago

@BitDen

Ты изменяешь comment, а проверяешь meanings. Убери все проверки, добавь проверку на comment. С user.id мне не очень нравится, но я потом подумаю, что с этим сделать. Пока хорошо, что изменение такое незначительное.
Убери console.log из теста.
По-моему я открытым текстом предложил добавить аргументов в addTestTranslation. И переименуй ее в updateTranslation.

Отлично!

Последним заданием в этой истории будет правка клиента на предмет обновления UI после редактирования термина. Когда ты поправил перевод и нажал "Сохранить и закрыть", тебя возвращает на экран с поиском и серверный ответ (реальные) должен отразиться на UI. Этот функционал сломался после того, как я заменил серверный ответ с входного объекта на реальный результат поиска после апдейта. Разбери возвращаемый сервером объект и подгони под него reducer.

dyuvzhenko commented 7 years ago

@dhilt Да, невнимательно прочитал что-то. Все добавил: https://github.com/dhilt/dharmadict/commit/2f76683e0bd0b0ca9777527a5246f195648841ed. В функцию updateTranslation достаточно передавать только testTerm или может еще создать новый testTermTranslation в ./_shared.js и вместо testTerm передавать одну из вариаций testTermTranslation?

dyuvzhenko commented 7 years ago

@dhilt Это однако оказалось непросто: https://github.com/dhilt/dharmadict/commit/edbf064f2d46e4dd8121f2f5634d8eb88715c2f2. Заметил что при неправильных данных (не знаю кто и что определяет правильность данных) на state.selected и при нажатии кнопки сохранить и выйти (кстати две кнопки работают вроде как одинаково), сразу после action TRANSLATION_UPDATE_END пробрасывается SELECT_TERM. И этот action вроде как должен был возвращать уже измененные данные с сервера с переводами одного выбранного пользователем термина, но возвращал старые данные. Возможно стоит позже посмотреть на actions...

dhilt commented 7 years ago

@BitDen Я так и не добрался до последнего кода и пока не получается. Посмотри, там в Backlog еще есть задача об изменении описания для пользователя.

dhilt / dharmadict

Terms ctrl refactoring and testing #13