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

Feature/issue 81 - Manager able to add LeaveTime #86

Merged
merged 14 commits into from
Apr 7, 2017

Conversation

Alexius-Huang
Copy link
Contributor

@Alexius-Huang Alexius-Huang commented Apr 6, 2017

  • 目前的表單是可以傳送,但是除了有 Time sensitive spec #84 的問題外,還有以下的spec正在修理:
    • leave_time_batch_builder_spec.rb: [ 23 / 51 / 71 ]
    • leave_time_builder_spec.rb: [ 22 / 34 / 52 / 73 / 93 / 111 ]
  • Review I18n 的部分是否有需要改的
  • 我在 rails console 裡面看到 LeaveTime 的部分這樣是正常的嗎? LeaveTime 物件裡的 leave_type 是 nil
    screen shot 2017-04-06 at 4 53 39 pm

@YushengLi
Copy link
Contributor

LeaveType 不應該允許空值才是

@Alexius-Huang
Copy link
Contributor Author

Alexius-Huang commented Apr 6, 2017

LeaveTime 中的 leave_type 已經修復,可以正常顯示,不過 rspec 部分還在看,估計是因為 remark 被設定成 required 所以在test時產生了很多衝突

= f.input :password, autocomplete: "off", hint: t('devise.hints.leave_blank'), required: false
= f.input :password_confirmation, required: false
= f.input :current_password, hint: t('devise.hints.confirm_password'), required: true
= f.input :login_name, label: "員工帳號", required: false
Copy link
Contributor

Choose a reason for hiding this comment

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

請盡可能使用 i18n 設定 label

@@ -70,7 +70,7 @@ def no_data_alert(message = t('warnings.no_data'))
end

def status_select_option
Copy link
Contributor

Choose a reason for hiding this comment

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

#87 中,對於 enum select options 的生成。

@@ -25,7 +26,7 @@ class LeaveTime < ApplicationRecord
delegate :seniority, :name, to: :user
has_many :leave_applications

validates :leave_type, :effective_date, :expiration_date, :quota, :usable_hours, :used_hours, :user, presence: true
validates :leave_type, :effective_date, :expiration_date, :quota, :usable_hours, :used_hours, :user, :remark, presence: true
Copy link
Contributor

Choose a reason for hiding this comment

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

我個人覺得 :remark 不需強制 present,這邊 @Eileen0917 覺得?

= f.input :quota
= f.input :effective_date, date_time_picker_hash(:start, current_object[:effective_date])
= f.input :expiration_date, date_time_picker_hash(:end, current_object[:expiration_date])
= f.input :remark, as: :text, required: true, label: t("title.backend/leave_times.remark")
Copy link
Contributor

Choose a reason for hiding this comment

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

請依照 i18n 中對於 Model attributes translation 的設定方式設定。

@@ -23,6 +23,7 @@
%ul.dropdown-menu
%li= link_to t("title.backend/users.index"), backend_users_path
%li= link_to t("title.backend/users.new"), new_backend_user_path
%li= link_to t("title.backend/users.new_leave_time"), new_backend_leave_time_path
Copy link
Contributor

Choose a reason for hiding this comment

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

如果這個跟 t("title.backend/leave_times.new") 沒有需要特別區隔開來的話,可以盡量用同一個 translation key 來降低維護 translation 的成本。

@@ -2,10 +2,11 @@

= simple_form_for [:backend, current_object] do |f|
= f.association :user, required: true
= f.input :leave_type, required: true, collection: LeaveTime::LEAVE_POOLS_TYPES_SYM
= f.input :leave_type, required: true, collection: LeaveTime.leave_types.keys.map(&:to_sym)
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly dependent on PR #87

sick_1: '病假'
sick_2: '半薪病假'
official: '公假'
marriage: '婚假'
maternity: '產假'
bereavement: '喪假' # Funeral & Bereavement 需要同時存在嗎?
Copy link
Contributor

Choose a reason for hiding this comment

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

不應該同時存在。

@YushengLi
Copy link
Contributor

Commit 1b9b82522b2fd6 fa8ad22似乎並不應該在此時出現,下次可以試著用用看 rebase 並卻保 working branch 永遠跟 upstream 的主 branch 同步。

使用 git rebase 避免無謂的 merge

@Alexius-Huang Alexius-Huang changed the title Feature/issue 81 Feature/issue 81 - Manager able to add LeaveTime Apr 7, 2017
@YushengLi YushengLi merged commit 1571e96 into 5xRuby:development Apr 7, 2017
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.

2 participants