Skip to content

Draft: Factor out computed data from Roadmap Vuex state

euko requested to merge extract-computed-data-from-roadmap-vuex-state into master

Why the MR?

1. Roadmap components and vuex store contain redundant logic

Take a look at the computed property startDate (or endDate) in epic_item.vue#L75 and milestone_item.vue:

startDate() {
  if (this.epic.startDateOutOfRange) {
    return this.epic.originalStartDate;
  }

  return this.epic.startDate;
},

The above computed property startDate really represents a proxy start date. For example, even if an epic doesn't have a fixed start date, we need to use an arbitrary date to render a timeline bar for the epic.

However, we already calculate and store the proxy start date when we fetch the data from the backend in the Vuex store actions.js#L88. So the logic inside startDate actually resolves to the following:

startDate() {
  if (!this.epic.startDateUndefined && !this.epic.startDateOutOfRange) {
    return this.epic.originalStartDate
  }

  if (!this.epic.startDateUndefined) {
    if (!this.epic.startDateOutOfRange)) {
      return this.epic.originalStartDate;
    } else {
      return newDate(timeframeStart);
    }
  } else {
    return newDate(timeframeStart);
  }
},

Note that originalStartDate, startDateOutOfRange and startDateUndefined are NOT states. They are computed states and we modify the startDate field of the fetched data to store its proxy version in the state.

2. Storing the client-computed data in the vuex state is requiring unnecessary refetching logic

Because we're storing computed data in the state, we need to manually re-calculate them every time we need to re-fetch(refresh) the data using these vuex actions:

3. Storing the client-computed data in the vuex state is also breaking encapsulation and leaking lower-level details.

Only few components actually need proxy start/end dates for rendering timeline bars but modifying and storing the fetched data in place and in the vuex store means ALL of the Roadmap components (and their specs + mock data) need to be aware of the proxy date logic.

What does this MR do?

  • Avoids storing computed data in the vuex store (so we cache the data from the backend intact in the vuex state)
  • Uses computed properties for proxy start/end dates in common_mixins.js and clearly labels them as renderStartDate and renderEndDate.
  • Removes refreshEpicDates and refreshMilestoneDates vuex actions.
  • Fix specs.

Possible follow-ups:

  • Rename endDate to dueDate because epics and milestones have "due dates" and "dueDate" is the name of the field stored in the database.

Screenshots (strongly suggested)

No functional/visual change.

Roadmap-related recent MRs

merge request description
!48654 (merged) Remove unused skeleton loader, css and fix mock data for roadmap specs
!49433 (merged) Update the existing mixin specs for roadmap
!49530 (merged) Refactor hasStartDate* methods to accept an argument timeframeItem rather than using this.timeframeItem computed prop.
!50158 (merged) Move getter methods into roadmap_item_utils.js
!50652 (merged) Render EpicItemTimeline only once every row in Roadmap
!50673 (merged) Refactor epic sort comparator for roadmap

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by euko

Merge request reports