Removed deduplication logic. See #4249

[zehata]

Context

Resolves #4249

Implementation

If I am not mistaken, the previous deduplication logic is no longer needed if we are going to be accounting for classNo within the logic like this:

-				combinationKey := slot.LessonType + "|" + slot.Day + "|" + slot.StartTime + "|" + buildingName + "|" + slot.WeeksString
+				combinationKey := slot.LessonType + "|" + slot.classNo + "|" + slot.Day + "|" + slot.StartTime + "|" + buildingName + "|" + slot.WeeksString

[vercel]

@zehata is attempting to deploy a commit to the modsbot's projects Team on Vercel.

A member of the Team first needs to authorize it.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.14%. Comparing base (988c6fd) to head (d87cbcc).
⚠️ Report is 147 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4250      +/-   ##
==========================================
+ Coverage   54.52%   55.14%   +0.61%     
==========================================
  Files         274      297      +23     
  Lines        6076     6724     +648     
  Branches     1455     1617     +162     
==========================================
+ Hits         3313     3708     +395     
- Misses       2763     3016     +253     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[thejus03]

@zehata Thanks for finding this bug!

However, I believe that the solution was not to include classNo. For that section of code, I was trying to merge all slots with the lessonType, Day, StartTime and building name to reduce the search space. If we did include classNo, then we would effectively never merge anything because we would simply never encounter another grouping with the same classNo because they are all unique.

I believe the problem that I oversaw was that each grouping (eg G07) has 3 slots right? I was previously merging them as individual entities without looking at them as a whole grouping. So the solution was to find a better way to create combinationKey for the WHOLE grouping

I will send my PR and please take a look and see if you agree!

[zehata]

Let me know of a potential alternative to my proposed change, since mine will incur quite the cost.

As you said, adding the classNo would be effectively removing the logic, and so the PR I proposed was removing the logic for combining. But of course my change would be negatively impacting performance.

You may want to take a look at #4225 as well, since I actually found this bug while diagnosing an issue whereby some lessons are missing from the resultant share link, and I don't want us to spend too much time on a solution that would not work in conjunction with #4225.

[thejus03]

Let me know of a potential alternative to my proposed change, since mine will incur quite the cost.

As you said, adding the classNo would be effectively removing the logic, and so the PR I proposed was removing the logic for combining. But of course my change would be negatively impacting performance.

Yea for an alternative solution you can refer to #4251. I think it works but please try and test it out, if you have time.

You may want to take a look at #4225 as well, since I actually found this bug while diagnosing an issue whereby some lessons are missing from the resultant share link, and I don't want us to spend too much time on a solution that would not work in conjunction with #4225.

Ah I see. Just to clarify the missing lessons in the resultant share link are for the partial timetables only right? So something similar to #4145?

[zehata]

@thejus03 I am closing this as it's superseded by 4251. let's move any further discussion to #4251