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

Base setup #943

Open
wants to merge 13 commits into
base: feature/calculator-constructor
Choose a base branch
from
Open

Base setup #943

wants to merge 13 commits into from

Conversation

ktpe
Copy link
Contributor

@ktpe ktpe commented Nov 7, 2024

dev

GitHub Board

Code reviewers

Second Level Review

Summary of issue

to create a foundation for the calculator builder.

Summary of change

the addition of models, updating of the controller, updating of new.html, and creation of new _partials were implemented. Additionally, the cocoon gem was added to enable working with multiple calculator fields.

Testing approach

ToDo

CHECK LIST

  • СI passed
  • Сode coverage >=95%
  • PR is reviewed manually again (to make sure you have 100% ready code)
  • All reviewers agreed to merge the PR
  • I've checked new feature as logged in and logged out user if needed
  • PR meets all conventions
01 02

@ktpe ktpe closed this Nov 7, 2024
@ktpe ktpe reopened this Nov 7, 2024
@ktpe ktpe changed the base branch from calculators_constructor to feature/calculator-constructor November 7, 2024 19:24
app/models/calculator.rb Show resolved Hide resolved
Comment on lines 27 to 37
validates :uk_name, :en_name, presence: true
validates :uk_name, :en_name,
length: { minimum: 3, maximum: 30 },
format: { with: /\A[\p{L}0-9\s'-]+\z/i },
uniqueness: { case_sensitive: false },
allow_blank: true
validates :priority, numericality: { greater_than_or_equal_to: 0 }

scope :ordered_by_name, -> { order(:uk_name) }
scope :ordered_by_priority, -> { order(:priority) }
scope :unsigned_categories, ->(product) { where.not(id: product.categories_by_prices) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

а чому це видалили?)

Copy link
Contributor Author

@ktpe ktpe Nov 10, 2024

Choose a reason for hiding this comment

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

повернули
але видаляли бо не до кінця було зрозуміло чи потрібен був цей код для реалізації конструктора

Comment on lines 29 to 30
has_many :categories, dependent: :destroy
accepts_nested_attributes_for :categories, reject_if: :all_blank, allow_destroy: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
has_many :categories, dependent: :destroy
accepts_nested_attributes_for :categories, reject_if: :all_blank, allow_destroy: true
has_many :categories, dependent: :destroy
accepts_nested_attributes_for :categories, reject_if: :all_blank, allow_destroy: true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

app/models/field.rb Show resolved Hide resolved
Copy link
Collaborator

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.

fixed

Comment on lines 4 to 6
t.string :expression, null: false, default: ""
t.string :uk_label, null: false, default: ""
t.string :en_label, null: false
Copy link
Collaborator

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.

ага, додали і для :en_label

db/migrate/20241107134610_create_formulas.rb Show resolved Hide resolved
Comment on lines 3 to 8
remove_columns :fields, :uuid, :selector, :type, :label, :name, :value, :from, :to, :kind, :unit

add_column :fields, :uk_label, :string, null: false, default: ""
add_column :fields, :en_label, :string, null: false, default: ""
add_column :fields, :var_name, :string, null: false, default: ""
add_column :fields, :field_type, :string, null: false, default: ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

change_table bulk: true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -0,0 +1,8 @@
class ChangeCategories < ActiveRecord::Migration[7.2]
def change
remove_columns :categories, :priority, :preferable
Copy link
Collaborator

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.

прибрав

Copy link

codecov bot commented Nov 9, 2024

Codecov Report

Attention: Patch coverage is 68.42105% with 6 lines in your changes missing coverage. Please review.

Project coverage is 64.09%. Comparing base (ada0b57) to head (c8ac0fa).

Files with missing lines Patch % Lines
app/controllers/account/calculators_controller.rb 25.00% 3 Missing ⚠️
app/models/formula.rb 0.00% 3 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (ada0b57) and HEAD (c8ac0fa). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (ada0b57) HEAD (c8ac0fa)
2 1
Additional details and impacted files
@@                         Coverage Diff                         @@
##           feature/calculator-constructor     #943       +/-   ##
===================================================================
- Coverage                           91.16%   64.09%   -27.07%     
===================================================================
  Files                                  70       71        +1     
  Lines                                 973      997       +24     
===================================================================
- Hits                                  887      639      -248     
- Misses                                 86      358      +272     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ktpe ktpe requested a review from loqimean November 10, 2024 09:34

self.selector = "#{kind&.first&.upcase}#{index}"
end
FIELD_TYPES = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Роби хешом, а ще тре конвертнеш в масив

<%= f.input :en_name, label: "Calculator Name:", class: 'form-control' %>

<!-- formula input-->
<div id="formulas" class="space-y-4">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Щось тут не гуд по ідентецшинам

Copy link
Collaborator

Choose a reason for hiding this comment

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

досі не гуд

@@ -23,4 +23,4 @@ production:
<<: *default
# database: zero_waste_production
# username: zero_waste
# password: <%= ENV['ZERO_WASTE_DATABASE_PASSWORD'] %>
# password: <%= ENV['ZERO_WASTE_DATABASE_PASSWORD'] %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Нова строка

db/migrate/20241107134610_create_formulas.rb Show resolved Hide resolved
change_table :categories, bulk: true do |t|
t.remove :preferable, type: :boolean

t.float :price, null: false, default: 0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Завжди для ціни юзай BigDecimal, чи то Decimal. Загугли

Copy link
Contributor Author

Choose a reason for hiding this comment

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

зробив через decimal
пишуть що BigDecimal вже буде юзатись у рубі коді, не в міграціях

Copy link
Collaborator

Choose a reason for hiding this comment

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

Видали, якщо не тре

Copy link
Collaborator

Choose a reason for hiding this comment

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

Теж

Copy link
Collaborator

Choose a reason for hiding this comment

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

видаляй)


before_create :set_selector, if: -> { selector.blank? }
# enum :kind, { form: 0, parameter: 1, result: 2 }
Copy link
Collaborator

Choose a reason for hiding this comment

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

kind, FIELD_TYPES

<%= f.input :en_name, label: "Calculator Name:", class: 'form-control' %>

<!-- formula input-->
<div id="formulas" class="space-y-4">
Copy link
Collaborator

Choose a reason for hiding this comment

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

досі не гуд

@ktpe ktpe requested a review from loqimean November 12, 2024 21:15
Copy link
Collaborator

Choose a reason for hiding this comment

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

видаляй

@@ -15,6 +15,13 @@ def show
# TODO: fill it
end

def new
@calculator = Calculator.new
Copy link
Collaborator

Choose a reason for hiding this comment

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

тести)

Comment on lines +17 to +18
validates :en_name, presence: true
validates :en_name,
Copy link
Collaborator

Choose a reason for hiding this comment

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

а чому нема укр назви?) також транслейтабл?)


private
enum :kind, { number: 0, category: 1 }
Copy link
Collaborator

Choose a reason for hiding this comment

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

юзайте текстові велєю + константи на ті велʼю


before_create :set_selector, if: -> { selector.blank? }
accepts_nested_attributes_for :categories, reject_if: :all_blank, allow_destroy: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

це має бути в самому низу

Comment on lines +4 to +8
<%= f.input :kind,
collection: Field.kinds.map { |k, _| [k.humanize, k] },
prompt: "Select Field Type",
label: "Field Type:",
input_html: { data: { field_type_target: "fieldTypeSelect" } } %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<%= f.input :kind,
collection: Field.kinds.map { |k, _| [k.humanize, k] },
prompt: "Select Field Type",
label: "Field Type:",
input_html: { data: { field_type_target: "fieldTypeSelect" } } %>
<%= f.input(
:kind,
collection: Field.kinds.map { |k, _| [k.humanize, k] },
prompt: "Select Field Type",
label: "Field Type:",
input_html: { data: { field_type_target: "fieldTypeSelect" }}
) %>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants