-
Notifications
You must be signed in to change notification settings - Fork 268
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
Speed up entry creation #422
Speed up entry creation #422
Conversation
= f.association :project, required: true, collection: Project.unarchived.by_name, label: false, placeholder: t("entries.index.project") | ||
= f.input :value, required: true, as: :integer, label:false, placeholder: t("entries.index.mileages") | ||
= f.association :project, required: true, collection: Project.unarchived.by_name, label: false, placeholder: t("entries.index.project"), selected: (@last_mileage_logged.project_id if @last_mileage_logged ) | ||
= f.input :value, required: true, as: :integer, label:false, placeholder: t("entries.index.mileages"), input_html: { value: (@last_mileage_logged.value if @last_mileage_logged) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using instance variables in partials views
Line is too long. [180/80]
@@ -1,6 +1,6 @@ | |||
= simple_form_for @mileages_entry do |f| | |||
= f.error_notification | |||
= f.association :project, required: true, collection: Project.unarchived.by_name, label: false, placeholder: t("entries.index.project") | |||
= f.input :value, required: true, as: :integer, label:false, placeholder: t("entries.index.mileages") | |||
= f.association :project, required: true, collection: Project.unarchived.by_name, label: false, placeholder: t("entries.index.project"), selected: (@last_mileage_logged.project_id if @last_mileage_logged ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using instance variables in partials views
Line is too long. [207/80]
= f.input :date, required: true, as: :string, input_html: { value: (@hours_entry.date || DateTime.current).strftime("%d/%m/%Y"), class: "datepicker"}, label: false | ||
.taggable | ||
= f.input :description, input_html: { data: { data: Tag.list }, autocomplete: :off }, label: false, autocomplete: "off", placeholder: t("entries.index.description") | ||
= f.input :description, input_html: { data: { data: Tag.list }, autocomplete: :off }, input_html: {value: (@last_hour_logged.description if @last_hour_logged)}, label: false, autocomplete: "off", placeholder: t("entries.index.description") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using instance variables in partials views
Line is too long. [243/80]
= f.input :value, required: true, label: false, placeholder: t("entries.index.hours") | ||
= f.association :project, required: true, collection: Project.unarchived.by_name, label: false, placeholder: t("entries.index.project"), selected: (@last_hour_logged.project_id if @last_hour_logged) | ||
= f.association :category, required: true, collection: Category.by_name, label: false, placeholder: t("entries.index.category"), selected: (@last_hour_logged.category_id if @last_hour_logged) | ||
= f.input :value, required: true, label: false, placeholder: t("entries.index.hours"), input_html: {value: (@last_hour_logged.value if @last_hour_logged)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using instance variables in partials views
Line is too long. [156/80]
= f.association :category, required: true, collection: Category.by_name, label: false, placeholder: t("entries.index.category") | ||
= f.input :value, required: true, label: false, placeholder: t("entries.index.hours") | ||
= f.association :project, required: true, collection: Project.unarchived.by_name, label: false, placeholder: t("entries.index.project"), selected: (@last_hour_logged.project_id if @last_hour_logged) | ||
= f.association :category, required: true, collection: Category.by_name, label: false, placeholder: t("entries.index.category"), selected: (@last_hour_logged.category_id if @last_hour_logged) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using instance variables in partials views
Line is too long. [193/80]
= f.association :project, required: true, collection: Project.unarchived.by_name, label: false, placeholder: t("entries.index.project") | ||
= f.association :category, required: true, collection: Category.by_name, label: false, placeholder: t("entries.index.category") | ||
= f.input :value, required: true, label: false, placeholder: t("entries.index.hours") | ||
= f.association :project, required: true, collection: Project.unarchived.by_name, label: false, placeholder: t("entries.index.project"), selected: (@last_hour_logged.project_id if @last_hour_logged) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using instance variables in partials views
Line is too long. [200/80]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current approach would just break when the number of users start to grow...
app/controllers/hours_controller.rb
Outdated
@@ -21,6 +21,7 @@ def update | |||
def edit | |||
super | |||
resource | |||
@last_hour_logged = Hour.last |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A problem arises when multiple users add entries at the same time, you might end up with a form prefilled with your colleagues project. I think you might want to add a reference to the created entry on the redirect and use that to prefill the form....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @tarzan I will look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tarzan, concerning this task, instead of passing in the Hour.last or Mileage.last, I decided to pass in current_user.hours.last which is last hour entry by that user. So this ensures that the hours displayed is that of the current user when the user wants to fill up the entry or mileage form. Adding a reference to the created entry on the redirect may not give us the behaviour we want because that value is only available when a user creates the entry when the user signout and returns back later, the user may not see the values prefilled.
Those
this sound good?
5c1d35b
to
9bedf1c
Compare
Thanks for the feedback @tarzan. It is really good. But I came up with another idea. Instead of injecting the created record into the redirect, we could instead, get the last hour for the current user. We could pass in current_user.hours.last to prefilled the hours' form and current_user.mileages.last to prefilled the mileages form. With this approach, even when the user logged out and comes back after some time, the form would still be prefilled with the last saved entry. I hope that sounds good. |
@@ -21,6 +21,7 @@ def update | |||
def edit | |||
super | |||
resource | |||
@last_hour_logged = current_user.hours.last |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you think that this means that when a user selects a particular entry(hour) to edit, instead of prefilling the selected one, the last hour is selected, which might not be the same as what was selected?.
@@ -24,6 +24,7 @@ def update | |||
def edit | |||
super | |||
resource | |||
@last_mileage_logged = current_user.mileages.last |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/DefactoSoftware/Hours/pull/422/files#r276466822
Same as comment above 👆🏽
We've decided to freeze feature development as none of the maintainers currently have time to properly steward contributions from the community. |
PR Description
This PR ensures that entries are created quickly by persisting the last entry on the form.
Issue
#400