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

career path #512

Merged
merged 2 commits into from Apr 26, 2023
Merged

career path #512

merged 2 commits into from Apr 26, 2023

Conversation

usernaimandrey
Copy link
Contributor

No description provided.

@usernaimandrey usernaimandrey marked this pull request as draft April 12, 2023 17:14
@usernaimandrey usernaimandrey force-pushed the creer-path branch 2 times, most recently from 3f40c39 to e23d173 Compare April 13, 2023 20:09
@usernaimandrey usernaimandrey marked this pull request as ready for review April 16, 2023 18:55
Comment on lines 11 to 16
import editorRun from '../scripts/markdownEditor.js';
import '../scripts/selectVacancyFilter.js';

ujs.start();
// editorRun();
editorRun();
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.

Я его только в админки включил, проверил пока работает нормально

@usernaimandrey
Copy link
Contributor Author

Это пока только админка для создания треков и шагов

Copy link
Contributor

@grozwalker grozwalker left a comment

Choose a reason for hiding this comment

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

все супер, написал просто свои вопросики

app/controllers/web/admin/steps_controller.rb Outdated Show resolved Hide resolved
app/controllers/web/admin/steps_controller.rb Outdated Show resolved Hide resolved
include CareerRepository

belongs_to :creator, class_name: 'User'
has_many :items, dependent: :nullify, class_name: 'Career::Item'
Copy link
Contributor

Choose a reason for hiding this comment

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

перепутаны местами dependent и class_name (Прости за придирание к опечаткам)

@@ -5,5 +5,6 @@ module UserRepository

included do
scope :web, -> { order(id: :desc).permitted }
scope :in_career_path, -> { includes(member: :career).joins(member: :career) }
Copy link
Contributor

Choose a reason for hiding this comment

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

in_career_path - звучит, как роут. Может найти какое-то более ясное наименование? has_career или что-то такое

Copy link
Contributor

Choose a reason for hiding this comment

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

может и назвать in_career_track? in или has не знаю, но вместо path использовать track

app/models/step.rb Outdated Show resolved Hide resolved
app/models/career.rb Outdated Show resolved Hide resolved
app/models/career.rb Outdated Show resolved Hide resolved
config/routes.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
test/fixtures/career.yml Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
# frozen_string_literal: true

class Web::Admin::Api::ApplicationController < Web::Admin::ApplicationController
Copy link
Contributor

Choose a reason for hiding this comment

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

обычно web это противоположно api и кажется должны быть разные неймспейсы

@@ -5,5 +5,7 @@ module UserRepository

included do
scope :web, -> { order(id: :desc).permitted }
scope :in_career_track, -> { includes(member: :career).joins(member: :career) }
scope :current_career_path, ->(career) { joins(members: :career).where(members: { state: 'active', career: }) }
Copy link
Contributor

Choose a reason for hiding this comment

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

чтобы не использовать строки (https://ru.hexlet.io/blog/posts/izbavlyaytes-ot-strok)

можно сделать where.merge(...Member.with_state(:active))

if @member.save
f(:success)
else
f(:error)
Copy link
Contributor

Choose a reason for hiding this comment

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

при ошибке же недолжен редиректится а должен new рендерится с отображением ошибок

Copy link
Contributor

Choose a reason for hiding this comment

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

@career_member = Career::Member.new внутри не будет же ошибок как же их отобразить

redirect_to admin_careers_path
else
f(:error)
@career = career.becomes(Career)
Copy link
Contributor

Choose a reason for hiding this comment

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

кажется можно избавится от becomes если сделать наоборот, поменять местами career и @career


validates :name, :description, :locale, presence: true
validates :slug, presence: true, uniqueness: { case_sensitive: false }
has_many :items, class_name: 'Career::Item', dependent: :nullify
Copy link
Contributor

Choose a reason for hiding this comment

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

nullify тут ок?

@@ -24,6 +24,7 @@ class User < ApplicationRecord
has_many :resume_answer_likes, through: :resume_answers, source: :likes
has_many :resume_comments, class_name: 'Resume::Comment', dependent: :destroy
has_many :notifications, dependent: :destroy
has_many :members, class_name: 'Career::Member', dependent: :destroy
Copy link
Contributor

Choose a reason for hiding this comment

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

has_many :members
предложил бы уточнять в названии какие именно мемберы career_members. Так как юзер потенциально может быть мембером и для других сущностей в будущем

@@ -8,4 +8,18 @@ def full_name
end

alias to_s full_name

def date_finished_career_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 тут не нужно в имени

@usernaimandrey usernaimandrey merged commit 0843f78 into Hexlet:main Apr 26, 2023
1 check passed

class Web::Admin::Careers::MembersController < Web::Admin::Careers::ApplicationController
def new
resource_career
Copy link
Contributor

Choose a reason for hiding this comment

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

??

Comment on lines +45 to +46
@items = career.items.order(order: :asc)
@steps = career.steps
Copy link
Contributor

Choose a reason for hiding this comment

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

это для формы же? тогда есть два варика как это сделать лучше

  1. брать прям в шаблоне carrer.items
  2. добавить в класс формы методы для получения этих штук ну и вызывать в шаблоне

@@ -99,4 +99,11 @@ def seo_for_paging(number_page, text)

"#{t('page', number: number_page)}-#{text}"
end

# rubocop:disable Rails/HelperInstanceVariable
def append_javascript_packs(*packs)
Copy link
Contributor

Choose a reason for hiding this comment

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

а чет я не нашел - кто потом рендерить эти javascript_packs на страницу?

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.

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

usernaimandrey added a commit to usernaimandrey/hexlet-cv that referenced this pull request Apr 26, 2023
usernaimandrey added a commit that referenced this pull request Apr 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.

None yet

6 participants