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

Show calculator #946

Open
wants to merge 13 commits into
base: base-setup
Choose a base branch
from
Open

Show calculator #946

wants to merge 13 commits into from

Conversation

NVMakarenko
Copy link
Contributor

@NVMakarenko NVMakarenko commented Nov 8, 2024

dev

Project

Code reviewers

Summary of issue

If Admin created calculator via constructor, User could see it.

Summary of change

image

  • add show page for user
  • add routes

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

@NVMakarenko NVMakarenko changed the base branch from master to base-setup November 8, 2024 16:36
Copy link

codecov bot commented Nov 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (base-setup@5de9f1d). Learn more about missing BASE report.

Additional details and impacted files
@@              Coverage Diff              @@
##             base-setup     #946   +/-   ##
=============================================
  Coverage              ?   64.66%           
=============================================
  Files                 ?       71           
  Lines                 ?      982           
  Branches              ?        0           
=============================================
  Hits                  ?      635           
  Misses                ?      347           
  Partials              ?        0           

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

@@ -59,7 +59,7 @@ def collection
end

def calculator
@calculator = Calculator.friendly.find(params[:slug])
@calculator = Calculator.find(params[:slug])
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
Contributor Author

Choose a reason for hiding this comment

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

SEO-friendly урла залишена з попердньої реалізації.

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.

я і не казав забирати слаг

<td><%= calculator.id %></td>
<td><%= calculator.name %></td>
<td><%= calculator.en_name %></td>
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
<td><%= calculator.en_name %></td>
<td><%= calculator.name %></td>

app/views/calculators/show.html.erb Show resolved Hide resolved
<div class="container place-items-center">
<h1 class="text-4xl font-bold">Calculator <%= @calculator.en_name %></h1><br>

<%= form_with url: calculate_account_calculator_path(slug: @calculator.slug || @calculator.id) do |form| %>
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
<%= form_with url: calculate_account_calculator_path(slug: @calculator.slug || @calculator.id) do |form| %>
<%= form_with url: calculate_account_calculator_path(@calculator) do |form| %>

@@ -59,7 +59,7 @@ def collection
end

def calculator
@calculator = Calculator.friendly.find(params[:slug])
@calculator = Calculator.find(params[:slug])
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 31 to 33
def name
(I18n.locale == :uk) ? uk_name : en_name
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -59,7 +59,7 @@ def collection
end

def calculator
@calculator = Calculator.friendly.find(params[:slug])
@calculator = Calculator.find(params[:slug])
Copy link
Collaborator

Choose a reason for hiding this comment

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

я і не казав забирати слаг

@@ -45,6 +46,6 @@ def collection
end

def resource
collection.friendly.find(params[:slug])
collection.find(params[:slug])
Copy link
Collaborator

Choose a reason for hiding this comment

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

а чому шукає слаг, але скоуп френдлі видаляється?

@@ -1,7 +1,7 @@
# frozen_string_literal: true

class Account::CalculatorsController < Account::BaseController
before_action :calculator, only: [:edit, :update, :destroy]
before_action :resource, only: [:edit, :update, :destroy]
Copy link
Collaborator

Choose a reason for hiding this comment

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

видали цей біфор екшин

end

it "assigns the correct calculator to @calculator" do
get :show, params: { slug: calculator.slug, locale: :en }
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
get :show, params: { slug: calculator.slug, locale: :en }
get :show, params: { slug: calculator.slug, locale: :en }

Copy link
Collaborator

Choose a reason for hiding this comment

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

і так скрізь. розділяй дії та експектейшини

@loqimean
Copy link
Collaborator

loqimean commented Dec 1, 2024

порізолвай конфлікти

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

Successfully merging this pull request may close these issues.

3 participants