-
Notifications
You must be signed in to change notification settings - Fork 652
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix:issue with meeting dates extending beyond the ending date. #6028
Merged
ragesoss
merged 13 commits into
WikiEducationFoundation:master
from
shishiro26:bug/#651_frontend_meetings_display_after_the_end_date
Dec 31, 2024
Merged
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
3235699
Fix: Improve meeting date extraction and validation in Week component
shishiro26 a88dde3
Merge branch 'WikiEducationFoundation:master' into bug/#651_frontend_…
shishiro26 15bdf2a
Refactored the regex into a named-component
shishiro26 68d6f40
changed the date extraction to the course_date_utils.js
shishiro26 c77f248
Merge branch 'WikiEducationFoundation:master' into bug/#651_frontend_…
shishiro26 f81e51b
fixed the linting errors
shishiro26 b49dbce
Merge branch 'WikiEducationFoundation:master' into bug/#651_frontend_…
shishiro26 e5df4ec
Fix issue with meeting display after the end date
shishiro26 4541a23
Merge branch 'WikiEducationFoundation:master' into bug/#651_frontend_…
shishiro26 4248440
Merge branch 'WikiEducationFoundation:master' into bug/#651_frontend_…
shishiro26 bab231a
Merge branch 'WikiEducationFoundation:master' into bug/#651_frontend_…
shishiro26 453d53c
fix rspec test
shishiro26 fbff657
fix rspec tests and check if meetings go beyond timeline.end
shishiro26 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
class ExtractDate { | ||
/** | ||
* Class to extract a meeting date from a specified date format. | ||
* | ||
* Date format expected: "Day (MM/DD)" where MM is the month and DD is the day. | ||
* | ||
* @param {string} date - The input date string from which to extract the date. | ||
*/ | ||
constructor(date) { | ||
this.date = date; | ||
} | ||
|
||
/** | ||
* Extracts the date in MM/DD format from the stored date string. | ||
* | ||
* The method uses a regular expression to find the date within parentheses. | ||
* The expected format is (MM/DD), with possible whitespace around the date. | ||
* | ||
* @returns {string|null} - Returns the extracted date in MM/DD format (without parentheses) if found, | ||
* or null if no valid date is present. | ||
*/ | ||
getDate() { | ||
// Use a regular expression to match the date format (MM/DD). | ||
// The regex breakdown: | ||
// - `\(`: Matches the literal opening parenthesis '('. | ||
// - `\s*`: Matches any whitespace characters (spaces or tabs) zero or more times. | ||
// - `(\d{1,2}\/\d{1,2})`: Capturing group that matches: | ||
// - `\d{1,2}`: Exactly one or two digits (for MM). | ||
// - `\/`: Matches the literal '/' character. | ||
// - `\d{1,2}`: Exactly one or two digits (for DD). | ||
// - `\s*`: Matches any whitespace characters zero or more times. | ||
// - `\)`: Matches the literal closing parenthesis ')'. | ||
const match = this.date.trim().match(/\(\s*(\d{1,2}\/\d{1,2})\s*\)/); | ||
|
||
// Check if a match was found and return the first capturing group (MM/DD). | ||
// If no match is found, return null. | ||
return match && match[1] ? match[1] : null; | ||
} | ||
} | ||
|
||
export default ExtractDate; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you still working on this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yuppp..