fix:issue with meeting dates extending beyond the ending date.

[shishiro26]

NOTE: Please review the pull request process before opening your first PR: https://github.com/WikiEducationFoundation/WikiEduDashboard/blob/master/CONTRIBUTING.md#pull-request-process

What this PR does

This pull request addresses an issue where meeting dates extend beyond the specified ending date.

Screenshots

Before:

After:

Open questions and concerns

I have a question regarding the timeline dates. For testing purposes, I used this course: University of Pennsylvania - Medical Missionaries to Community Partners (Fall 2023). I adjusted the start and end dates, which can be seen in the before and after screenshots.

In the current state, the start date appears to extend beyond the end date at week 10. Is this behavior intentional, or does it indicate a bug? If it is indeed an issue, should I create a new issue and work on resolving it?

#651

[ragesoss]

This is going to be very hard to interpret, with the regex in particular. If a calculation like this is necessary, it should be extracted to well-named method with comments to explain the intended behavior.

[shishiro26]

@ragesoss I have updated the regex in the well-named component and added proper comments to enhance the intended behaviour

[ragesoss]

Thanks!

It is indeed a bug that the week end dates for all of the weeks starting at week 10 are equal to the course or timeline end date, as shown in the right-hand week nav component.

[ragesoss]

I wonder if it makes more sense to do this somewhere else, such as the weekMeetings function in course_date_utils.js?

I don't like the idea of extracting the date from a string, when we've already generated this data structure by starting from dates and converting them into strings.

[shishiro26]

@ragesoss Actually, the issue is that the function responsible for converting the date to a string doesn't currently take the weeks (which contain the start and end dates for each week) as props. Because of this, I need to perform this check outside the date-to-string conversion function. I'll try to implement this check in the weekMeetings function within course_date_utils.js instead of writing it seperately .

[ragesoss]

Are you still working on this change?

[shishiro26]

Yuppp..

[shishiro26]

Before:

After:

@ragesoss, could you please review this PR?

[ragesoss]

@shishiro26 it's not necessary to merge updates from master unless merge conflicts with your PR have developed.

[shishiro26]

Apologies for that. Sometimes, I’m unable to run the RSpec tests on my local machine, so I was trying to merge here and check for any errors.

[shishiro26]

@ragesoss, it’s been a while, and I apologize for the delay. I've been debugging, but I couldn't identify why the test cases are failing. However, I have some doubts that I need clarification on.

I added some logs to check the values in the test cases. Below is the course_overview_spec.rb file:

# frozen_string_literal: true

require 'rails_helper'

describe 'course overview page', type: :feature, js: true do
  let(:slug)         { 'This_university.foo/This.course_(term_2015)' }
  let(:course_start) { '2015-02-11'.to_date }
  let(:course_end)   { course_start + 6.months }
  let(:course) do
    create(:course,
           id: 10001,
           title: 'This.course',
           slug:,
           start: course_start.to_date,
           end: course_end.to_date,
           timeline_start: course_start.to_date,
           timeline_end: course_end.to_date,
           school: 'This university.foo',
           term: 'term 2015',
           description: 'This is a great course',
           weekdays: '1001001')
  end
  let(:campaign) { create(:campaign) }
  let!(:campaigns_course) do
    create(:campaigns_course, campaign_id: campaign.id, course_id: course.id)
  end
  let(:week) { create(:week, course_id: course.id) }
  let(:content) { 'Edit Wikipedia' }
  let!(:block)  { create(:block, week_id: week.id, content:) }
  let(:admin)   { create(:admin) }

  before do
    stub_token_request
    login_as(admin, scope: :user)
  end

  context 'when course has started' do
    before do
      visit "/courses/#{course.slug}"
      sleep 1
    end

    it 'displays week activity for this week' do
      find '.course__this-week' do
        expect(page).to have_content 'This Week'
      end
    end
  end

  context 'when course starts in future' do
    let(:timeline_start) { '2025-02-11'.to_date + 2.weeks }

    before do
      course.update(timeline_start:)
      course.reload
      visit "/courses/#{course.slug}"
      sleep 1

      puts "DEBUG: course.start = #{course.start}"
      puts "DEBUG: course.end = #{course.end}"
      puts "DEBUG: course.timeline_start = #{course.timeline_start}"
      puts "DEBUG: course.timeline_end = #{course.timeline_end}"

      puts "DEBUG: timeline_start = #{timeline_start}"
      puts "DEBUG: Expected first week's start = #{timeline_start.beginning_of_week(:sunday)}"
      puts "DEBUG: Expected first week's end = #{timeline_start.end_of_week(:sunday)}"

      (0..5).each do |i|
        week_start = timeline_start + (i * 7).days
        week_end = week_start.end_of_week(:sunday)
        puts "DEBUG: Week #{i + 1}: Start = #{week_start.beginning_of_week(:sunday).strftime('%m/%d')}, End = #{week_end.strftime('%m/%d')}"
      end
    end

    it 'displays week activity for the first week' do
      save_and_open_page

      within '.course__this-week' do
        expect(page).to have_content('First Active Week')
        puts "DEBUG: Found 'First Active Week' in .course__this-week"
        expect(page).to have_content content
        puts "DEBUG: Found content = #{content} in .course__this-week"
      end

      within '.week-range' do
        puts "DEBUG: Timeline start is #{timeline_start}"

        puts "DEBUG: Calculating expected start of the week."
        expected_week_start = timeline_start.beginning_of_week(:sunday).strftime('%m/%d')

        puts "DEBUG: Calculated expected start of the week: #{expected_week_start}"

        puts "DEBUG: Checking if page contains the start date #{expected_week_start} in '.week-range' section."
        expect(page).to have_content(expected_week_start)

        puts "DEBUG: Found week range with start date #{expected_week_start} in the page."
      end

      within '.margin-bottom' do
        expect(page).to have_content('Meetings: Wednesday (02/26), Saturday (03/01)')
        puts "DEBUG: Found meetings content in .margin-bottom"
      end

      within '.week-index' do
        expect(page).to have_content(/Week \d+/)
        puts "DEBUG: Found week index in .week-index"
      end
    end
  end
end

Test Command:

For both before and after, I ran the same test using the following command:

bundle exec rspec ./spec/features/course_overview_spec.rb

Before:

In the logs before running the test, everything looked fine, and the output showed "Week 1023 (09/10 - 08/12)" as the timeline start and end dates.

Screenshots before the test:

After:

After running the test, the output showed "Week 525 (02/23 - 08/12)" with a different timeline start, even though the timeline_start was unchanged.

Screenshots after the test:

Issue:

The logs before and after seem identical in terms of timeline start values, yet the output differs. Before, it shows "Week 525 (02/23 - 08/12)," and after it shows "Week 1023 (09/10 - 08/12)." The timeline_start is the same, but the weeks and their corresponding dates are completely different.

Can you help clarify why this is occurring?

[ragesoss]

The week numbers in those screenshots are very high (and different). Maybe that has something to do with it?

In both cases the assignment start is outside of the start/end range of the course, which is clearly a bug; normally it shouldn't be possible to have an assignment start (aka timeline start) that is before the start or after the end of the course.

[shishiro26]

The week numbers in those screenshots are very high (and different). Maybe that has something to do with it?

I'm not entirely sure how the week numbers are assigned here, as normally they correspond to the index number and the "weeks before the timeline" in the code. However, there's no index number in this case, so I'm not sure how to proceed.

In both cases, the assignment start is outside the course's start/end range, which is definitely a bug. Normally, it shouldn't be possible for the assignment start (i.e., the timeline start) to be before the course start or after the course end.

However, in the test, it checks whether the course starts in the future, The line let(:timeline_start) { '2025-02-11'.to_date + 2.weeks } might be causing the timeline_start to fall outside the course's start and end date range.

[ragesoss]

I think that timeline_start line that you identified is probably the right place to start. It probably makes sense to explicitly set appropriate start/end dates along with that timeline_start date, for that specific test. (It should also be further in the future; I guess 2025 seemed a long way away when that was last edited 8 years ago.)

[shishiro26]

These new changes ensure that if the meetings extend beyond the course's end date, they are not displayed.

Before:

After:

[ragesoss]

Nice, this looks like it does what we want!