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 admin vacancies on moderation #677

Merged

Conversation

qsimpleq
Copy link
Contributor

Добавил фичу #674

@qsimpleq
Copy link
Contributor Author

@fey, @usernaimandrey

Есть два вопроса:

  1. Новый issue для фронтов - переделать:
    image
    первую строку отображаемую в отдельную сущность-строку,
    Даты - отдельной строкой
    Отправить-сбросить - отдельной строкой

  2. Почему в тесте не подгружается переменная @vacancies из контроллера, хотя там есть fixture необходимая?

@fey
Copy link
Collaborator

fey commented Sep 19, 2023

первую строку отображаемую в отдельную сущность-строку,

не совсем понял, как это будет выглядеть и зачем.

Почему в тесте не подгружается переменная @vacancies из контроллера, хотя там есть fixture необходимая?

тут нужен @usernaimandrey для ответа

PS Пожалуйста, задеплойте результат на render.com чтобы потыкать кнопочки.

@qsimpleq
Copy link
Contributor Author

Улучшение UX

  1. В рамках ссылки 'На модерации' это выглядит плохо, т.к. там выбор статуса не нужен
    image
  2. Даты должны быть отдельной строкой, в такой форме
  3. Плывут кнопки подтверждения и сброса, а они должны быть внизу формы

@qsimpleq
Copy link
Contributor Author

Тут вопрос конкретно по стилям, не более того.

Задача выполнена, но есть нюансы)

@fey
Copy link
Collaborator

fey commented Sep 19, 2023

Ну вообще кажется, что можно просто открывать страницу вакансий с нужным фильтром. То есть по сути "на модерации" - это просто страница типа

https://cv.hexlet.io/ru/admin/vacancies?q%5Bstate_eq%5D=on_moderate

@qsimpleq
Copy link
Contributor Author

Фильтры сбиваются - поганый UX

@qsimpleq
Copy link
Contributor Author

qsimpleq commented Sep 19, 2023

Это страница для модерируемых сообщений - не более
Там не нужен дополнительный фильтр.
В итоге всё уезжает, хотя там и так было грустно)

@qsimpleq qsimpleq closed this Sep 19, 2023
@qsimpleq qsimpleq reopened this Sep 19, 2023
@usernaimandrey
Copy link
Contributor

usernaimandrey commented Sep 19, 2023

Почему в тесте не подгружается переменная @vacancies из контроллера, хотя там есть fixture необходимая?

https://github.com/Hexlet/hexlet-cv/blob/main/test/controllers/web/admin/vacancies_controller_test.rb#L7 - вот здесь чтоли?

там отдельно в тест кейсах edit и update она достается, а в других не используется по этому нет смысла ее в сетап пихать

def on_moderate
params[:q] ||= {}
params[:q][:state_eq] = 'on_moderate'
index
Copy link
Contributor

Choose a reason for hiding this comment

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

лучше так не надо делать, раз уж ты создал для этого дела отдельный экшен то и вьюху отдельную создай on_moderate.html.slim и табличку из index можно вынести в отдельный паршл и ее там переиспользовать, когда 2 экшена используют одну и ту же вьюху это сбивает столку

Copy link
Contributor

Choose a reason for hiding this comment

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

Посути здесь надо просто сделать выборку вакансий со стэйтом на модерации, сортировку сделать начиная с самых старых

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Максимально независимо бить?

Забить на dry я давно понял)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Посути здесь надо просто сделать выборку вакансий со стэйтом на модерации, сортировку сделать начиная с самых старых

Не было в ТЗ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

лучше так не надо делать, раз уж ты создал для этого дела отдельный экшен то и вьюху отдельную создай on_moderate.html.slim и табличку из index можно вынести в отдельный паршл и ее там переиспользовать, когда 2 экшена используют одну и ту же вьюху это сбивает с толку

И хочется и колется - код один на всё - больно делить на ровном месте, но придётся)

Copy link
Contributor

Choose a reason for hiding this comment

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

Забить на dry я давно понял)
ну на dry тут ни кто не забивает, но в рельсе так не принято, это все равно что использовать один и тот же контроллер Vacancies для админа и обычного пользователя

@@ -15,6 +15,8 @@ html.h-100 lang="#{I18n.locale}"
= sidebar_menu_item t('.admins'), 'bi-person-fill-gear', admin_root_path
= sidebar_menu_item t('.all_users'), 'bi-people', admin_users_path
= sidebar_menu_item t('.all_resumes'), 'bi-card-text', admin_resumes_path
= sidebar_menu_divider t('.vacancies')
= sidebar_menu_item t('.on_moderate'), 'bi-briefcase', on_moderate_admin_vacancies_path
= sidebar_menu_item t('.all_vacancies'), 'bi-briefcase', admin_vacancies_path
Copy link
Contributor

Choose a reason for hiding this comment

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

давай сюда такой бейдж briefcase-fill, а выше тот оставим

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ок, сделаю завтра

@@ -1,8 +1,10 @@
.p-3.mb-3.bg-light
= search_form_for q, default_filter_form_options(url: admin_vacancies_path, html: { class: 'row row-cols-1 row-cols-sm-2 row-cols-lg-3 row-cols-xl-4 g-2 g-lg-3', novalidate: true }) do |f|
- form_path = current_page?(on_moderate_admin_vacancies_path) ? on_moderate_admin_vacancies_path : admin_vacancies_path
Copy link
Contributor

Choose a reason for hiding this comment

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

path можно передавать как параметр в паршл, просто если фильтр надо будет использовать еще где то то условие разрастется

= f.input :company_name_cont, placeholder: han('vacancy', :company_name), label: false
= f.input :creator_first_name_or_creator_last_name_or_creator_email_cont, label: false, placeholder: t('.creator')
= f.input :state_eq, collection: states_for_select(Vacancy), prompt: han('vacancy', :state), include_blank: true, label: false
- unless current_page?(on_moderate_admin_vacancies_path)
= f.input :state_eq, collection: states_for_select(Vacancy), prompt: han('vacancy', :state), include_blank: true, label: false
Copy link
Contributor

Choose a reason for hiding this comment

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

тоже можно в виде параметра передать, так абстракция не будет течь, это же паршл и он посути не должен, да ему и не надо знать где его подключили, он просто в зависимости от преданных параметров отображает необходимые поля
= render search_form path: ..., is_visible_state: true - что то типа

@qsimpleq
Copy link
Contributor Author

Итоговый вид
image

@fey
Copy link
Collaborator

fey commented Sep 20, 2023

@qsimpleq задеплойте плз на render.com результат.

./bin/rails db:migrate
./bin/rails db:fixtures:load


Copy link
Contributor

Choose a reason for hiding this comment

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

Это конечно надо было в рамках другого ПР делать, мы же здесь другую задачу решаем

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Я ревертну потом. Сейчас я тестирую эту фичу

./bin/rails db:create
./bin/rails db:schema:load
./bin/rails db:migrate
./bin/rails db:fixtures:load
Copy link
Contributor

Choose a reason for hiding this comment

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

это в продакшен режиме зафейлится

Copy link
Contributor Author

@qsimpleq qsimpleq Sep 20, 2023

Choose a reason for hiding this comment

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

render.com - тестовый стенд для этого проекта

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Требуется показ развёртывания, проблема в этом

Copy link
Contributor

Choose a reason for hiding this comment

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

render.com - тестовый стенд для этого проекта

да я знаю я туда выкатывал демку CV, но это полноценный продакшен, и вот та команда по загрузке фикстур упадет, либо меняй при развертывании переменную окружения, хотя я если чесно так не пробовал
вот демка репы https://github.com/usernaimandrey/demo-hexlet-cv

Copy link
Contributor

Choose a reason for hiding this comment

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

Ты пробовал с этим скриптом выкатить в рендер и чтобы все работало?

bundle exec rake assets:precompile
bundle exec rake assets:clean

./bin/rails db:drop
Copy link
Contributor

Choose a reason for hiding this comment

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

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

#!/usr/bin/env bash
# exit on error
set -o errexit

bundle install
npm i
npm run build
bundle exec rake assets:precompile
bundle exec rake assets:clean
bundle exec rake db:migrate

Copy link
Contributor Author

@qsimpleq qsimpleq Sep 20, 2023

Choose a reason for hiding this comment

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

Я откачу эти изменения. Проблема в том, что моё решение без выкладки на левый ресурс - не тестируют

Copy link
Contributor Author

Choose a reason for hiding this comment

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

У меня на vps места больше нет, чтобы самому развернуть спокойно

th = sort_link(@q, 'published_at')
td = t('actions')
tbody
- @vacancies.each do |vacancy|
Copy link
Contributor

Choose a reason for hiding this comment

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

инстанс-пременные лучше не использовать в паршлах, лучше передовать их так же ввиде праметров

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ок

= f.input :company_name_cont, placeholder: han('vacancy', :company_name), label: false
= f.input :creator_first_name_or_creator_last_name_or_creator_email_cont, label: false, placeholder: t('.creator')
= f.input :state_eq, collection: states_for_select(Vacancy), prompt: han('vacancy', :state), include_blank: true, label: false
= f.input :state_eq, collection: states_for_select(Vacancy), prompt: han('vacancy', :state), include_blank: true, label: false, disabled: is_state_disabled
Copy link
Contributor

Choose a reason for hiding this comment

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

давай в экшене on_moderate совсем это поле не рендерить, смысл в нем нет

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Едет вёрстка. Проблема в этом

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@usernaimandrey , пример тут #677 (comment)

Это действительно плохо выглядит

Copy link
Contributor

@usernaimandrey usernaimandrey Sep 25, 2023

Choose a reason for hiding this comment

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

Тогда давай оставим, так у меня больше нет замечаний)


private

def index_respond_to(query)
Copy link
Contributor

Choose a reason for hiding this comment

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

это можно оставить в индексе, там обычно на модерации не так много вакансий что бы их выгружать

@@ -62,4 +38,39 @@ def update
render :edit, status: :unprocessable_entity
end
end

def on_moderate
query = { s: 'created_at asc', state_eq: 'on_moderate' }.merge(params.permit![:q] || {})
Copy link
Contributor

Choose a reason for hiding this comment

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

в этом экшене можно просто выбрать вакансии которые только со стейто on_moderate:

@q = Vacancy.on_moderate.ransack(query)
@vacancies = @q.result(distinct: true).page(params[:page])

query parms пихнуть в отделный метод

def query_params(default_params = { })
default_params.merge(params.permit![:q] || {})
end

Copy link
Contributor Author

@qsimpleq qsimpleq Sep 25, 2023

Choose a reason for hiding this comment

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

@usernaimandrey, тогда ссылку на экспорт со страницы on_moderate убираем, чтобы не было сломанного функционала?

Я просто ради этого и выносил index_respond_to

Copy link
Contributor

Choose a reason for hiding this comment

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

да убираем

@fey fey marked this pull request as draft September 21, 2023 09:54
@qsimpleq
Copy link
Contributor Author

qsimpleq commented Sep 21, 2023

Насколько всё проще, когда есть доступ к консоли сервиса или место на своём vps, а не этот драный render.com :(

@qsimpleq
Copy link
Contributor Author

qsimpleq commented Sep 21, 2023

Я теперь понимаю почему парень отказался писать доку по деплою на этот самый render.com

Это - пытка

Отладка на уровне - невозможно, в рамках бесплатного доступа!!!

@usernaimandrey
Copy link
Contributor

Насколько всё проще, когда есть доступ к консоли сервиса или место на своём vps, а не этот драный render.com :(

у них вроде тоже есть https://render.com/docs/cli

@qsimpleq
Copy link
Contributor Author

qsimpleq commented Sep 21, 2023

Там буквально alpha и полностью бесполезная

@qsimpleq qsimpleq force-pushed the feature-admin-vacancies-on-moderation branch from 38c091c to accaa39 Compare September 24, 2023 17:29
@qsimpleq qsimpleq force-pushed the feature-admin-vacancies-on-moderation branch from accaa39 to 6470b2f Compare September 25, 2023 08:29
@qsimpleq
Copy link
Contributor Author

@qsimpleq qsimpleq marked this pull request as ready for review September 25, 2023 10:32
@@ -62,4 +62,16 @@ def update
render :edit, status: :unprocessable_entity
end
end

def on_moderate
query = query_params({ s: 'created_at asc', state_eq: 'on_moderate' })
Copy link
Contributor

Choose a reason for hiding this comment

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

Буквально пару штрихов, в query_params стейт state_eq: 'on_moderate' уже же не надо передавать у нас же ниже скоуп и так делает выборку только on_moderate

Copy link
Contributor Author

@qsimpleq qsimpleq Sep 25, 2023

Choose a reason for hiding this comment

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

Я для того, чтобы выбран селект был оставил, иначе как на картинке тут будет #677 (comment)

А если убираем селект, то будет так
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Давай поле просто за дисейблим, а из запроса вот это уберем state_eq: 'on_moderate', сделаешь?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ок

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Готово

@fey
Copy link
Collaborator

fey commented Sep 25, 2023

по мне выглядит ок. Попрошу Алису глянуть. пока не вырубайте.

@fey
Copy link
Collaborator

fey commented Sep 26, 2023

внешне выглядит ок по мне. Как код ревью закончите - можно мержить =)

@usernaimandrey usernaimandrey merged commit 2da9f0d into Hexlet:main Sep 26, 2023
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