feat(milestones):add dates to milestones section #6020

shishiro26 · 2024-10-29T18:36:47Z

What this PR does

This PR addresses the issue of missing date information in the Milestones section on the Home tab. Previously, while the Milestones section displayed the week numbers derived from the Timeline data, it did not include the corresponding start and end dates for each milestone block.

Screenshots

Before:

After:

#279

ragesoss · 2024-10-29T18:44:51Z

Can you add some screenshots that show that the dates are the same in the Milestones vs the Timeline and This Week?

The tricky part of this particular issue is to guarantee that we're using the same algorithm to calculate the dates everywhere they are shown — ideally, only calculating them once, or if done multiple times, then using the same function with the same inputs.

shishiro26 · 2024-10-29T18:51:23Z

Can you add some screenshots that show that the dates are the same in the Milestones vs the Timeline and This Week?

Timeline:

Milestone:

The tricky part of this particular issue is to guarantee that we're using the same algorithm to calculate the dates everywhere they are shown — ideally, only calculating them once, or if done multiple times, then using the same function with the same inputs.

I am using the same DateCalculator that is used in the Timeline section.

ragesoss · 2024-10-29T19:03:09Z

app/assets/javascripts/components/overview/milestones.jsx

@@ -28,9 +30,11 @@ const Milestones = createReactClass({
    const weekNumberOffset = CourseDateUtils.weeksBeforeTimeline(this.props.course);
    const blocks = [];

-    this.props.allWeeks.map((week) => {
+    this.props.allWeeks.map((week,navIndex) => {


Where does this come from? In Timeline.jsx, it the result of some complex code in the render cycle for handling empty weeks.

The navIndex is generated by the map function, which helps identify unique elements in the array. Additionally, the DateCalculator requires the navIndex prop for its calculations. Upon cross-checking in the Timeline page, I confirmed that the navIndex is simply the index number generated by the map function.

ragesoss · 2024-10-29T19:03:23Z

You've shown the first week, but it's important to check for how it handles empty weeks as well.

ragesoss · 2024-10-29T19:29:28Z

Empty weeks can come at the beginning of a course, by including a gap between course start and timeline_start, and also in the middle of the course by using the calendar (via Edit Course Dates on the Timeline tab) to create weeks with no meeting days.

shishiro26 · 2024-10-29T19:35:30Z

You've shown the first week, but it's important to check for how it handles empty weeks as well.

In the Milestones section, empty weeks are not displayed

Milestones:

Weeks 159 and 164 are empty, and they are not displayed in the Milestones section.

Timeline:

I also tried using another course for this: University of Pennsylvania Medical Missionaries to Community Partners (Fall 2023).

ragesoss · 2024-10-29T19:53:52Z

cool. this looks like it's working properly. do you see a good way to avoid repeating the date calculations so that both the timeline and the milestones component can use the same value without a risk that they will diverge with later changes to the timeline component? i think it's dangerous to have these hard-to-understand parallel implementations.

shishiro26 · 2024-10-29T20:01:54Z

cool. this looks like it's working properly. do you see a good way to avoid repeating the date calculations so that both the timeline and the milestones component can use the same value without a risk that they will diverge with later changes to the timeline component? i think it's dangerous to have these hard-to-understand parallel implementations.

@ragesoss Yes, but it’s only two lines of code. I hope there won’t be any divergence in the future, as I believe it’s a straightforward implementation. If we implement a new way to eliminate redundancy in the code, we would still need to pass timelineStart, timelineEnd, and navIndex, which would make the code redundant again, right?

const dateCalc = new DateCalculator(this.props.timelineStart, this.props.timelineEnd, navIndex, {
    zeroIndexed: true
});

ragesoss · 2024-11-01T15:57:28Z

@shishiro26 I think the ideal strategy would be to do whatever data manipulation and calculation is necessary in a selector that operates on data from the redux store, so that the same selector (which can also memo-ize the result) is used in both cases. However, that likely requires a significant refactor of the Timeline component. I think I'm okay with this current implementation that you have done, but it should include some explanatory comments to note how this implementation is coupled to the code in the Timeline component and that we need to make sure both have the same results and account for empty weeks the same way.

…ates_to_milestones

shishiro26 · 2024-11-04T18:28:12Z

@ragesoss I have reviewed the codebase and confirmed that the date-related data is derived from the course or stored in the Redux store. In line with your previous suggestion, I plan to create a selector that extracts the course timelines and returns the start and end dates. This approach will help prevent discrepancies in date calculations and ensure that components such as Milestones and Timeline display consistent data. Centralizing date calculations within the selector will enhance uniformity across the application. Would you suggest any additional refactoring beyond this plan?

ragesoss · 2024-11-04T18:30:53Z

@shishiro26 I think it's wise to do the minimum amount of refactoring necessary to unify this particular calculation, and if you notice anything along the way that you think ought to be refactored, make a note of it.

shishiro26 · 2024-11-04T18:35:50Z

@ragesoss, I will ensure that I do the minimum necessary refactoring and update the commit accordingly. I will also make sure to note any important refactoring opportunities if they arise.

shishiro26 · 2024-11-07T17:25:13Z

@ragesoss I have updated the code. Could you please check if this aligns with what we discussed?

ragesoss · 2024-11-07T17:43:22Z

app/assets/javascripts/components/overview/milestones.jsx

@@ -4,6 +4,7 @@ import createReactClass from 'create-react-class';
 import { filter } from 'lodash-es';

 import CourseDateUtils from '../../utils/course_date_utils';
+import DateCalculator from '../../utils/date_calculator';


Is this still needed?

Nope,I forgot to remove the DateCalculator. I'll go ahead and remove it now, sorry about that.

ragesoss · 2024-11-07T17:46:39Z

app/assets/javascripts/components/timeline/timeline.jsx

@@ -388,16 +389,24 @@ const Timeline = createReactClass({
        navClassName += ' is-current';
      }

-      const dateCalc = new DateCalculator(this.props.course.timeline_start, this.props.course.timeline_end, navIndex, { zeroIndexed: true });


If this was the only use of DateCalculator, it no longer needs to be imported in the file.

ragesoss · 2024-11-07T17:48:17Z

app/assets/javascripts/components/overview/milestones.jsx

      const milestoneBlocks = filter(week.blocks, block => block.kind === this.milestoneBlockType);
      return milestoneBlocks.map((block) => {
+        if (


It looks we're doing something similar in both this component and the Timeline, checking for whether to map a particular block to null. Is this something that can become shared logic in the selector, or do they handle these cases differently?

Actually, the problem I encountered is that in the selector, I needed the index for the weeks to display, so I used weeks in the const getWeeks = state => state.timeline.weeks; function to memoize all the results. However, the issue is that the allWeekDates being sent from getAllWeekDates(state) contains weeks that are after the timeline end date, but the getWeeks function doesn't include them.

To handle this, when creating the DateCalculator instance:

const dateCalc = new DateCalculator(course.timeline_start, course.timeline_end, index, { zeroIndexed: true });

I found that the last indexes didn't have any dates, causing it to be empty when trying to render in the milestones and timeline. To fix this, I added a check for that in both files.

ragesoss · 2024-11-07T17:49:38Z

app/assets/javascripts/components/overview/milestones.jsx

+        if (
+          !this.props.weekDates[navIndex]?.start || 
+          !this.props.weekDates[navIndex]?.end || 
+          this.props.weekDates[navIndex]?.start > this.props.weekDates[navIndex]?.end


This is quite verbose, but it's not obvious what this if statement is for. What situation(s) for a block are being checked for here?

This check was added to prevent any date from showing after the timeline's end date in the milestone.

Hmm... I don't think that' necessary. The Timeline can handle dates that are after the end date (in which case, they get highlighted in red). I think in the Milestones component, it would be better to show dates that stretch beyond the end date, when that happens.

Okay, I will update the part where the deadline extends. Should I display the extended deadline within the milestone like in the below image, or should I show it after the timeline's end date?

I don't think any special formatting is needed in the Milestones component.

Yes, I have updated it. Could you please check it?

ragesoss · 2024-11-07T17:50:51Z

app/assets/javascripts/components/timeline/timeline.jsx

+
+      if (
+        !startDate || 
+        !endDate


Why were these checks not necessary in the current implementation?

ragesoss · 2024-11-08T19:44:12Z

Build is currently failing because of JS linting errors.

shishiro26 · 2024-11-08T19:47:24Z

I would like to do this since I have been using Docker. Could you provide me with the steps to run lint in Docker?

ragesoss · 2024-11-08T19:56:43Z

You're kind of on your own with that, as we don't have up-to-date docs for docker, and I don't use it.

…ates_to_milestones

shishiro26 · 2024-11-09T12:22:51Z

I have resolved all the linting issues, @ragesoss.

shishiro26 · 2024-11-11T18:12:26Z

@ragesoss I came across an issue while fixing the lint errors. I saw the line lint-non-build": "eslint 'test/**/.{jsx,js}' '.js'" and I’m not sure if this is the only line responsible for linting, as I couldn't find any other run functions in the codebase. Last time, when I encountered lint issues, I fixed everything explicitly. Could you guide me on what I need to do for this?

ragesoss · 2024-11-14T22:00:14Z

The failing tests seem to be related to this change.

…ates_to_milestones

ragesoss · 2024-11-19T18:09:59Z

Looks like the one failing test is related.

…to use allWeeksDates

shishiro26 · 2024-12-03T20:29:43Z

I hope these new changes work now, @ragesoss! 🤞🤞

ragesoss · 2024-12-04T18:04:28Z

app/assets/javascripts/components/timeline/timeline.jsx

@@ -397,7 +398,7 @@ const Timeline = createReactClass({
      if (usingCustomTitles) {
        let navTitle = '';
        if (weekInfo.emptyWeek) {
-          const datesStr = `${dateCalc.start()} - ${dateCalc.end()}`;
+          const datesStr = `${weekDates[navIndex].start} - ${weekDates[navIndex].end}`;


I note that his branch of code is being switched to use weekDates, but another branch below uses dateCalc. Is that the intended/correct behavior?

Thank you for pointing this out! I don't recall the exact branch in question, but this is the updated and correct code. The change to weekDates is intentional and reflects the latest update to the logic.

What I mean is, is it intention to switch from dateCalc to weekDates in only one branch, or should it be done in both?

It should be implemented in only one branch, not both.

The other branch is the older one, and I have been working on different changes there. In this branch, I’ve completely rewritten the code, while the other branch continues to focus on the previous changes or issues

I feel a little lost here. Why can't the calls to dateCalc in the else branch below also be switched to use weekDates?

Apologies for any confusion on my end. I currently have two branches in development: one focused on resolving the current issue, and the other working on issue #651. The second branch is entirely separate, and I need to make some changes to it. As a result, I've converted that branch into a draft and have been focusing solely on the first branch for now. Are we on the same page, or is there still some misunderstanding on my part? This is why I've added the weekDates to this branch and not the other branch .

Oh! Sorry, I meant logical branch of the code, as in the if/else statement that starts at line 399. This PR changes the if portion but does not change the else portion, and I think it should probably change both.

Thanks for the clarification! I’ve updated both the if and else parts as suggested and pushed the changes. Let me know if anything else is needed! 😅👍

…ates_to_milestones

ragesoss · 2024-12-10T19:46:22Z

app/assets/javascripts/components/course/course.jsx

@@ -27,6 +27,7 @@ import { getStudentCount, getCurrentUser, getWeeksArray } from '../../selectors'
 import ActivityHandler from '../activity/activity_handler';
 import CourseApproval from './course_approval';

+


Please remove this.

ragesoss · 2024-12-10T19:47:24Z

app/assets/javascripts/selectors/index.js

@@ -1,11 +1,28 @@
 import { createSelector } from 'reselect';
-import { sortBy, difference, uniq, filter, includes, pickBy, find } from 'lodash-es';
+import {


I'd prefer not to include all these formatting changes alongside the new feature.

ragesoss · 2024-12-10T19:48:21Z

app/assets/javascripts/selectors/index.js

+  [getAllWeeksArray, getCourse],
+  (weeksArray, course) => {
+    return weeksArray.map((_, index) => {
+      const DateCalc = new DateCalculator(


By convention, this DateCalc object should start with a lower-case letter.

ragesoss · 2024-12-10T19:51:01Z

I am happy with this implementation, and I'm ready to merge it after cleanup of the minor issues in my latest comments.

shishiro26 · 2024-12-11T13:23:08Z

@ragesoss, I have fixed all the formatting changes. This should work now! 😃😃

…ates_to_milestones

feat(milestones):add dates to milestones section

473d579

ragesoss reviewed Oct 29, 2024

View reviewed changes

Merge branch 'WikiEducationFoundation:master' into feature/#279_add_d…

e1815fe

…ates_to_milestones

Refactored code to use the selector and removed unnecessary coupling

e0e689e

ragesoss reviewed Nov 7, 2024

View reviewed changes

shishiro26 added 2 commits November 7, 2024 23:56

Removed the unnecessary 'DateCalculator' import

e4795b2

updated the dates which are extending the end date

a4f6784

shishiro26 and others added 2 commits November 9, 2024 02:03

Merge branch 'WikiEducationFoundation:master' into feature/#279_add_d…

96043ea

…ates_to_milestones

fixed all the linting errors

2efce7d

shishiro26 and others added 2 commits November 15, 2024 23:13

Merge branch 'WikiEducationFoundation:master' into feature/#279_add_d…

e0693bd

…ates_to_milestones

fixed the ci errors

bb4f8b3

shishiro26 marked this pull request as draft November 18, 2024 05:06

refactor: remove unused weekDates prop and update related components …

f44f44e

…to use allWeeksDates

shishiro26 marked this pull request as ready for review December 3, 2024 20:28

ragesoss reviewed Dec 4, 2024

View reviewed changes

shishiro26 and others added 2 commits December 6, 2024 00:52

added the weekDates in the else block

2572a9b

Merge branch 'WikiEducationFoundation:master' into feature/#279_add_d…

034bfde

…ates_to_milestones

ragesoss reviewed Dec 10, 2024

View reviewed changes

fixed all the formatting changes

e644c96

Merge branch 'WikiEducationFoundation:master' into feature/#279_add_d…

9174b5c

…ates_to_milestones

ragesoss merged commit def64a2 into WikiEducationFoundation:master Dec 13, 2024
1 check passed

shishiro26 deleted the feature/#279_add_dates_to_milestones branch December 14, 2024 12:55

		@@ -27,6 +27,7 @@ import { getStudentCount, getCurrentUser, getWeeksArray } from '../../selectors'
		import ActivityHandler from '../activity/activity_handler';
		import CourseApproval from './course_approval';

feat(milestones):add dates to milestones section #6020

feat(milestones):add dates to milestones section #6020

Conversation

shishiro26 commented Oct 29, 2024 • edited Loading

What this PR does

Screenshots

ragesoss commented Oct 29, 2024

shishiro26 commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ragesoss commented Oct 29, 2024

ragesoss commented Oct 29, 2024

shishiro26 commented Oct 29, 2024 • edited Loading

ragesoss commented Oct 29, 2024

shishiro26 commented Oct 29, 2024 • edited Loading

ragesoss commented Nov 1, 2024

shishiro26 commented Nov 4, 2024

ragesoss commented Nov 4, 2024

shishiro26 commented Nov 4, 2024

shishiro26 commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ragesoss commented Nov 8, 2024

shishiro26 commented Nov 8, 2024

ragesoss commented Nov 8, 2024

shishiro26 commented Nov 9, 2024

shishiro26 commented Nov 11, 2024

ragesoss commented Nov 14, 2024

ragesoss commented Nov 19, 2024

shishiro26 commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ragesoss commented Dec 10, 2024

shishiro26 commented Dec 11, 2024

shishiro26 commented Oct 29, 2024 •

edited

Loading

shishiro26 commented Oct 29, 2024 •

edited

Loading

shishiro26 commented Oct 29, 2024 •

edited

Loading