Skip to content
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 Recurring Events #1583

Closed
palisadoes opened this issue Dec 27, 2023 · 74 comments
Closed

Fix Recurring Events #1583

palisadoes opened this issue Dec 27, 2023 · 74 comments
Assignees
Labels
bug Something isn't working question Further information is requested

Comments

@palisadoes
Copy link
Contributor

palisadoes commented Dec 27, 2023


Note

This is a very important feature to implement.

  1. We need someone with either demonstrable:
    1. experience with GraphQL
    2. activity in their GitHub account with a focus on TypeScript
  2. This is not an issue for a first time attempt at the technology.
    1. The first PR needs to be created within a week
    2. All PRs must be completed by the end of February
  3. Read through the comments in the closed PR about the progress and why the approach was selected. There are further details below.

In summary:

  1. This is not for a newbie
  2. This is not for someone with limited time over the next month

Describe the bug
The creation of a recurring event only creates a single event in the API clients.

To Reproduce
Steps to reproduce the behavior:

  1. Create a recurring event
  2. Only a single instance is visible

Expected behavior

  1. You'll need to update or create new mutations that include start and end date for the event recurrence, not just the event itself
  2. There will need to be a query created or updated that would return all the calendar entries for a particular time frame including the recurring ones.
  3. We will need the ability to edit any recurrence of the event at any time that the event occurs. The edits would need to be valid for:
    1. All instances in the series
    2. Just the instance in question
    3. The instance in question and all those in future
  4. Administrators will need to be able to track attendance for any type of event. Therefore you will need to think of ways to create permanence for historical recurring events for this purpose
  5. We don't want the creation of an infinite number of event calendar entries if a recurring event has no end date
  6. Other functionality must not be affected
  7. You must write tests to validate your work

Actual behavior

  • See above

Screenshots

  • N/A

Additional details

  1. Work with @xoldyckk on evaluating your solution here and in the subsequent PR
  2. This is a prerequisite for our menu of project ideas for GSoC 2024
  3. When this issue is resolved we'll need to create another one to support custom recurrences, such as 3rd week of the month, similar to what is available in Google Calendar
  4. We almost completed the work for Weekly recurring when the assignee @Community-Programmer was unable to continue due to exam time constraints.
    1. The first PR for this issue will need to build on the work in this abandoned PR as it was almost approved for merging.
      1. Fix bug recurring event #1738
    2. You will need to submit a plan as described here
      1. Fix bug recurring event #1738 (comment)
  5. You will need to split this issue into multiple PRs, each with custom options provided by the RRULE package
    1. Weekly recurring
    2. Daily recurring
    3. Monthly recurring
    4. Annually recurring

Potential internship candidates

@palisadoes palisadoes added the bug Something isn't working label Dec 27, 2023
@github-actions github-actions bot added question Further information is requested unapproved Unapproved for Pull Request labels Dec 27, 2023
@Community-Programmer
Copy link
Contributor

Please assign me , i will work on this issue

@palisadoes palisadoes removed the unapproved Unapproved for Pull Request label Dec 27, 2023
@palisadoes
Copy link
Contributor Author

Please read the requirements. They have been frequently updated.

@Community-Programmer
Copy link
Contributor

Community-Programmer commented Dec 27, 2023

Thank you for providing clarification on the requirements for this issue. I have carefully reviewed the outlined steps and understand the expectations for the solution.

But, I'd like to request to give me some time to thoroughly investigate the issue. To ensure a comprehensive resolution, I plan to go through the codebase, understand the program's flow, and conduct a deep analysis of the problem before diving into the implementation.

Rest assured, I'll keep you posted on my progress, and if there are any points where I require clarification or assistance, I won't hesitate to reach out for your guidance.

Thank You

@palisadoes
Copy link
Contributor Author

Please work with @xoldyckk on this for updates, advice and general comments

@palisadoes
Copy link
Contributor Author

We don't want the creation of an infinite number of event calendar entries if a recurring event has no end date

@Community-Programmer
Copy link
Contributor

We don't want the creation of an infinite number of event calendar entries if a recurring event has no end date

@palisadoes

Yeah, got it. Will handle this with any of the following solutions:

  • Giving user confirmation or warning
  • Default maximum recurrence or system-defined maximum

I'm currently going through the issue , and I'm actively exploring all possible solutions. Once I've completed my analysis, I'll reach out @xoldyckk to discuss the process I plan to follow

@palisadoes
Copy link
Contributor Author

  1. You may want to consider a separate collection for recurring events with just a start and end date. Then let the web app populate the calendar based on that information. That would eliminate the infinite problem.
  2. The challenge would then be editing any event in the series (only this event, all events, this and future events), which could be solved by a linked list of recurring events each with is own start and stop date.

Whether graphql could solve this is another matter.

@Community-Programmer
Copy link
Contributor

Community-Programmer commented Dec 28, 2023

  1. You may want to consider a separate collection for recurring events with just a start and end date. Then let the web app populate the calendar based on that information. That would eliminate the infinite problem.
  2. The challenge would then be editing any event in the series (only this event, all events, this and future events), which could be solved by a linked list of recurring events each with is own start and stop date.

Whether graphql could solve this is another matter.

@palisadoes @xoldyckk

Instead of implementing a linked list of recurring events, I have another, better, and more effective solution.

  1. We will create a separate collection for recurring events that will be linked to the Event schema.
  2. When an event is created, we will check whether it is recurring or not. If it is, we will create instances for recurring events.
  3. This approach will ensure smoother and more reliable event editing for present and future events.
  4. Additionally, we will get a unique ID to each event, which will be helpful for efficient editing of events.

image

Here is a rough idea how we can implement this:

Mongoose Schema

const mongoose = require('mongoose');
const { Schema } = mongoose;

const recurringEventInstanceSchema = new Schema({
  eventId: { type: Schema.Types.ObjectId, ref: 'Event', required: true },
  start: { type: Date, required: true },
  end: { type: Date, required: true },
  //Accordingly more required fields can be added
});

const eventSchema = new Schema({
  title: { type: String, required: true },
  start: { type: Date, required: true },
  end: { type: Date, required: true },
  frequency: { type: String, required: true },
  interval: { type: Number, required: true },
  recurring: {type:  boolean}, //Either true or false
  //Accordingly more required fields can be added
});

const RecurringEventInstanceModel = mongoose.model('RecurringEventInstance', recurringEventInstanceSchema);
const EventModel = mongoose.model('Event', eventSchema);

GraphQL mutation

Mutation: {
    createEvent: async (_, { event }) => {
      try {
        const createdEvent = await EventModel.create(event);
        
        // If the event is recurring, generate and store individual instances
        if (createdEvent.recurring) {
          await generateRecurringEventInstances(createdEvent);
        }
        
        return createdEvent;
      } catch (error) {
        throw new Error('Error creating event');
      }
    }
};

According to this approach, we can generate a query for updating individual events with the unique event ID. I believe this will be the most efficient solution to solve this issue.

What is your opinion on this?

@palisadoes
Copy link
Contributor Author

  1. FYI - We already have an Event collection with some of the fields mentioned
    1. https://docs.talawa.io/docs/schema/objects/event
  2. This post describes what Google Calendar does.
    1. They have the equivalent of recurringEventInstanceSchema
    2. They create the entire series of events upon creation of the recurring event
    3. One of the comments states that Google's recurring events all end in the year 2100.
    4. They have a mechanism for creating new parents in recurringEventInstanceSchema if any part of a recurring instance is updated. eg:
      1. Only this event
      2. All events
      3. This and future events
    5. We should use this approach.
    6. So in your case all the frequency tracking fields would be in the recurringEventInstanceSchema it would seem.
    7. The existing Event collection would have a pointer back to the recurringEventInstanceSchema
    8. https://stackoverflow.com/questions/50204299/recurring-events-database-model
  3. You will need to accommodate the customizations that Google has for recurring events
    1. Weekly on any combination of days
    2. Monthly on a specific day(s), and week(s) of the month
      1. For the sake of simplicity, we should do this as a follow up issue as soon as this one closes. I don't want to have tricky timing delay the overall goal.

Your mutation logic for handling edits could be tricky, including.

  1. Adjusting the day that the event falls on from date X onward.
    1. We should also leave this for a follow up issue for simplicity. Open it up as soon as the PR for the basic functionality is complete.

@Community-Programmer
Copy link
Contributor

@xoldyckk @palisadoes

Thanks for providing detailed information. I've reviewed the Google Calendar approach and the recurringEventInstanceSchema, and I agree with your proposal to follow a similar model for our system.

I'll prioritize implementing the basic functionality first, aligning with Google Calendar's method. Once that's completed and the initial PR is merged, we can address the additional complexities and customizations, such as accommodating different recurrence patterns.

I'll keep you posted on the progress, and we can open follow-up issues as needed.

Thanks for your guidance, and let's move forward with this plan

@Community-Programmer
Copy link
Contributor

@xoldyckk @palisadoes

I am almost prepared to create a pull request for this issue but require some clarifications.

I've included the same fields for recurring events as those for regular events, adding only one more field: 'parentEvent.' Everything else, including both GraphQL input and type, remains unchanged. Do I need to make any additional additions or removals?

image

recurring Event Model

image

@palisadoes
Copy link
Contributor Author

palisadoes commented Dec 29, 2023

@Community-Programmer

  1. What is the logic of operation? In other words, what happens in your logic when a Talawa end user creates an event, that happens to be recurring?
  2. How is it the same and different from the stackoverflow post?
    1. https://stackoverflow.com/questions/50204299/recurring-events-database-model

image

According to the Stack Overflow post Google calendar:

  1. Creates a series of single instance events whenever an event is created
  2. Makes all events in a recurring series linked to an DB collection/table tracking the series
  3. Makes modifications to an event by back referencing the tracker to update all the future events in the series
    1. For example, if you add people to a calendar entry, the back reference to the tracker gets you all the calendar entries in the series. Then it updates all the individual calendar entries from day X onward with the update.
  4. Stores the details of each recurrence in the tracker
  5. Changes in the frequency, creates a new recurring series entry starting from day X, and all the old events remain with the previous series, and the previous series ends when the new one starts.

This seems like a relatively simple methodology. Is that what you are planning?

We need to ensure as much backward compatibility as possible with the Talawa client apps that expect this feature to work.

@palisadoes
Copy link
Contributor Author

palisadoes commented Dec 30, 2023

I think we'll need to split this up into multiple PR with child issues:

  1. Create a recurring instance defaulting to a week
  2. Edit the recurring instance
    1. Change invitees
      1. All the above at any point in the recurrence with the changes propagating forward
    2. Change end date
    3. No end date
  3. Create recurring instances for (separate PRs each which we could open to other contributors too)
    1. for every x (weeks, months) with and without end dates
    2. for every x (weekdays, days) with and without end dates
    3. editing recurring instances
      1. Every x weeks to every x months and vice versa
      2. Invitees
      3. End dates
      4. All the above at any point in the recurrence with the changes propagating forward
    4. editing recurring instances (every x days to every x months and/or weeks and vice versa)
  4. For the other custom Google options

@Community-Programmer
Copy link
Contributor

Community-Programmer commented Dec 30, 2023

@Community-Programmer

  1. What is the logic of operation? In other words, what happens in your logic when a Talawa end user creates an event, that happens to be recurring?

  2. How is it the same and different from the stackoverflow post?

    1. https://stackoverflow.com/questions/50204299/recurring-events-database-model

image

According to the Stack Overflow post Google calendar:

  1. Creates a series of single instance events whenever an event is created

  2. Makes all events in a recurring series linked to an DB collection/table tracking the series

  3. Makes modifications to an event by back referencing the tracker to update all the future events in the series

    1. For example, if you add people to a calendar entry, the back reference to the tracker gets you all the calendar entries in the series. Then it updates all the individual calendar entries from day X onward with the update.
  4. Stores the details of each recurrence in the tracker

  5. Changes in the frequency, creates a new recurring series entry starting from day X, and all the old events remain with the previous series, and the previous series ends when the new one starts.

This seems like a relatively simple methodology. Is that what you are planning?

We need to ensure as much backward compatibility as possible with the Talawa client apps that expect this feature to work.

The logic is nearly the same: when a Talawa end user creates a recurring event, the createRecurringEvent mutation is called. This mutation generates all recurring event instances from the specified start date to end date, with the user-selected frequency ["ONCE," "DAILY," "WEEKLY," "MONTHLY," "YEARLY"]. By default, the frequency is set to weekly.
If a user doesn't create a recurring event, the createEvent mutation is called.

This is the approach I am considering. I would appreciate your insights on it.

Thank You

@Community-Programmer
Copy link
Contributor

I think we'll need to split this up into multiple PR with child issues:

  1. Create a recurring instance defaulting to a week

  2. Edit the recurring instance

    1. Change invitees

      1. All the above at any point in the recurrence with the changes propagating forward
    2. Change end date

    3. No end date

  3. Create recurring instances for (separate PRs each which we could open to other contributors too)

    1. for every x (weeks, months) with and without end dates

    2. for every x (weekdays, days) with and without end dates

    3. editing recurring instances

      1. Every x weeks to every x months and vice versa
      2. Invitees
      3. End dates
      4. All the above at any point in the recurrence with the changes propagating forward
    4. editing recurring instances (every x days to every x months and/or weeks and vice versa)

  4. For the other custom Google options

Ok, breaking down the tasks into multiple pull requests (PRs) with child issues is a thoughtful approach.

@palisadoes
Copy link
Contributor Author

palisadoes commented Dec 30, 2023

Why not assume all events are recurring with 0 to N repetitions? With 0 repetitions it would just generate a single event. That way you don't need a separate mutation.

Then if a single event were made recurring, it would be easier logic, wouldn't it?

@Community-Programmer
Copy link
Contributor

I appreciate your suggestion, and it's valid. Initially, I opted for a separate mutation to enhance clarity and understanding of the codebase. However, upon further consideration, I agree that maintaining a single mutation could simplify the logic without compromising functionality.

I'll made the adjustment accordingly. Thank you for your guidance

@palisadoes
Copy link
Contributor Author

OK. Please open a PR when ready with the appropriate tests.

I'd suggest thinking of all the various parameters to use in the mutation and implement them one at a time.

We need to change the sample database to include weekly recurring events so that a full calendar will always be visible. This will help other developers get a sense of what we're trying to achieve. There's a separate issue for that.

Think of how we could support this first, possibly with a Talawa Admin collaborator to add a weekly option when creating an event. A checkbox to start with more UI friendly options later.

Remember we need to create incremental PRs to fix incrementally advancing functionality.

At some point we'll have multiple people working simultaneously on the mutation logic which will cause merge conflicts. Think of ways of simply refactoring the logic into classes (or something equivalent), each residing in a separate file to reduce the risk without making the code hard to follow. Hopefully this would allow us to delegate sequencing work a class/file at a time.

For example, classes for weeks, months, days, weekdays and all the customizations within those time frames that could occur. All tied together with the standard event generation and regeneration logic.

There may be better ways that you could think of too.

@palisadoes
Copy link
Contributor Author

There are two schema generators at the end of this GitHub action file. One of them exports markdown files that we commit to the Talawa Docs repo to create the Talawa website. It will have the information you require on how to document your schema code to make the markdown user friendly

@Community-Programmer
Copy link
Contributor

Sure, I'll follow your suggestions and work on implementing the mutation logic for the weekly recurring events. I'll start by considering various parameters and implementing them one at a time, as you've suggested.

I'll keep you updated on the progress and ensure that the PRs are created with appropriate tests.

Thank You

@DecodeAndCode
Copy link

Can I raise the PR after this issue is resolved.

@palisadoes
Copy link
Contributor Author

@Community-Programmer Please resubmit the PR as it was merged without realizing that there were reviewer comments

@Community-Programmer
Copy link
Contributor

@palisadoes Ok, I appreciate the feedback. I will resubmit the PR with the suggested changes as per the reviewer's comments.

@meetulr
Copy link

meetulr commented Feb 22, 2024

Yes, I'm adding comments at every step. I realize the keepEvent flag would be good here, in case the user is making a single event recurring.

  1. If it's created as a single event by mistake or in a hurry, and is being updated to be a recurring event, keepEvent would be false, and the events would be generated based on the recurrence pattern provided (or default weekly).
  2. But like I mentioned earlier, in some cases, keepEvent: true would make sense, for example:
  • Some past or current event that was experimental, which was agreed or approved to be a recurring event, in which case we'd keep that event and generate recurring instances based on its data.
  • Or some future event which was created as a single event (say on a sunday), updated to be recurring, but to only follow the recurrence after that first instance. i.e. the first one would be on sunday, after that, the recurring instances would follow the recurrence pattern provided (or default weekly).
    In those cases, we'd want to keep the first occurence (which was the single event), and generate the recurring instances ahead.

But maybe I'm overthinking this for now.
Google calendar just deletes the current single event (past or future), and just creates the recurring instances, while updating a single event to be recurring. We could start with that to get the basic update functionality done, and add the modifications later if required.

Would that be ok?

@palisadoes
Copy link
Contributor Author

Do it the Google way

@palisadoes
Copy link
Contributor Author

What else is outstanding?

@meetulr
Copy link

meetulr commented Feb 27, 2024

What else is outstanding?

Delete functionality. After that, basic CRUD for recurring events would be done.

@meetulr
Copy link

meetulr commented Feb 29, 2024

Currently, when we delete single events with removeEvent mutation, we simply update their status to DELETED, and when we delete it with adminRemoveEvent mutation, we're deleting the document from the database too.

Could you please elaborate on the purpose of these distinct mutations, and the event status "ACTIVE/BLOCKED/DELETED"?

Lastly, how do we want them to work for recurring events, when delete multiple/single instances.

@palisadoes
Copy link
Contributor Author

  1. Why the inconsistency between deletions by Admins and regular users? It should be the same. Is it an oversight? If there is no undelete feature, then they should all be deleted.
  2. I don't think there is a BLOCKED status that is used, therefore that only leaves ACTIVE which therefore could be completely removed

@meetulr
Copy link

meetulr commented Feb 29, 2024

Why the inconsistency between deletions by Admins and regular users? It should be the same. Is it an oversight? If there is no undelete feature, then they should all be deleted.

That's what I wanted to know.

I asked because separate mutations were made and tested. What was the original purpose of the status field?
Were there initially some plans that justified implementing it this way?
The event interface describes status as ACTIVE, BLOCKED, or DELETED.

I don't think there is a BLOCKED status that is used, therefore that only leaves ACTIVE which therefore could be completely removed.

Yes, it's not being used, and neither is adminRemoveEvent.
removeEvent is being used, which changes the status of the event to DELETED.

The query for events currently returns all the ACTIVE events.

@Community-Programmer
Copy link
Contributor

@meetulr

I hope this message finds you well. I wanted to express my deepest gratitude for stepping in and handling this issue while I am deeply engrossed in my exams. Your willingness to take on this responsibility means more to me than words can convey
Thank you so much!!

@palisadoes
Copy link
Contributor Author

  1. We should remove the orphan code, especially if DELETED and BLOCKED is never used by the clients.
  2. Verify with the other repos and create issues to make the updates.

Keeping the DELETED flag adds to the confusion as in certain cases the recurring events are physically deleted, versus just having the flag being set.

@meetulr
Copy link

meetulr commented Feb 29, 2024

I think the status is not being used anywhere.

So, I'm deleting the documents (from database) when deleting an event, rather than changing its status.

We could fix it later if it causes some breaking change.

@palisadoes
Copy link
Contributor Author

palisadoes commented Feb 29, 2024

OK. Remove the status code too.

@meetulr
Copy link

meetulr commented Feb 29, 2024

@meetulr

I hope this message finds you well. I wanted to express my deepest gratitude for stepping in and handling this issue while I am deeply engrossed in my exams. Your willingness to take on this responsibility means more to me than words can convey Thank you so much!!

Ofcourse brother, your research and discussion on this issue played a major part. It helped a lot!

@meetulr
Copy link

meetulr commented Feb 29, 2024

OK. Remove the status code too.

I have removed it from the delete event mutation (For single events too).

@palisadoes
Copy link
Contributor Author

  1. Should be closed now?
  2. What about the documentation for Talawa-Docs?

@meetulr
Copy link

meetulr commented Mar 1, 2024

  1. Should be closed now?

Yes.

  1. What about the documentation for Talawa-Docs?

Yes, I'll open an issue in talawa-docs. I'll be fine tuning the page, and adding more stuff to it.

@meetulr
Copy link

meetulr commented Mar 1, 2024

Can I open an issue for recurring events in admin? I could start implementing the features, and make corrections to the backend (wherever/if required) along the way.

@palisadoes
Copy link
Contributor Author

Yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants