-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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 bug recurring events #1712
Fix bug recurring events #1712
Conversation
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersWhen your PR has been assigned reviewers contact them to get your code reviewed and approved via:
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
This is a Daily recurring event follow up PR to the Weekly calendar PR merged earlier today. Please take a look: |
@palisadoes |
Please review. This is a fix for the daily recurring calendar entry that was never implemented |
Yes, we'll need verification. |
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.
The code is functioning, but it certainly is not scalable. Even if a user enters a date too far into the future by mistake, this will blow up our database with events that we wouldn't be using anytime soon. We need to work on a dynamic strategy that only generates these events as and when required. You need to research how other large calendar based products like Google Calender
, Taskade
etc. store and query the same efficiently.
You certainly don't need to add all the functionality in one PR, but you need to first devise a strategy that accounts for all the future and edge use cases. Then we can work on incorporating each functionality in a step by step manner. |
Thank you for the thorough review and insightful suggestions. Your points on scalability and potential database issues were very valid. After conducting further research, I discovered that Google Calendar employs a similar approach by limiting event creation to Can we follow the same? By restricting event creation to a reasonable timeframe, we can mitigate the risk of database overload and ensure a more scalable solution. |
Can you share the link for the same? |
|
Don't stick to the link I shared. There are many other blog posts about the same. Please try to read as many as you can and come up with a strategy for efficient querying. Please also share all the links that guided your design when sharing, so that even we can vet them once. |
@palisadoes |
I see. Either we should revert it, or we can let it be dormant. When this discussion is completed, we can update it's code too. The ideal solution once achieved should allow for all levels of flexibility in recuraance patterns without much additional configuration. |
@EshaanAgg please have a look at this once |
We can follow various approaches for managing events:
In that case , updating a single occurrence within the recurring series becomes more challenging because MongoDB operates on the document level
I believe for this scenario, a denormalized structure is better for storing each occurrence separately. One approach could be to reduce constraints, such as allowing the creation of events for only a 10-year period. what your views on it ? |
Regarding the recurrence limits:
I'm not a professional coder, so this should be taken into account for my next suggestion. I'd like to hear your feedback.
This was the approach recommended here: You could also do a representation of a recurring event as a start date with an instance count tracker to save space. But that would be compute intensive when people scroll through calendars and the API has to generate recurring events in real time while simultaneously presenting non-recurring events. In the issue discussion thread we thought it would be easier to make all events recurring, even one-time events (0 repetitions), as it would:
The editing process would similarly need a historical tracker for the same reasons I mentioned previously. |
Absolutely, I agree with your thoughts on the denormalized approach for managing recurring events. The idea of incorporating a sequence count limitation, especially for daily events over a 5-10 year period, seems like a sensible enhancement.I should have applied this approach previously but I didn't know about database constraints. Treating every event as recurring not only simplifies the logic but also streamlines the codebase. Overall I think it can be a good call! |
After conducting in-depth analysis and research, I have an approach to use a denormalized structure with minimal impact on the database, particularly for MongoDB, which operates at the document level. This strategy is crucial as it involves managing and tracking edits for each event. Approach:
By implementing this approach, we effectively limit the number of events stored, ensuring efficient data storage and query performance Articles: https://scullwm.medium.com/design-and-manage-recurring-events-fb43676e711a I have also come across another method mentioned in the article that involves the exclusion of a particular event. However, this approach presents a challenge as it prevents us from tracking all events. https://news.ycombinator.com/item?id=18477975 I would appreciate your input on this matter so that we can move forward and successfully implement this feature within the given time frame. |
@Community-Programmer @EshaanAgg The RFP (https://www.ietf.org/rfc/rfc2445.txt) referenced in the Medium article has the basics of what we were talking about in creating a recurring instance tracker.
Recurring events have an RRULE entry inserted in the event and it looks like they use a linked list format too, as I was thinking. With a sequence of multiple recurring rules each with a specified start and end date. The RRULE also allows you specify days of the week etc. I even found this NPM package. Most of our work may be solved. Just use the schema it recommends. I vote for the RFP approach. It's what iCal is based on. |
Ok sounds good so we are going to store |
|
I think it will somehow help us to create recurring events logic and will give us recurrance dates. |
In this case, couldn't you replace the existing document with a new one that points back to the original recurring instance? |
Do we need to add exclusions for the day that is edited or updated, and should it point to the parent document? We can certainly do that, but using Here are some considerations:
Implementing this will take some time, and we will need to make changes to several functions. There will be complex queries, and a majority of functionalities all over the project will be affected and need adjustments accordingly. |
dynamically generating the events that correspond to the same recurring event with identical data seems to be the better option an instance of a recurring event that has been modified by the end user(an exception that has deviated from the identical data present in other instances corresponding to the same recurring event) should be created and stored seperately in another table more info in this article:- https://vertabelo.com/blog/again-and-again-managing-recurring-events-in-a-data-model/ |
@xoldyckk @EshaanAgg @palisadoes I am totally confused about which method to follow for recurring events creation, |
I agree with @EshaanAgg and @xoldyckk. If we dynamically generate and insert events beyond some global LIMIT, then we reduce DB bloat. It is also unlikely that there will be calendar view overload because the first person who views the event beyond the LIMIT will create the event, thereby relieving the need to do further computation by others viewing the same event. The extra coding to do this shouldn't be excessive as RRULE will be doing most of the work. Seeing the updated schema would assist. |
@Community-Programmer, @xoldyckk, @EshaanAgg This is my understanding.
|
We can't do a pure generation on demand as the events in our series will often be uniquely modified. Possible cases include:
With this method, future generated events would copy all the characteristics of the most recently generated event, ie. the event at the LIMIT date. |
@Community-Programmer is this clearer now? |
@palisadoes @xoldyckk @EshaanAgg This is the conclusion I have drawn from the conversation. |
|
No, 'not on demand' means that, on event edit, only the event that is edited will be created and stored as a skipped event. This implies that we will not be generating any events until they are edited |
Can you please explain the flow? I believe there might be some conflicts in our understanding of the solution. What exactly are you thinking? |
|
yaa exactly skipped events are events that should not be in the RRULE series as they have been edited or modified so they will be stored as exclusion in the database and refer to the parent_event. The flow is very simple I am talking about below approach |
This is the link we should reference: From the discussion:
This means that the RRULE object would need to include an event object to set the initial parameters to use for the series. This would need to be reflected in your schema. If a user decides to change any parameter in the RRULE string from a point in time onward, how would that work? It would seem to be like this, which is validated by my Google Calendar tests
Google Calendar (Test: 1):
Deleting all the weekly events, doesn't delete the daily or monthly. They are handled as completely different recurrences. Google Calendar (Test: 2):
Google Calendar (Test: 3):
It appears Google only creates a new recurrence instance when the recurrence definition changes. This looks fairly straightforward to implement. Please post the updated schema. |
Have you seen this thread? You'll need to ensure that your solution works with the community version of MongoDB. If this is the case, why did your tests pass for the previous PR? |
As discussed earlier, the solution we're trying to implement at starting requires database replication for functioning of transactions. The error you encountered with the standalone instance is due to the absence of replication. In the previous pull request, the tests passed because we were not starting a session. Transactions were not initiated, which is why the issue didn't arise. |
The PR cannot be reverted it's too late. So you'll have to update those files in this PR |
OK, let's proceed as we now have a way to minimize the data footprint while optimizing CPU load. |
Adding for cross referencing |
OK, I will make all the changes in this pull request only |
@palisadoes @xoldyckk @EshaanAgg OK, so I am proceeding now with my first steps:
Afterwards, I will focus on editing events and adding an exception for the edited and modified event |
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1712 +/- ##
===========================================
+ Coverage 98.17% 98.38% +0.20%
===========================================
Files 184 203 +19
Lines 10767 12473 +1706
Branches 835 1018 +183
===========================================
+ Hits 10571 12272 +1701
+ Misses 186 171 -15
- Partials 10 30 +20 ☔ View full report in Codecov by Sentry. |
e6f92a8
to
663a08b
Compare
Sorry,the force push was an unintended action, that resulted in the automatic closure of the pull request (PR). I understand the inconvenience and disruption this may have caused. Thank you for your understanding |
What kind of change does this PR introduce?
Fixed the bug on creating recurring event only single event is created
Issue Number:
Fixes #1583
Did you add tests for your changes?
yes
Snapshots/Videos:
2024-01-17.23-14-33.mp4
If relevant, did you update the documentation?
No
Summary
Task-2
Implementation Details:
Validation:
No other functionalities is affected by changes made in the createEvent mutation
All test files passed
Does this PR introduce a breaking change?
Yes
Have you read the contributing guide?
Yes