Merge branch '30355-use-hours-only-for-time-tracking' into 'master'

Limit time tracking units to hours

Closes #30355

See merge request gitlab-org/gitlab-ce!29469

app/assets/javascripts/boards/components/board_sidebar.js

@@ -38,6 +38,7 @@ export default Vue.extend({
       issue: {},
       list: {},
       loadingAssignees: false,
+      timeTrackingLimitToHours: boardsStore.timeTracking.limitToHours,
     };
   },
   computed: {

app/assets/javascripts/boards/components/issue_time_estimate.vue

@@ -2,6 +2,7 @@
 import { GlTooltip } from '@gitlab/ui';
 import Icon from '~/vue_shared/components/icon.vue';
 import { parseSeconds, stringifyTime } from '~/lib/utils/datetime_utility';
+import boardsStore from '../stores/boards_store';
 
 export default {
   components: {
@@ -14,12 +15,17 @@ export default {
       required: true,
     },
   },
+  data() {
+    return {
+      limitToHours: boardsStore.timeTracking.limitToHours,
+    };
+  },
   computed: {
     title() {
-      return stringifyTime(parseSeconds(this.estimate), true);
+      return stringifyTime(parseSeconds(this.estimate, { limitToHours: this.limitToHours }), true);
     },
     timeEstimate() {
-      return stringifyTime(parseSeconds(this.estimate));
+      return stringifyTime(parseSeconds(this.estimate, { limitToHours: this.limitToHours }));
     },
   },
 };

app/assets/javascripts/boards/index.js

@@ -49,6 +49,7 @@ export default () => {
   }
 
   boardsStore.create();
+  boardsStore.setTimeTrackingLimitToHours($boardApp.dataset.timeTrackingLimitToHours);
 
   issueBoardsApp = new Vue({
     el: $boardApp,

app/assets/javascripts/boards/stores/boards_store.js

@@ -12,6 +12,9 @@ import eventHub from '../eventhub';
 
 const boardsStore = {
   disabled: false,
+  timeTracking: {
+    limitToHours: false,
+  },
   scopedLabels: {
     helpLink: '',
     enabled: false,
@@ -222,6 +225,10 @@ const boardsStore = {
   setIssueDetail(issueDetail) {
     this.detail.issue = issueDetail;
   },
+
+  setTimeTrackingLimitToHours(limitToHours) {
+    this.timeTracking.limitToHours = parseBoolean(limitToHours);
+  },
 };
 
 BoardsStoreEE.initEESpecific(boardsStore);

app/assets/javascripts/lib/utils/datetime_utility.js

@@ -479,9 +479,13 @@ export const pikadayToString = date => {
  * Seconds can be negative or positive, zero or non-zero. Can be configured for any day
  * or week length.
  */
-export const parseSeconds = (seconds, { daysPerWeek = 5, hoursPerDay = 8 } = {}) => {
+export const parseSeconds = (
+  seconds,
+  { daysPerWeek = 5, hoursPerDay = 8, limitToHours = false } = {},
+) => {
   const DAYS_PER_WEEK = daysPerWeek;
   const HOURS_PER_DAY = hoursPerDay;
+  const SECONDS_PER_MINUTE = 60;
   const MINUTES_PER_HOUR = 60;
   const MINUTES_PER_WEEK = DAYS_PER_WEEK * HOURS_PER_DAY * MINUTES_PER_HOUR;
   const MINUTES_PER_DAY = HOURS_PER_DAY * MINUTES_PER_HOUR;
@@ -493,9 +497,18 @@ export const parseSeconds = (seconds, { daysPerWeek = 5, hoursPerDay = 8 } = {})
     minutes: 1,
   };
 
-  let unorderedMinutes = Math.abs(seconds / MINUTES_PER_HOUR);
+  if (limitToHours) {
+    timePeriodConstraints.weeks = 0;
+    timePeriodConstraints.days = 0;
+  }
+
+  let unorderedMinutes = Math.abs(seconds / SECONDS_PER_MINUTE);
 
   return _.mapObject(timePeriodConstraints, minutesPerPeriod => {
+    if (minutesPerPeriod === 0) {
+      return 0;
+    }
+
     const periodCount = Math.floor(unorderedMinutes / minutesPerPeriod);
 
     unorderedMinutes -= periodCount * minutesPerPeriod;

app/assets/javascripts/sidebar/components/time_tracking/comparison_pane.vue

@@ -28,11 +28,16 @@ export default {
       type: String,
       required: true,
     },
+    limitToHours: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   computed: {
     parsedTimeRemaining() {
       const diffSeconds = this.timeEstimate - this.timeSpent;
-      return parseSeconds(diffSeconds);
+      return parseSeconds(diffSeconds, { limitToHours: this.limitToHours });
     },
     timeRemainingHumanReadable() {
       return stringifyTime(this.parsedTimeRemaining);

app/assets/javascripts/sidebar/components/time_tracking/sidebar_time_tracking.vue

@@ -53,6 +53,7 @@ export default {
       :time-spent="store.totalTimeSpent"
       :human-time-estimate="store.humanTimeEstimate"
       :human-time-spent="store.humanTotalTimeSpent"
+      :limit-to-hours="store.timeTrackingLimitToHours"
       :root-path="store.rootPath"
     />
   </div>

app/assets/javascripts/sidebar/components/time_tracking/time_tracker.vue

@@ -37,6 +37,10 @@ export default {
       required: false,
       default: '',
     },
+    limitToHours: {
+      type: Boolean,
+      default: false,
+    },
     rootPath: {
       type: String,
       required: true,
@@ -129,6 +133,7 @@ export default {
         :time-spent="timeSpent"
         :time-spent-human-readable="humanTimeSpent"
         :time-estimate-human-readable="humanTimeEstimate"
+        :limit-to-hours="limitToHours"
       />
       <transition name="help-state-toggle">
         <time-tracking-help-state v-if="showHelpState" :root-path="rootPath" />

app/assets/javascripts/sidebar/mount_milestone_sidebar.js

 import Vue from 'vue';
 import timeTracker from './components/time_tracking/time_tracker.vue';
+import { parseBoolean } from '~/lib/utils/common_utils';
 
 export default class SidebarMilestone {
   constructor() {
@@ -7,7 +8,7 @@ export default class SidebarMilestone {
 
     if (!el) return;
 
-    const { timeEstimate, timeSpent, humanTimeEstimate, humanTimeSpent } = el.dataset;
+    const { timeEstimate, timeSpent, humanTimeEstimate, humanTimeSpent, limitToHours } = el.dataset;
 
     // eslint-disable-next-line no-new
     new Vue({
@@ -22,6 +23,7 @@ export default class SidebarMilestone {
             timeSpent: parseInt(timeSpent, 10),
             humanTimeEstimate,
             humanTimeSpent,
+            limitToHours: parseBoolean(limitToHours),
             rootPath: '/',
           },
         }),

app/assets/javascripts/sidebar/stores/sidebar_store.js

@@ -8,7 +8,7 @@ export default class SidebarStore {
   }
 
   initSingleton(options) {
-    const { currentUser, rootPath, editable } = options;
+    const { currentUser, rootPath, editable, timeTrackingLimitToHours } = options;
     this.currentUser = currentUser;
     this.rootPath = rootPath;
     this.editable = editable;
@@ -16,6 +16,7 @@ export default class SidebarStore {
     this.totalTimeSpent = 0;
     this.humanTimeEstimate = '';
     this.humanTimeSpent = '';
+    this.timeTrackingLimitToHours = timeTrackingLimitToHours;
     this.assignees = [];
     this.isFetching = {
       assignees: true,

app/helpers/application_settings_helper.rb

@@ -253,6 +253,7 @@ module ApplicationSettingsHelper
       :throttle_unauthenticated_enabled,
       :throttle_unauthenticated_period_in_seconds,
       :throttle_unauthenticated_requests_per_period,
+      :time_tracking_limit_to_hours,
       :two_factor_grace_period,
       :unique_ips_limit_enabled,
       :unique_ips_limit_per_user,

app/helpers/boards_helper.rb

@@ -14,7 +14,8 @@ module BoardsHelper
       issue_link_base: build_issue_link_base,
       root_path: root_path,
       bulk_update_path: @bulk_issues_path,
-      default_avatar: image_path(default_avatar)
+      default_avatar: image_path(default_avatar),
+      time_tracking_limit_to_hours: Gitlab::CurrentSettings.time_tracking_limit_to_hours.to_s
     }
   end
 

app/helpers/issuables_helper.rb

@@ -430,7 +430,8 @@ module IssuablesHelper
       editable: issuable.dig(:current_user, :can_edit),
       currentUser: issuable[:current_user],
       rootPath: root_path,
-      fullPath: issuable[:project_full_path]
+      fullPath: issuable[:project_full_path],
+      timeTrackingLimitToHours: Gitlab::CurrentSettings.time_tracking_limit_to_hours
     }
   end
 

app/models/application_setting_implementation.rb

@@ -82,6 +82,7 @@ module ApplicationSettingImplementation
         throttle_unauthenticated_enabled: false,
         throttle_unauthenticated_period_in_seconds: 3600,
         throttle_unauthenticated_requests_per_period: 3600,
+        time_tracking_limit_to_hours: false,
         two_factor_grace_period: 48,
         unique_ips_limit_enabled: false,
         unique_ips_limit_per_user: 10,

app/views/admin/application_settings/_localization.html.haml

@@ -8,4 +8,11 @@
       .form-text.text-muted
         = _('Default first day of the week in calendars and date pickers.')
 
+    .form-group
+      = f.label :time_tracking, _('Time tracking'), class: 'label-bold'
+      .form-check
+        = f.check_box :time_tracking_limit_to_hours, class: 'form-check-input'
+        = f.label :time_tracking_limit_to_hours, class: 'form-check-label' do
+          = _('Limit display of time tracking units to hours.')
+
   = f.submit _('Save changes'), class: "btn btn-success"

app/views/shared/boards/components/sidebar/_time_tracker.html.haml

@@ -3,4 +3,5 @@
     ":time-spent" => "issue.timeSpent || 0",
     ":human-time-estimate" => "issue.humanTimeEstimate",
     ":human-time-spent" => "issue.humanTimeSpent",
+    ":limit-to-hours" => "timeTrackingLimitToHours",
     "root-path" => "#{root_url}" }

app/views/shared/milestones/_sidebar.html.haml

@@ -93,7 +93,11 @@
               = milestone.issues_visible_to_user(current_user).closed.count
 
     .block
-      #issuable-time-tracker{ data: { time_estimate: @milestone.total_issue_time_estimate, time_spent: @milestone.total_issue_time_spent, human_time_estimate: @milestone.human_total_issue_time_estimate, human_time_spent: @milestone.human_total_issue_time_spent } }
+      #issuable-time-tracker{ data: { time_estimate: @milestone.total_issue_time_estimate,
+        time_spent: @milestone.total_issue_time_spent,
+        human_time_estimate: @milestone.human_total_issue_time_estimate,
+        human_time_spent: @milestone.human_total_issue_time_spent,
+        limit_to_hours: Gitlab::CurrentSettings.time_tracking_limit_to_hours.to_s } }
         // Fallback while content is loading
         .title.hide-collapsed
           = _('Time tracking')

changelogs/unreleased/30355-use-hours-only-for-time-tracking.yml

+---
+title: Add option to limit time tracking units to hours
+merge_request: 29469
+author: Jon Kolb
+type: added

db/migrate/20190611090827_add_time_tracking_limit_to_hours_to_application_settings.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddTimeTrackingLimitToHoursToApplicationSettings < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default :application_settings, :time_tracking_limit_to_hours, :boolean, default: false, allow_null: false
+  end
+
+  def down
+    remove_column :application_settings, :time_tracking_limit_to_hours
+  end
+end

db/schema.rb

@@ -229,6 +229,7 @@ ActiveRecord::Schema.define(version: 20190620112608) do
     t.integer "custom_project_templates_group_id"
     t.boolean "elasticsearch_limit_indexing", default: false, null: false
     t.string "geo_node_allowed_ips", default: "0.0.0.0/0, ::/0"
+    t.boolean "time_tracking_limit_to_hours", default: false, null: false
     t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id", using: :btree
     t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id", using: :btree
     t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id", using: :btree

doc/api/settings.md

@@ -231,6 +231,7 @@ are listed in the descriptions of the relevant settings.
 | `throttle_unauthenticated_enabled`       | boolean          | no                                   | (**If enabled, requires:** `throttle_unauthenticated_period_in_seconds` and `throttle_unauthenticated_requests_per_period`) Enable unauthenticated request rate limit. Helps reduce request volume (e.g. from crawlers or abusive bots). |
 | `throttle_unauthenticated_period_in_seconds` | integer      | required by: `throttle_unauthenticated_enabled` | Rate limit period in seconds.  |
 | `throttle_unauthenticated_requests_per_period` | integer    | required by: `throttle_unauthenticated_enabled` | Max requests per period per IP. |
+| `time_tracking_limit_to_hours`           | boolean          | no                                   | Limit display of time tracking units to hours. Default is `false`. |
 | `two_factor_grace_period`                | integer          | required by: `require_two_factor_authentication` | Amount of time (in hours) that users are allowed to skip forced configuration of two-factor authentication. |
 | `unique_ips_limit_enabled`               | boolean          | no                                   | (**If enabled, requires:** `unique_ips_limit_per_user` and `unique_ips_limit_time_window`) Limit sign in from multiple ips. |
 | `unique_ips_limit_per_user`              | integer          | required by: `unique_ips_limit_enabled` | Maximum number of ips per user. |

doc/workflow/time_tracking.md

@@ -73,7 +73,15 @@ The following time units are available:
 
 Default conversion rates are 1mo = 4w, 1w = 5d and 1d = 8h.
 
-Other interesting links:
+### Limit displayed units to hours
+
+> Introduced in GitLab 12.0.
+
+The display of time units can be limited to hours through the option in **Admin Area > Settings > Preferences** under 'Localization'.
+
+With this option enabled, `75h` is displayed instead of `1w 4d 3h`.
+
+## Other interesting links
 
 - [Time Tracking landing page on about.gitlab.com](https://about.gitlab.com/solutions/time-tracking/)
 

lib/gitlab/time_tracking_formatter.rb

@@ -6,7 +6,7 @@ module Gitlab
 
     def parse(string)
       with_custom_config do
-        string.sub!(/\A-/, '')
+        string = string.sub(/\A-/, '')
 
         seconds = ChronicDuration.parse(string, default_unit: 'hours') rescue nil
         seconds *= -1 if seconds && Regexp.last_match
@@ -16,10 +16,12 @@ module Gitlab
 
     def output(seconds)
       with_custom_config do
-        ChronicDuration.output(seconds, format: :short, limit_to_hours: false, weeks: true) rescue nil
+        ChronicDuration.output(seconds, format: :short, limit_to_hours: limit_to_hours_setting, weeks: true) rescue nil
       end
     end
 
+    private
+
     def with_custom_config
       # We may want to configure it through project settings in a future version.
       ChronicDuration.hours_per_day = 8
@@ -32,5 +34,9 @@ module Gitlab
 
       result
     end
+
+    def limit_to_hours_setting
+      Gitlab::CurrentSettings.time_tracking_limit_to_hours
+    end
   end
 end

locale/gitlab.pot

@@ -5857,6 +5857,9 @@ msgstr ""
 msgid "Let's Encrypt is a free, automated, and open certificate authority (CA) that gives digital certificates in order to enable HTTPS (SSL/TLS) for websites. Learn more about Let's Encrypt configuration by following the %{docs_link_start}documentation on GitLab Pages%{docs_link_end}."
 msgstr ""
 
+msgid "Limit display of time tracking units to hours."
+msgstr ""
+
 msgid "Limited to showing %d event at most"
 msgid_plural "Limited to showing %d events at most"
 msgstr[0] ""

spec/features/boards/sidebar_spec.rb

@@ -18,6 +18,8 @@ describe 'Issue Boards', :js do
   let!(:list)        { create(:list, board: board, label: development, position: 0) }
   let(:card)         { find('.board:nth-child(2)').first('.board-card') }
 
+  let(:application_settings) { {} }
+
   around do |example|
     Timecop.freeze { example.run }
   end
@@ -27,6 +29,8 @@ describe 'Issue Boards', :js do
 
     sign_in(user)
 
+    stub_application_setting(application_settings)
+
     visit project_board_path(project, board)
     wait_for_requests
   end
@@ -223,16 +227,24 @@ describe 'Issue Boards', :js do
   end
 
   context 'time tracking' do
+    let(:compare_meter_tooltip) { find('.time-tracking .time-tracking-content .compare-meter')['data-original-title'] }
+
     before do
       issue2.timelogs.create(time_spent: 14400, user: user)
-      issue2.update!(time_estimate: 28800)
+      issue2.update!(time_estimate: 128800)
+
+      click_card(card)
     end
 
     it 'shows time tracking progress bar' do
-      click_card(card)
+      expect(compare_meter_tooltip).to eq('Time remaining: 3d 7h 46m')
+    end
 
-      page.within('.time-tracking') do
-        expect(find('.time-tracking-content .compare-meter')['data-original-title']).to eq('Time remaining: 4h')
+    context 'when time_tracking_limit_to_hours is true' do
+      let(:application_settings) { { time_tracking_limit_to_hours: true } }
+
+      it 'shows time tracking progress bar' do
+        expect(compare_meter_tooltip).to eq('Time remaining: 31h 46m')
       end
     end
   end

spec/frontend/lib/utils/datetime_utility_spec.js

@@ -334,6 +334,12 @@ describe('prettyTime methods', () => {
       assertTimeUnits(aboveOneDay, 33, 2, 2, 0);
       assertTimeUnits(aboveOneWeek, 26, 0, 1, 9);
     });
+
+    it('should correctly parse values when limitedToHours is true', () => {
+      const twoDays = datetimeUtility.parseSeconds(173000, { limitToHours: true });
+
+      assertTimeUnits(twoDays, 3, 48, 0, 0);
+    });
   });
 
   describe('stringifyTime', () => {

spec/javascripts/boards/boards_store_spec.js

@@ -355,4 +355,14 @@ describe('Store', () => {
       expect(boardsStore.moving.list).toEqual(dummyList);
     });
   });
+
+  describe('setTimeTrackingLimitToHours', () => {
+    it('sets the timeTracking.LimitToHours option', () => {
+      boardsStore.timeTracking.limitToHours = false;
+
+      boardsStore.setTimeTrackingLimitToHours('true');
+
+      expect(boardsStore.timeTracking.limitToHours).toEqual(true);
+    });
+  });
 });

spec/javascripts/boards/components/issue_time_estimate_spec.js

 import Vue from 'vue';
 import IssueTimeEstimate from '~/boards/components/issue_time_estimate.vue';
+import boardsStore from '~/boards/stores/boards_store';
 import mountComponent from '../../helpers/vue_mount_component_helper';
 
-describe('Issue Tine Estimate component', () => {
+describe('Issue Time Estimate component', () => {
   let vm;
 
   beforeEach(() => {
-    const Component = Vue.extend(IssueTimeEstimate);
-    vm = mountComponent(Component, {
-      estimate: 374460,
-    });
+    boardsStore.create();
   });
 
   afterEach(() => {
     vm.$destroy();
   });
 
+  describe('when limitToHours is false', () => {
+    beforeEach(() => {
+      boardsStore.timeTracking.limitToHours = false;
+
+      const Component = Vue.extend(IssueTimeEstimate);
+      vm = mountComponent(Component, {
+        estimate: 374460,
+      });
+    });
+
     it('renders the correct time estimate', () => {
       expect(vm.$el.querySelector('time').textContent.trim()).toEqual('2w 3d 1m');
     });
@@ -37,4 +45,26 @@ describe('Issue Tine Estimate component', () => {
         done();
       });
     });
+  });
+
+  describe('when limitToHours is true', () => {
+    beforeEach(() => {
+      boardsStore.timeTracking.limitToHours = true;
+
+      const Component = Vue.extend(IssueTimeEstimate);
+      vm = mountComponent(Component, {
+        estimate: 374460,
+      });
+    });
+
+    it('renders the correct time estimate', () => {
+      expect(vm.$el.querySelector('time').textContent.trim()).toEqual('104h 1m');
+    });
+
+    it('renders expanded time estimate in tooltip', () => {
+      expect(vm.$el.querySelector('.js-issue-time-estimate').textContent).toContain(
+        '104 hours 1 minute',
+      );
+    });
+  });
 });

spec/javascripts/sidebar/components/time_tracking/time_tracker_spec.js

@@ -13,6 +13,7 @@ describe('Issuable Time Tracker', () => {
     timeSpent,
     timeEstimateHumanReadable,
     timeSpentHumanReadable,
+    limitToHours,
   }) => {
     setFixtures(`
     <div>
@@ -25,6 +26,7 @@ describe('Issuable Time Tracker', () => {
       timeSpent,
       humanTimeEstimate: timeEstimateHumanReadable,
       humanTimeSpent: timeSpentHumanReadable,
+      limitToHours: Boolean(limitToHours),
       rootPath: '/',
     };
 
@@ -128,6 +130,29 @@ describe('Issuable Time Tracker', () => {
         });
       });
 
+      describe('Comparison pane when limitToHours is true', () => {
+        beforeEach(() => {
+          initTimeTrackingComponent({
+            timeEstimate: 100000, // 1d 3h
+            timeSpent: 5000, // 1h 23m
+            timeEstimateHumanReadable: '',
+            timeSpentHumanReadable: '',
+            limitToHours: true,
+          });
+        });
+
+        it('should show the correct tooltip text', done => {
+          Vue.nextTick(() => {
+            expect(vm.showComparisonState).toBe(true);
+            const $title = vm.$el.querySelector('.time-tracking-content .compare-meter').dataset
+              .originalTitle;
+
+            expect($title).toBe('Time remaining: 26h 23m');
+            done();
+          });
+        });
+      });
+
       describe('Estimate only pane', () => {
         beforeEach(() => {
           initTimeTrackingComponent({

spec/lib/gitlab/time_tracking_formatter_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Gitlab::TimeTrackingFormatter do
+  describe '#parse' do
+    subject { described_class.parse(duration_string) }
+
+    context 'positive durations' do
+      let(:duration_string) { '3h 20m' }
+
+      it { expect(subject).to eq(12_000) }
+    end
+
+    context 'negative durations' do
+      let(:duration_string) { '-3h 20m' }
+
+      it { expect(subject).to eq(-12_000) }
+    end
+  end
+
+  describe '#output' do
+    let(:num_seconds) { 178_800 }
+
+    subject { described_class.output(num_seconds) }
+
+    context 'time_tracking_limit_to_hours setting is true' do
+      before do
+        stub_application_setting(time_tracking_limit_to_hours: true)
+      end
+
+      it { expect(subject).to eq('49h 40m') }
+    end
+
+    context 'time_tracking_limit_to_hours setting is false' do
+      before do
+        stub_application_setting(time_tracking_limit_to_hours: false)
+      end
+
+      it { expect(subject).to eq('1w 1d 1h 40m') }
+    end
+  end
+end

spec/services/system_note_service_spec.rb

@@ -946,6 +946,18 @@ describe SystemNoteService do
 
         expect(subject.note).to eq "changed time estimate to 1w 4d 5h"
       end
+
+      context 'when time_tracking_limit_to_hours setting is true' do
+        before do
+          stub_application_setting(time_tracking_limit_to_hours: true)
+        end
+
+        it 'sets the note text' do
+          noteable.update_attribute(:time_estimate, 277200)
+
+          expect(subject.note).to eq "changed time estimate to 77h"
+        end
+      end
     end
 
     context 'without a time estimate' do
@@ -1022,6 +1034,18 @@ describe SystemNoteService do
       end
     end
 
+    context 'when time_tracking_limit_to_hours setting is true' do
+      before do
+        stub_application_setting(time_tracking_limit_to_hours: true)
+      end
+
+      it 'sets the note text' do
+        spend_time!(277200)
+
+        expect(subject.note).to eq "added 77h of time spent"
+      end
+    end
+
     def spend_time!(seconds)
       noteable.spend_time(duration: seconds, user_id: author.id)
       noteable.save!