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

定休日カレンダー機能を実装 #4974

Merged
merged 37 commits into from
Nov 30, 2021
Merged

Conversation

yKazihara
Copy link
Contributor

@yKazihara yKazihara commented Mar 17, 2021

概要(Overview・Refs Issue)

管理画面で定休日を設定し、フロントのブロックで2ヶ月分の営業日カレンダーを表示できるようにしました。

関連Issue

#4420

方針(Policy)

  • 管理画面で定休日の個別設定(登録・修正・削除)が可能

    • えんぴつボタンクリックで編集できます
    • Xボタンで削除できます
    • 既に存在する同日を登録しようとすると重複エラーとなります
      image
  • 管理画面>設定>店舗設定 に「定休日カレンダー設定」メニューを追加
    image

  • コンテンツ管理>レイアウト管理から、カレンダーブロックをドラッグ&ドロップし適宜配置していただく想定にしています

    image
    image

    • 土日について
      • EC-CUBEで構築されたサイトの定休日カレンダーをいくつか確認したところ、土日が休業表示されていることが多かった為、twigの処理で土日を定休日表示としています。
        削除もしくは編集が必要な場合は以下の部分を変更してください。
        image
  • 対応していないこと

    • 曜日設定は対応していません
    • csvファイルアップロードでの一括登録には対応していません
    • csvのエクスポートには対応していません
    • 既に導入されている定休日カレンダープラグインなどのデータ引き継ぎには対応していません
    • 既にカレンダーのプラグイン導入や実装をされている場合を考慮して、インストール後にデフォルト表示は行いません

実装に関する補足(Appendix)

  • フロントのカレンダーのスタイルについて
    以下をデフォルトとして設定しています。
    キャプチャは新着情報の下にカレンダーブロックを配置した場合。
    赤字が定休日、黄色の背景が本日です。
    image

テスト(Test)

  • 単体テスト実装済
  • Webテスト実装済

相談(Discussion)

既に導入済の機能とぶつからないように(カレンダーが2個表示されるなど)ブロックで配置していただく実装にしていますが、プラグインの方がよい、本体に追加だとこんなデメリットがある、などありましたらぜひお知らせ下さい。

マイナーバージョン互換性保持のための制限事項チェックリスト

  • 既存機能の仕様変更
  • フックポイントの呼び出しタイミングの変更
  • フックポイントのパラメータの削除・データ型の変更
  • twigファイルに渡しているパラメータの削除・データ型の変更
  • Serviceクラスの公開関数の、引数の削除・データ型の変更
  • 入出力ファイル(CSVなど)のフォーマット変更

レビュワー確認項目

  • 動作確認
  • コードレビュー
  • E2E/Unit テスト確認(テストの追加・変更が必要かどうか)
  • 互換性が保持されているか
  • セキュリティ上の問題がないか

@yKazihara yKazihara changed the title [WIP] 定休日カレンダー機能を実装 定休日カレンダー機能を実装 Mar 24, 2021
@codecov-io
Copy link

codecov-io commented Mar 24, 2021

Codecov Report

❗ No coverage uploaded for pull request base (4.0@6e33056). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##             4.0    #4974   +/-   ##
======================================
  Coverage       ?   76.31%           
  Complexity     ?     6178           
======================================
  Files          ?      442           
  Lines          ?    20943           
  Branches       ?        0           
======================================
  Hits           ?    15982           
  Misses         ?     4961           
  Partials       ?        0           
Flag Coverage Δ Complexity Δ
tests 76.31% <0.00%> (?) 0.00% <0.00%> (?%)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e33056...ca5b8b7. Read the comment docs.

@chihiro-adachi
Copy link
Contributor

編集時に重複エラーが発生します。
登録→鉛筆をクリック→決定で、重複エラーが発生。

<span class="ec-secHeading__line"></span>
<span class="ec-secHeading__ja">{{ 'front.block.calendar.title__ja'|trans }}</span>
</div>
* 赤字は休業日です。<br>
Copy link
Contributor

Choose a reason for hiding this comment

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

@yKazihara
message.yamlへの切り出しをお願いします。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

確認いただきありがとうございます。
文言を切り出しました。

@chihiro-adachi
Copy link
Contributor

@yKazihara
コメント内容修正されているのを確認しました。動作も問題ありません。
* 赤字は休業日ですだけ修正お願いします。

@chihiro-adachi chihiro-adachi added the enhancement 機能追加 label Mar 29, 2021
@chihiro-adachi chihiro-adachi added this to the 4.1 milestone Mar 29, 2021
@chihiro-adachi chihiro-adachi modified the milestones: 4.1, 4.1.x Sep 3, 2021
@matsuoshi matsuoshi changed the base branch from 4.0 to 4.1 November 15, 2021 06:24
@matsuoshi matsuoshi modified the milestones: 4.1.x, 4.1.1 Nov 15, 2021
Copy link
Contributor

@nanasess nanasess left a comment

Choose a reason for hiding this comment

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

@yKazihara いくつかコメント入れましたのでご確認お願いします

/**
* カレンダー設定の初期表示・登録
*
* @Route("/%eccube_admin_route%/setting/shop/calendar", name="admin_setting_shop_calendar")
Copy link
Contributor

Choose a reason for hiding this comment

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

methods={"GET", "POST"} を指定お願いします

}

/**
* @Route("/block/calendar", name="block_calendar")
Copy link
Contributor

Choose a reason for hiding this comment

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

methods={"GET"} を追記お願いします

$nextMonthCalendar = $this->setHolidayAndTodayFlag($nextMonthCalendar, $holidayListOfTwoMonths, $today->copy()->addMonth(1));

// 各カレンダータイトルを作成
$thisMonthTitle = $firstDateOfThisMonth->format('Y年n月');
Copy link
Contributor

Choose a reason for hiding this comment

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

メッセージリソースの使用をお願いします

class Calendar extends \Eccube\Entity\AbstractEntity
{
/**
* @var integer
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @var integer
* @var int

@var int が正しいです

throw new \Exception('Calendar not found. id = '.$id);
}

return $this->find($id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return $this->find($id);
return $calendar;

return $calendar; で良いと思います

/**
* delete.
*
* @param int|Calend $Calendar
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @param int|Calend $Calendar
* @param int|Calendar $Calendar

* @param int|Calendar $Calendar が正しいと思います

<th id="this-month-title" colspan="7" class="ec-calendar__title">{{ ThisMonthTitle }}</th>
</tr>
<tr>
<th class="ec-calendar__sun">日</th>
Copy link
Contributor

Choose a reason for hiding this comment

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

メッセージリソースを使用する必要がありそうです

<th id="next-month-title" colspan="7" class="ec-calendar__title">{{ NextMonthTitle }}</th>
</tr>
<tr>
<th class="ec-calendar__sun">日</th>
Copy link
Contributor

Choose a reason for hiding this comment

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

メッセージリソースを使用する必要がありそうです

use Eccube\Tests\EccubeTestCase;

/**
* TaxRuleRepository test cases.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* TaxRuleRepository test cases.
* CalendarRepository test cases.

/**
* TaxRuleRepository test cases.
*
* @author Kentaro Ohkouchi
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @author Kentaro Ohkouchi
* @author Yuko Kajihara

@matsuoshi
Copy link
Contributor

@nanasess 確認ありがとうございます、ひととおり修正反映しました

@carkn
Copy link
Contributor

carkn commented Nov 30, 2021

動作確認を行いました。問題ないかと思います。
マージします。

@k-yamamura
Copy link
Contributor

getHolidayListに

useResultCache(true, $this->getCacheLifetime())

も実装して欲しいですが不要なのでしょうか。

後すごい今更ですが、2系とEntity名や仕様を合わせてほしかったなと思います。

@matsuoshi matsuoshi added the affected:template フロントテンプレートの変更 label Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected:template フロントテンプレートの変更 enhancement 機能追加
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants