fix repeater for simple timestamps

changelog.org

@@ -10,6 +10,13 @@ When there are updates to the changelog, you will be notified and see a 'gift' i
    - When going to the Agenda view, the selected tab is persisted - meaning it will be pre-selected when you go to the Agenda next time.
      - Relevant PR: https://github.com/200ok-ch/organice/pull/562
 
+** Fixed
+   - Having an active timestamp with a repeater was broken.
+     - When the TODO state changes for a header that has a repeater (either as SCHEDULED, DEADLINE or active timestamp), a log entry is written and the timestamp is updated.
+     - Relevant PR: https://github.com/200ok-ch/organice/pull/568
+   - Removing an active timestamp was broken.
+     - Relevant PR: https://github.com/200ok-ch/organice/pull/568
+
 * [2020-11-20 Fri]
 ** Fixed
    - organice understands =:PROPERTIES:= drawers and smartly parses the values in case one of the values is a timestamp.

src/actions/org.js

@@ -531,6 +531,13 @@ export const removePlanningItem = (headerId, planningItemIndex) => ({
   dirtying: true,
 });
 
+export const removeTimestamp = (headerId, timestampId) => ({
+  type: 'REMOVE_TIMESTAMP',
+  headerId,
+  timestampId,
+  dirtying: true,
+});
+
 export const updatePropertyListItems = (headerId, newPropertyListItems) => ({
   type: 'UPDATE_PROPERTY_LIST_ITEMS',
   headerId,

src/components/OrgFile/components/HeaderContent/index.js

@@ -152,7 +152,10 @@ class HeaderContent extends PureComponent {
   }
 
   handleTimestampClick(timestampId) {
-    this.props.base.activatePopup('timestamp-editor', { timestampId });
+    this.props.base.activatePopup('timestamp-editor', {
+      timestampId,
+      headerId: this.props.header.get('id'),
+    });
   }
 
   handleLogEntryTimestampClick(headerId) {

src/components/OrgFile/components/TimestampEditorModal/components/TimestampEditor/index.js

@@ -39,13 +39,14 @@ class TimestampEditor extends PureComponent {
     this.props.onChange(this.props.timestamp.update('isActive', (isActive) => !isActive));
   }
 
-  handleDateChange(event, planningItemIndex) {
+  handleDateChange(event, planningItemIndex, timestampId) {
     // The user deleted the timestamp
     if (_.isEmpty(event.target.value)) {
       // It's a planning item and the parser knows which one.
-      // TODO: Also delete timestamps which are not planningItems
       if (_.isNumber(planningItemIndex)) {
-        this.props.org.removePlanningItem(this.props.selectedHeaderId, planningItemIndex);
+        this.props.org.removePlanningItem(this.props.headerId, planningItemIndex);
+      } else if (_.isNumber(timestampId)) {
+        this.props.org.removeTimestamp(this.props.headerId, timestampId);
       }
       this.props.onClose();
     } else {
@@ -285,7 +286,7 @@ class TimestampEditor extends PureComponent {
   }
 
   render() {
-    const { timestamp, planningItemIndex } = this.props;
+    const { timestamp, timestampId, planningItemIndex } = this.props;
     const {
       isActive,
       year,
@@ -296,7 +297,6 @@ class TimestampEditor extends PureComponent {
       endHour,
       endMinute,
     } = timestamp.toJS();
-
     return (
       <div>
         <div className="timestamp-editor__render">{renderAsText(timestamp)}</div>
@@ -316,7 +316,7 @@ class TimestampEditor extends PureComponent {
                 data-testid="timestamp-selector"
                 type="date"
                 className="timestamp-editor__date-input"
-                onChange={(event) => this.handleDateChange(event, planningItemIndex)}
+                onChange={(event) => this.handleDateChange(event, planningItemIndex, timestampId)}
                 // Needed for iOS due to React bug
                 // https://github.com/facebook/react/issues/8938#issuecomment-519074141
                 onFocus={(event) => (event.nativeEvent.target.defaultValue = '')}
@@ -336,15 +336,8 @@ class TimestampEditor extends PureComponent {
   }
 }
 
-const mapStateToProps = (state) => {
-  const selectedHeaderId = state.org.present.get('selectedHeaderId');
-  return {
-    selectedHeaderId,
-  };
-};
-
 const mapDispatchToProps = (dispatch) => ({
   org: bindActionCreators(orgActions, dispatch),
 });
 
-export default connect(mapStateToProps, mapDispatchToProps)(TimestampEditor);
+export default connect(null, mapDispatchToProps)(TimestampEditor);

src/components/OrgFile/components/TimestampEditorModal/index.js

@@ -42,14 +42,23 @@ export default class TimestampEditorModal extends PureComponent {
   }
 
   render() {
-    const { timestamp, onClose, singleTimestampOnly, planningItemIndex } = this.props;
+    const {
+      headerId,
+      timestamp,
+      timestampId,
+      onClose,
+      singleTimestampOnly,
+      planningItemIndex,
+    } = this.props;
 
     return (
       <Drawer onClose={onClose}>
         <h2 className="timestamp-editor__title">Edit timestamp</h2>
 
         <TimestampEditor
+          headerId={headerId}
           timestamp={timestamp.get('firstTimestamp')}
+          timestampId={timestampId}
           planningItemIndex={planningItemIndex}
           onClose={onClose}
           onChange={this.handleChange('firstTimestamp')}
@@ -65,7 +74,9 @@ export default class TimestampEditorModal extends PureComponent {
               </div>
 
               <TimestampEditor
+                headerId={headerId}
                 timestamp={timestamp.get('secondTimestamp')}
+                timestampId={timestampId}
                 onChange={this.handleChange('secondTimestamp')}
               />
 

src/components/OrgFile/components/TitleLine/index.js

@@ -154,7 +154,10 @@ class TitleLine extends PureComponent {
   }
 
   handleTimestampClick(timestampId) {
-    this.props.base.activatePopup('timestamp-editor', { timestampId });
+    this.props.base.activatePopup('timestamp-editor', {
+      headerId: this.props.header.get('id'),
+      timestampId,
+    });
   }
 
   handleInsertTimestamp(event) {

src/components/OrgFile/index.js

@@ -335,7 +335,9 @@ class OrgFile extends PureComponent {
 
         return (
           <TimestampEditorModal
+            headerId={activePopupData.get('headerId')}
             timestamp={editingTimestamp}
+            timestampId={activePopupData.get('timestampId')}
             planningItemIndex={activePopupData.get('planningItemIndex')}
             singleTimestampOnly={!activePopupData.get('timestampId')}
             onClose={this.handlePopupClose}

src/lib/parse_org.js

@@ -521,7 +521,7 @@ export const _parsePlanningItems = (rawText) => {
           _.range(planningTypeIndex + 1, planningTypeIndex + 1 + 13)
         );
 
-        return createTimestamp({ type, timestamp });
+        return createOrUpdateTimestamp({ type, timestamp });
       })
       .filter((item) => !!item)
   );
@@ -535,7 +535,8 @@ export const _parsePlanningItems = (rawText) => {
   }
 };
 
-const createTimestamp = ({ type, timestamp }) => fromJS({ type, timestamp, id: generateId() });
+const createOrUpdateTimestamp = ({ type, timestamp, id }) =>
+  fromJS({ type, timestamp, id: id || generateId() });
 
 const parsePropertyList = (rawText) => {
   const lines = rawText.split('\n');
@@ -878,7 +879,9 @@ const extractActiveTimestampsForPlanningItemsFromParse = (type, parsedData) => {
   // planningItems only accept a single timestamp -> ignore second timestamp
   return parsedData
     .filter((x) => x.get('type') === 'timestamp' && x.getIn(['firstTimestamp', 'isActive']))
-    .map((x) => createTimestamp({ type: type, timestamp: x.get('firstTimestamp') }));
+    .map((x) =>
+      createOrUpdateTimestamp({ type: type, timestamp: x.get('firstTimestamp'), id: x.get('id') })
+    );
 };
 
 // Merge planningItems from parsed title, description, and planning keywords.

src/reducers/org.js

@@ -999,6 +999,33 @@ const removePlanningItem = (state, action) => {
   return state.removeIn(['headers', headerIndex, 'planningItems', planningItemIndex]);
 };
 
+const removeTimestamp = (state, action) => {
+  const { path } = pathAndPartOfTimestampItemWithIdInHeaders(
+    state.get('headers'),
+    action.timestampId
+  );
+
+  // remove parsed timestamp
+  state = state.removeIn(['headers', ...path]);
+
+  // in case this is an active timestamp, remove it from planning items.
+  state = state.updateIn(['headers', path[0], 'planningItems'], (planningItems) =>
+    planningItems.filter((item) => item.get('id') !== action.timestampId)
+  );
+
+  // rebuild text representation of header
+  state = state.setIn(
+    ['headers', path[0], 'titleLine', 'rawTitle'],
+    attributedStringToRawText(state.getIn(['headers', path[0], 'titleLine', 'title']))
+  );
+  state = state.setIn(
+    ['headers', path[0], 'rawDescription'],
+    attributedStringToRawText(state.getIn(['headers', path[0], 'description']))
+  );
+
+  return state;
+};
+
 export const updatePropertyListItems = (state, action) => {
   const headerIndex = indexOfHeaderWithId(state.get('headers'), action.headerId);
 
@@ -1252,6 +1279,8 @@ const reducer = (state, action) => {
       return addNewPlanningItem(state, action);
     case 'REMOVE_PLANNING_ITEM':
       return removePlanningItem(state, action);
+    case 'REMOVE_TIMESTAMP':
+      return removeTimestamp(state, action);
     case 'UPDATE_PROPERTY_LIST_ITEMS':
       return updatePropertyListItems(state, action);
     case 'SET_ORG_FILE_ERROR_MESSAGE':
@@ -1371,10 +1400,58 @@ function updatePlanningItemsWithRepeaters({
   timestamp,
 }) {
   indexedPlanningItemsWithRepeaters.forEach(([planningItem, planningItemIndex]) => {
+    const adjustedTimestamp = applyRepeater(planningItem.get('timestamp'), timestamp);
     state = state.setIn(
       ['headers', headerIndex, 'planningItems', planningItemIndex, 'timestamp'],
-      applyRepeater(planningItem.get('timestamp'), timestamp)
+      adjustedTimestamp
     );
+
+    // INFO: Active timestamps are now manually updated in place.
+    // Rationale: The active timestamps in title and description are
+    // added to `planningItems` on parse. Since there can be an
+    // arbitrary amount of timestamps it makes sense not to have one
+    // `planningItem` representing the title or the description. We
+    // need to preserve the place of a timestamp in title/description
+    // and we want to have it in a list of `planningItems`. So they
+    // necessarily exist in more than one place. There might be a
+    // cleaner solution where we store the timestamp only in one place
+    // and use references to that place but I don't see any extra
+    // benefit for what would be no negligible refactoring effort.
+
+    // Scheduled / deadline timestamps on the other hand are part of
+    // `rawDescription` but not of the parsed description. These
+    // timestamps only exist in one place (`planningItems`) so
+    // changing them there is visible and will be persisted.
+    switch (planningItem.get('type')) {
+      case 'TIMESTAMP_TITLE':
+        const titleIndex = state
+          .getIn(['headers', headerIndex, 'titleLine', 'title'])
+          .findIndex((titlePart) => planningItem.get('id') === titlePart.get('id'));
+        state = state.setIn(
+          ['headers', headerIndex, 'titleLine', 'title', titleIndex, 'firstTimestamp'],
+          adjustedTimestamp
+        );
+        state = state.setIn(
+          ['headers', headerIndex, 'titleLine', 'rawTitle'],
+          attributedStringToRawText(state.getIn(['headers', headerIndex, 'titleLine', 'title']))
+        );
+        break;
+      case 'TIMESTAMP_DESCRIPTION':
+        const descriptionIndex = state
+          .getIn(['headers', headerIndex, 'description'])
+          .findIndex((descriptionPart) => planningItem.get('id') === descriptionPart.get('id'));
+        state = state.setIn(
+          ['headers', headerIndex, 'description', descriptionIndex, 'firstTimestamp'],
+          adjustedTimestamp
+        );
+        state = state.setIn(
+          ['headers', headerIndex, 'rawDescription'],
+          attributedStringToRawText(state.getIn(['headers', headerIndex, 'description']))
+        );
+        break;
+      default:
+        break;
+    }
   });
   state = state.setIn(
     ['headers', headerIndex, 'titleLine', 'todoKeyword'],

src/reducers/org.unit.test.js

@@ -762,20 +762,23 @@ describe('org reducer', () => {
     let todoHeaderId;
     let doneHeaderId;
     let repeatingHeaderId;
+    let activeTimestampWithRepeaterHeaderId;
     let state;
     const testOrgFile = readFixture('various_todos');
 
     beforeEach(() => {
       state = readInitialState();
       state.org.present = parseOrg(testOrgFile);
-      // "This is done" is the 1st header,
-      // "Header with repeater" is the 2nd,
-      // "This is not a todo" is 3rd item, and
-      // "Repeating task" is 4th item; we count from 1.
+      // "This is done" is the 1st header
+      // "Header with repeater" is the 2nd header
+      // "This is not a todo" is 3rd header
+      // "Active timestamp task with repeater" is 4th header
+      // "Repeating task" is 5th header
       doneHeaderId = state.org.present.get('headers').get(0).get('id');
       todoHeaderId = state.org.present.get('headers').get(1).get('id');
       regularHeaderId = state.org.present.get('headers').get(2).get('id');
-      repeatingHeaderId = state.org.present.get('headers').get(3).get('id');
+      activeTimestampWithRepeaterHeaderId = state.org.present.get('headers').get(3).get('id');
+      repeatingHeaderId = state.org.present.get('headers').get(4).get('id');
     });
 
     function check_todo_keyword_kept(oldHeaders, newHeaders, headerId) {
@@ -840,7 +843,6 @@ describe('org reducer', () => {
       expect(headerWithId(newHeaders, repeatingHeaderId).get('description').size).toEqual(
         headerWithId(oldHeaders, repeatingHeaderId).get('description').size
       );
-
       expect(headerWithId(newHeaders, repeatingHeaderId).get('logNotes').size).toBeGreaterThan(
         headerWithId(oldHeaders, repeatingHeaderId).get('logNotes').size
       );
@@ -872,6 +874,47 @@ describe('org reducer', () => {
       expect(extractTitlesAndNestings(intermHeaders)).toEqual(extractTitlesAndNestings(newHeaders));
     });
 
+    it('should advance active timestamp with repeater in header', () => {
+      const oldHeaders = state.org.present.get('headers');
+      const newHeaders = reducer(
+        state.org.present,
+        types.advanceTodoState(activeTimestampWithRepeaterHeaderId)
+      ).get('headers');
+      check_todo_keyword_kept(oldHeaders, newHeaders, activeTimestampWithRepeaterHeaderId);
+
+      expect(
+        headerWithId(newHeaders, activeTimestampWithRepeaterHeaderId).get('planningItems')
+      ).not.toEqual(
+        headerWithId(oldHeaders, activeTimestampWithRepeaterHeaderId).get('planningItems')
+      );
+
+      // The active timestamp with repeater get's replaced in place
+      expect(
+        headerWithId(oldHeaders, activeTimestampWithRepeaterHeaderId).getIn([
+          'titleLine',
+          'rawTitle',
+        ])
+      ).toMatch(/<2020-11-15 Sun \+1d>/);
+      expect(
+        headerWithId(oldHeaders, activeTimestampWithRepeaterHeaderId).getIn([
+          'titleLine',
+          'rawTitle',
+        ])
+      ).not.toMatch(/<2020-11-16 Mon \+1d>/);
+      expect(
+        headerWithId(newHeaders, activeTimestampWithRepeaterHeaderId).getIn([
+          'titleLine',
+          'rawTitle',
+        ])
+      ).not.toMatch(/<2020-11-15 Sun \+1d>/);
+      expect(
+        headerWithId(newHeaders, activeTimestampWithRepeaterHeaderId).getIn([
+          'titleLine',
+          'rawTitle',
+        ])
+      ).toMatch(/<2020-11-16 Mon \+1d>/);
+    });
+
     it('should just dirty when applied to no header', () => {
       check_just_dirtying(state.org.present, types.advanceTodoState(undefined));
     });

test_helpers/fixtures/various_todos.org

@@ -1,5 +1,6 @@
 * DONE This is done
 * TODO Header with repeater
 * This is not a todo
+* TODO Task with active timestamp and repeater <2020-11-15 Sun +1d>
 * TODO Repeating task
   SCHEDULED: <2019-11-27 Wed +1d>