Fix bug recurring event

[Community-Programmer]

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-23.18-22-46.mp4

If relevant, did you update the documentation?
No

Summary

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

PR related:
#1658
#1712

[github-actions]

Our Pull Request Approval Process

We have these basic policies to make the approval process smoother for our volunteer team.

Testing Your Code

Please make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:

1. The overall code coverage drops below the target threshold of the repository
2. Any file in the pull request has code coverage levels below the repository threshold
3. Merge conflicts

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.

Reviewers

When your PR has been assigned reviewers contact them to get your code reviewed and approved via:

1. comments in this PR or
2. our slack channel

Reviewing Your Code

Your reviewer(s) will have the following roles:

1. arbitrators of future discussions with other contributors about the validity of your changes
2. point of contact for evaluating the validity of your work
3. person who verifies matching issues by others that should be closed.
4. person who gives general guidance in fixing your tests

CONTRIBUTING.md

Read our CONTRIBUTING.md file. Most importantly:

1. PRs with issues not assigned to you will be closed by the reviewer
2. Fix the first comment in the PR so that each issue listed automatically closes

Other

1. 🎯 Please be considerate of our volunteers' time. Contacting the person who assigned the reviewers is not advised unless they ask for your input. Do not @ the person who did the assignment otherwise.
2. Read the CONTRIBUTING.md file make

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (4c68ae5) 98.27% compared to head (2a3a672) 98.39%.
Report is 1 commits behind head on develop.

Files	Patch %	Lines
src/resolvers/Mutation/createEvent.ts	99.28%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1738      +/-   ##
===========================================
+ Coverage    98.27%   98.39%   +0.11%     
===========================================
  Files          205      203       -2     
  Lines        12511    12486      -25     
  Branches      1020     1021       +1     
===========================================
- Hits         12295    12285      -10     
+ Misses         184      171      -13     
+ Partials        32       30       -2     

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

[Cioppolo14]

@xoldyckk @EshaanAgg Can you review this PR?

[palisadoes]

Use this for historical reference:

• Fix bug recurring events #1712

[palisadoes]

Regarding the use of rruleStringin the Event schema

Wouldn't this be better represented by an RRULE object with the same structure as the JavaScript library?

Admin and mobile could use it too.

That way we can replace the package more easily in future in the function that translates between our objects and the underlying library.

[Community-Programmer]

Regarding the use of rruleStringin the Event schema

Wouldn't this be better represented by an RRULE object with the same structure as the JavaScript library?

Admin and mobile could use it too.

That way we can replace the package more easily in future in the function that translates between our objects and the underlying library.

The RRULE object can be generated from the rrulestring. I initially considered storing the object in the same format, but decided against it. This is because in monthly recurrence, we need to set events like 'last Monday' or 'second Monday.

If necessary I can try to do the same

[palisadoes]

It's supported in the object

• https://github.com/jkbrzt/rrule

[Community-Programmer]

@palisadoes
Can you tell exactly what I we have to store?
What we need to store is already present in the document, including information such as frequency, endDate, startDate, etc. that is required for rrlue.

[palisadoes]

I'd assume entries for all the RRULE constructor parameters:

• https://github.com/jkbrzt/rrule#rrule-constructor

[Community-Programmer]

I'd assume entries for all the RRULE constructor parameters:

• https://github.com/jkbrzt/rrule#rrule-constructor

Ok got it but I am still confused whether it is really needed?

It's unnecessary to store parameters like dstart (which is equivalent to startDate), until (which is equivalent to endDate), and frequency separately in the database. I believe storing only the event information makes more sense, as the rrule can be generated anytime from that
see below

[palisadoes]

Yes, whatever parameters you don't already have

[Community-Programmer]

@palisadoes

when saving all parameters we are getting it something like this

Here, where 0 represents Monday and 4 represents the fourth occurrence (equivalent to RRule.MO.nth(4)), it becomes challenging to reuse the logic, as one has to rewrite it. Can We store the entire RRule object as a JSON string?

Like this

[palisadoes]

OK. Start with that and let's see what the other reviewers say.

[EshaanAgg]

Refer to this article. We definitely do not need to store all the bloated data that is being provided by the Rrule object.

This is an example of a simple schema that can cover almost all of the recurrences that a user may need. We could create a utility in API that takes it and converts it to the format required by the library we are using to generate the dates. Storing JSON by converting it to strings is just bad, for understanding as well as performance. We should keep a minimal schema, and the application layer of API should host the logic of accessing the specific fields and using the RRule library.

[palisadoes]

@EshaanAgg

If we use RRULE then we have a lot of flexibility in not having to write code. I think it's necessary for the long term maintainability, new features and speed of implementation.

I feel we are better off storing the parameters as a formal data structure to enforce compliance.

The amount of overhead is low. There won't be billions of calendar entries per organization.

We will be wasting time creating our own solution that has to be updated with each new recurrence idea.

Are you in favor of storing the data in a structured format as I was hoping for earlier?

[Community-Programmer]

@EshaanAgg @palisadoes

Else can we store both rrule string + object?

[EshaanAgg]

Storing the RRule directly is not linked to ease of implementation IMO. Reading and parsing through a complete RRule object, and figuring out what are the appropriate fields that we should use would be a burden in itself. If we are able to explain all the desired functionality by the means of a few fields, if does not make sense to store all of them. Storing both the string and the rule object is wasteful.

@Community-Programmer I presume the library that we are using should provide a method to generate all the events between the startDate and the eventDate either based on the delayInOccurence (duration between two consecutive events) or keywords like weekly, yearly etc. That should be enough to cover all of the basic implementation we desire. So is there any other use of using the whole RRule object?

[palisadoes]

@EshaanAgg

We'll need to handle and desired combination of months, days, and weeks:

1. Every X day on the Nth week of the month
2. Weekly on X and Y day
3. Every X day on the last day of the month
4. Day X of every month
5. Day X of every year (birthdays, holidays etc)
6. Monthly on the last X weekday of the month

We need to support any combination of options that Google Calendar and its competitors provide. The end user is going to expect that. We may not support it all now, but we will need to do so eventually. Making provisions for this at the very beginning will reduce the burden later on.

[Community-Programmer]

@EshaanAgg

yaa we do have these methods

[Community-Programmer]

@palisadoes

can you tell which file is unauthorized why this test is failing?

[EshaanAgg]

@EshaanAgg

We'll need to handle and desired combination of months, days, and weeks:

1. Every X day on the Nth week of the month
2. Weekly on X and Y day
3. Every X day on the last day of the month
4. Day X of every month
5. Day X of every year (birthdays, holidays etc)
6. Monthly on the last X weekday of the month

We need to support any combination of options that Google Calendar and its competitors provide. The end user is going to expect that. We may not support it all now, but we will need to do so eventually. Making provisions for this at the very beginning will reduce the burden later on.

Got it. @Community-Programmer It's your call now. I am against storing the whole RRule object for I feel the same to be highly bloated, but if it is necesary to support all the use cases and more that @palisadoes expressed above, then we can. Please explore the options for the same, including any alternative libraries that might not be as bulky.

[EshaanAgg]

@palisadoes

can you tell which file is unauthorized why this test is failing?

package.json and package-lock.json as the you added the rrule library.

[Community-Programmer]

@EshaanAgg @palisadoes @xoldyckk
Bro, how can I take the call? I've worked on many open source projects previously, but this one is special. I've learned many things while implementing these ideas, fixing issues, and writing test cases. I really need guidance from you all to complete this project.

[palisadoes]

@Community-Programmer

1. I'll make the executive decision for you to add the RRULE fields in some way to the Event type and not store this data as text strings or JSON.
2. So many planned features will rely on this PR that we have to exhaust the options and get consensus agreement from they key contributors. This is certainly an exception.

Please proceed.

[palisadoes]

If this ready for review?

[xoldd]

@Community-Programmer before code implementation writing a detailed plan with visual diagrams would be better, explaining the flow of a client sending a graphql http request, mutation user input validation, mutation CRUD business logic, query dynamic data generation, database models, their relationships and constraints etc.

the bold italic highlighted parts are very important

[Community-Programmer]

If this ready for review?

Yes It is prepared for mutation and schema review

[palisadoes]

@xoldyckk

Some of that logic was provided here:

• Fix bug recurring events #1712 (comment)

[Community-Programmer]

@xoldyckk

I have prepared this please have a look and suggest all necessary changes
This is just a rough idea how things will work

[palisadoes]

@xoldyckk

1. There isn't a need for a description of business logic. It's simply implementing a recurring meeting.
2. We will be utilizing the logic of the RRULE package with the parameters here. The parameters are reflected in the schema presented in this PR
   1. Fix bug recurring event #1738 (comment)
3. The mutation validation will come from the RRULE package, but we should have our own validations.

[palisadoes]

@Community-Programmer

1. Please provide a brief answer for the questions on the constraints, input validations.
2. Update your code to match

@xoldyckk, @EshaanAgg

1. Over the past week, we had 62 comments in the previous PR prior to it being accidentally closed. This one has reached 26 in just 2 days.
2. Let's focus on the recurring event feature. If there are other tangential questions make that into a separate issue and not make it delay approval of this PR. We need to get this concluded.

[xoldd]

@palisadoes if number of comments on a pull request is a problem, the discussions should be held in the issues or discussions tab, having discussions is a good thing

this is a fairly complex feature request, that's why having discussions and finalizing on a general, easy to understand plan would be beneficial, the finalized plan has to be something easy to understand and reference for the reviewers when reviewing the PR, the reviewer can't scroll through the discussions to understand what has to be done in the PR

I mentioned business logic and other entities because while reviewing the PR the reviewers can't keep the context of all that in their heads, they should have a finalized general initial plan to reference, especially for PRs that are huge in size

figuring out mistakes after the code is implemented also leads to PRs with great number of comments because there's constant friction between what needed to be done and what was done, this is caused because of not having a general initial plan

[palisadoes]

@xoldyckk

I completely agree with your comments.

1. I'm more concerned with a digression in the discussion thread here causing delays versus discussion of the necessary aspects of this PR causing the delays. We can determine the forum for side discussions if needed at that time. I think the risk is low, but I wanted to set expectations.
2. We need the summary document at this stage after so much back and forth. Like you, I have asked for this too.

I wanted to set expectations because this is a key priority feature.

[palisadoes]

@Community-Programmer

Do you require any other guidance?

[Community-Programmer]

@Community-Programmer

Do you require any other guidance?

What should be my next step??
I think it is ready in terms of mutation and model

Should I have to create a general and finalized plan as @xoldyckk mentioned??

[palisadoes]

Yes, that would help

[palisadoes]

Thanks in advance. We need this work to be completed as it's fixing a glaring omission in the basic expected functionality of all the API clients

[palisadoes]

Closing as @Community-Programmer contacted me to say he won't have time to work on this for the next month.