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

提出物未チェックの場合、学習ステータスを修了にできないように変更 #7994

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions app/models/learning.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ class Learning < ApplicationRecord
presence: true,
uniqueness: { scope: :user_id }
validate :startable_practice
validate :submission_checked_for_completion, if: -> { practice.submission && status == 'complete' }

private

Expand All @@ -18,4 +19,11 @@ def startable_practice

errors.add :error, "すでに着手しているプラクティスがあります。\n提出物を提出するか修了すると新しいプラクティスを開始できます。"
end

def submission_checked_for_completion
product = practice.product(user)
return if product&.checked?
Comment on lines +24 to +25
Copy link
Member

Choose a reason for hiding this comment

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

2回使うのでなければproductという変数を作る意味はないかもです。(1行で良いと思います。)

Copy link
Member

Choose a reason for hiding this comment

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

@reckyy こちらいかがでしょうか?

Copy link
Contributor Author

@reckyy reckyy Nov 22, 2024

Choose a reason for hiding this comment

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

@komagata
おっしゃる通りです。
変数削除する修正をしたあと、返信させていただこうと思ってました。返信が遅れて申し訳ないです。 🙇

加えて長期間作業できておらず、ご迷惑をおかけしている点も重ねて申し訳ないです。
こちらのPRは一週間以内に作業再開するようにします!


errors.add :error, '提出物がチェックされていないため、修了にできません'
end
end
8 changes: 8 additions & 0 deletions test/decorators/user_decorator_test.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
# frozen_string_literal: true

require 'test_helper'
require 'supports/product_helper'
require 'active_decorator_test_case'

class UserDecoratorTest < ActiveDecoratorTestCase
include ProductHelper
Copy link
Member

Choose a reason for hiding this comment

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

他の箇所でもincludeとテストやsetupのメソッドは1行開けているので開けた方がいいかなと思います。

setup do
@admin_mentor_user = decorate(users(:komagata))
@student_user = decorate(users(:hajime))
Expand Down Expand Up @@ -74,23 +76,27 @@ class UserDecoratorTest < ActiveDecoratorTestCase
test '#completed_fraction don\'t calculate practice that include_progress: false' do
user = @admin_mentor_user
old_fraction = user.completed_practices_include_progress_size
create_checked_product(user, practices(:practice5))
user.completed_practices << practices(:practice5)

assert_not_equal old_fraction, user.completed_fraction

old_fraction = user.completed_practices_include_progress_size
create_checked_product(user, practices(:practice53))
user.completed_practices << practices(:practice53)

assert_equal old_fraction, user.completed_practices_include_progress_size
end

test '#completed_fraction don\'t calculate practice unrelated cource' do
old_fraction = @admin_mentor_user.completed_practices_include_progress_size
create_checked_product(@admin_mentor_user, practices(:practice5))
@admin_mentor_user.completed_practices << practices(:practice5)

assert_not_equal old_fraction, @admin_mentor_user.completed_practices_include_progress_size

old_fraction = @admin_mentor_user.completed_practices_include_progress_size
create_checked_product(@admin_mentor_user, practices(:practice55))
@admin_mentor_user.completed_practices << practices(:practice55)

assert_equal old_fraction, @admin_mentor_user.completed_practices_include_progress_size
Expand All @@ -99,6 +105,8 @@ class UserDecoratorTest < ActiveDecoratorTestCase
test '#completed_fraction_in_metas' do
fraction_in_metas = '2 (必須:1)'
@non_required_subject_completed_user.completed_practices = []
create_checked_product(@non_required_subject_completed_user, practices(:practice5))
create_checked_product(@non_required_subject_completed_user, practices(:practice61))
@non_required_subject_completed_user.completed_practices << practices(:practice5)
@non_required_subject_completed_user.completed_practices << practices(:practice61)
assert_equal fraction_in_metas, @non_required_subject_completed_user.completed_fraction_in_metas
Expand Down
4 changes: 2 additions & 2 deletions test/models/product_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ class ProductTest < ActiveSupport::TestCase
practice:
)

status = :complete
status = :started
Copy link
Contributor Author

Choose a reason for hiding this comment

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

このステータスを変更している理由は、このテストの目的から変更先のステータスが「修了」でなくてもいいと判断したからです。
もし修了のままだと、バリデーションを追加した関係で提出物をチェックする行程を追加しないといけなくコストが高くなるからです。

product.change_learning_status(status)
assert Learning.find_by(user:, practice:, status: :complete)
assert Learning.find_by(user:, practice:, status: :started)
end

test '#category' do
Expand Down
10 changes: 10 additions & 0 deletions test/models/user_test.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
# frozen_string_literal: true

require 'test_helper'
require 'supports/product_helper'

class UserTest < ActiveSupport::TestCase
include ProductHelper
test '#admin?' do
assert users(:komagata).admin?
assert users(:machida).admin?
Expand Down Expand Up @@ -75,11 +77,13 @@ class UserTest < ActiveSupport::TestCase
test '#completed_percentage don\'t calculate practice that include_progress: false' do
user = users(:komagata)
old_percentage = user.completed_percentage
create_checked_product(user, practices(:practice5))
user.completed_practices << practices(:practice5)

assert_not_equal old_percentage, user.completed_percentage

old_percentage = user.completed_percentage
create_checked_product(user, practices(:practice53))
user.completed_practices << practices(:practice53)

assert_equal old_percentage, user.completed_percentage
Expand All @@ -88,11 +92,13 @@ class UserTest < ActiveSupport::TestCase
test '#completed_percentage don\'t calculate practice unrelated cource' do
user = users(:komagata)
old_percentage = user.completed_percentage
create_checked_product(user, practices(:practice5))
user.completed_practices << practices(:practice5)

assert_not_equal old_percentage, user.completed_percentage

old_percentage = user.completed_percentage
create_checked_product(user, practices(:practice55))
user.completed_practices << practices(:practice55)

assert_equal old_percentage, user.completed_percentage
Expand Down Expand Up @@ -381,6 +387,7 @@ class UserTest < ActiveSupport::TestCase
practice2 = practices(:practice2)
today = Time.zone.today

create_checked_product(user, practice1)
Learning.create!(
user:,
practice: practice1,
Expand All @@ -389,6 +396,7 @@ class UserTest < ActiveSupport::TestCase
updated_at: (today - 2.weeks).to_formatted_s(:db)
)

create_checked_product(user, practice2)
Learning.create!(
user:,
practice: practice2,
Expand Down Expand Up @@ -425,6 +433,7 @@ class UserTest < ActiveSupport::TestCase
practice1 = practices(:practice1)
today = Time.zone.today

create_checked_product(user, practice1)
Learning.create!(
user:,
practice: practice1,
Expand All @@ -450,6 +459,7 @@ class UserTest < ActiveSupport::TestCase

machida = users(:machida)
practice1 = practices(:practice1)
create_checked_product(machida, practice1)
Learning.create!(
user: machida,
practice: practice1,
Expand Down
5 changes: 5 additions & 0 deletions test/supports/product_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,9 @@ def delete_most_unassigned_products!

products[2..products.size].each(&:delete)
end

def create_checked_product(user, practice)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

validation追加の関係で、テストがvalidationに引っかかるものがいくつかあったため対応策として作成しました。

product = Product.create(user:, practice:, body: 'test')
Check.create(user:, checkable: product)
end
end
3 changes: 3 additions & 0 deletions test/system/products_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ class ProductsTest < ApplicationSystemTestCase
end

test 'not display learning completion message when a user of the completed product visits after the second time' do
visit_with_auth "/products/#{products(:product65).id}", 'komagata'
click_button '提出物を確認'
assert_text '提出物の確認を取り消す'
visit_with_auth "/products/#{products(:product65).id}", 'kimura'
first('label.card-main-actions__muted-action.is-closer').click
assert_no_text '喜びをXにポストする!'
Expand Down