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

Improve resume skills description #744

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

eKulshan
Copy link

@eKulshan eKulshan commented May 8, 2024

resolves #711

@fey
Copy link
Collaborator

fey commented May 8, 2024

Задеплойте пожалуйста демку, чтобы посмотреть визуально

@eKulshan
Copy link
Author

eKulshan commented May 8, 2024

Задеплойте пожалуйста демку, чтобы посмотреть визуально

Мне доступен только деплой на рендер, но там надо добавлять файлы в репозиторий
Мне кажется это не подходящий способ, так как они отобразятся в пул реквесте
Не подскажете варианты какие есть еще? Хероку у меня не работает к сожалению

@fey
Copy link
Collaborator

fey commented May 8, 2024

@eiKulshan а что за файлы нужно добавлять? По идее деплой на рендер должен работать обычным способом, там есть поддержка рельсы. Посмотрите в ридми инструкцию по деплою https://github.com/Hexlet/hexlet-cv#deploy-to-rendercom

@eKulshan
Copy link
Author

@fey спасибо за ссылку, разобрался
все скрипты для деплоя на рендер уже были в репозитории, я про них говорил
задеплоил демо

@usernaimandrey
Copy link
Contributor

Ну и тест добавить, что если строка длинная резюме не сохранится

@fey fey requested a review from amshkv May 17, 2024 18:14
@amshkv amshkv requested a review from corsicanec82 May 20, 2024 08:03
app/views/web/shared/_resume_form.html.slim Show resolved Hide resolved
app/views/web/resumes/_information.html.slim Outdated Show resolved Hide resolved
app/models/resume.rb Outdated Show resolved Hide resolved
@eKulshan
Copy link
Author

Спасибо за ревью!
Внес правки.
Не совсем уверен в реализации презентера и валидации.
И не уверен что тест удачно написан.
Если можно улучшить, подскажите как, исправлю

# frozen_string_literal: true

module ResumePresenter
def skills
Copy link
Contributor

Choose a reason for hiding this comment

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

У модели резюме уже есть метод skills, через который работает гем ActsAsTaggableOn, тут ты его переопределил, нужно выбрать другое имя метода

resume = Resume.find_by(name: attrs[:name])
assert { !resume }

attrs[:skills_description] = 'Invalid_skill_lenght' * 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Если тестируешь два разных кейса то лучше разделить на два теста

resume: attrs
}

post(account_resumes_path, params:)
Copy link
Contributor

Choose a reason for hiding this comment

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

в параметры добавь пожалуйста locale, что бы тест прогонялся для двух локалей

post account_resumes_path, params:, locale: I18n.locale

@@ -233,6 +233,7 @@
t.string "evaluated_ai_state"
t.text "projects_description"
t.text "about_myself"
t.text "skills_description", limit: 250
Copy link
Contributor

Choose a reason for hiding this comment

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

прогони плиз еще раз миграцию, в схеме осталась валидация на длину на уровне базы

@@ -128,6 +131,12 @@ def initialize(attribute = nil)
super(attrs_with_defaults)
end

def skills_description_valid_format
return unless skills.size > 10 || skills.any? { |element| element.size > 25 }
Copy link
Contributor

Choose a reason for hiding this comment

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

почему или, должно же быть и - длинна скила не больше 25 и не больше 10 скилов

@@ -84,6 +84,7 @@ group :development, :test do
# Call 'byebug' anywhere in the code to stop execution and get a debugger console
gem 'byebug', platforms: %i[mri mingw x64_mingw]
# Use sqlite3 as the database for Active Record
gem 'debug'
Copy link
Contributor

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.

Отредактировать текст и поля, добавить пару фич
5 participants