Create weekly event recurring instance #1658
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
|
|
@palisadoes @sumitra19jha @ttaylor92 @xoldyckk Attention:I have made the required changes, but the test case is failing because, as suggested by @sumitra19jha, 'to start a transaction and make sure everything is a success or everything fails.'
So, the test case will fail if we are using a standalone MongoDB for development and testing. If we want to work with transactions in MongoDB, we'll need to set up a replica set even for our development environment. Convert a Standalone mongod to a Replica Set All test cases are passing when using replica sets and MongoDB Atlas Is there any other approach we should follow or continue with this? |
|
@Community-Programmer @sumitra19jha
I'd like to get this merged so that we can start creating issues in Admin and Mobile to implement weekly recurring meetings in their respective UIs |
can we comment out or skip the tests related to database replication for now? I'll make sure to include clear comments in the code, explicitly stating that these tests are skipped temporarily until database replication is implemented. This approach ensures that these tests are documented within the PR, and we can easily reintroduce them once the database replication feature is implemented. |
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #1658 +/- ##
===========================================
- Coverage 98.17% 97.34% -0.84%
===========================================
Files 184 202 +18
Lines 10767 12352 +1585
Branches 835 1000 +165
===========================================
+ Hits 10571 12024 +1453
- Misses 186 296 +110
- Partials 10 32 +22 ☔ View full report in Codecov by Sentry. |
|
@sumitra19jha @xoldyckk @ttaylor92 Please review |
|
Please write the code clean and correctly. Have you run the code to see if transactions are working? What do you mean by commenting out for replica? If you need replica, try to resolve it locally and run it there. If you don't, then don't put this unnecessary comments. Also please note when you ask for review, you have provided a clean code, containing logic, docs and test cases. Every mentor is busy, so when they put their time, they are looking for reviewing your final solution which might need few modifications (in another review) and then done. |
|
I have completely tested the code before submitting the PR. I tested each functionality locally including working of transactions and converted the standalone to replica locally for testing this, also tested it using MongoDB Atlas. I have commented out the code because if this gets merged it will cause trouble for others as test case will fail for them .So I commented out these test cases so that it can be reintroduced in the future once DB replication is implemented. Also i am documenting all the changes I am doing so that it help others when integrating this feature with Talawa admin and mobile In terms of cleaner code, I tried my best to reduce complexity and make it as understandable as possible. If you want, I can further clean it up by moving helper functions into a separate file. Further the complexity will increase when dealing with monthly recurring events. I apologise if asking for a review annoyed you. I understand you are busy reviewing other PRs. I'm sorry for my behavior Thank you |
There was a problem hiding this comment.
In hindsight, you have commented out test cases and stated that the code is thoroughly tested. The code coverage has declined significantly which is not desirable. This implies the testing done is manual, which is unverifiable. The code has therefore become more unstable.
If your testing logic requires replication to be enabled that means all other tests in the code base are at risk of being removed. This is not acceptable.
Restore the tests and figure out how to verify the functionality your new code thoroughly in an automated way.
|
I'd interpret |
Exactly correct. This is what I am trying to explain in my code review. The process is very simple
|
Community-Programmer
force-pushed
the
develop
branch
from
6646ec0 to
c8f2dd3
Compare
|
Please fix the failing tests |
|
Please take a look: |
|
resolved conversation please do see the comments once |
|
How do you plan to handle custom intervals like:
Where would that logic our in future? |
There was a problem hiding this comment.
How can the eventInstance.ts file be refactored so that functions to handle different intervals can be worked on simultaneously by multiple contributors minimizing merge conflicts?
We'll need to do this to speed up development time. Make a proposal because this approach will need to be done and is stated in the original issue
|
For regular intervals - I have already implemented the functionality for intervals as described below. The backend is set up to manage the scheduling of events based on the specified intervals. See below event is created 2024-01-08.13-04-09.mp4Similarly, I've implemented functionality for events occurring on the first Wednesday, last Wednesday, third Wednesday, etc. The backend takes care of these calculations. Now, we need to work on displaying the corresponding custom messages on the frontend. For custom intervals - I have thought of using two npm modules one The other functionality is almost complete, and I am waiting for this pull request to be thoroughly reviewed and merged. This will help me become familiar with it and serve as the foundation for all my future work, making it easier for other pull requests to be reviewed |
|
I am refactoring the file and also switching to refactoring like this eventInstance/ |
|
Please take a look: This is ready for review |
Yes, that's the better approach |
|
Sorry, I mistakenly closed the PR. |
Community-Programmer
requested review from
palisadoes and
JamaicanFriedChicken
|
|
Regarding the editing of events in a series, the plan is to introduce that functionality towards the end of the development process. Initially, we'll focus on adding features for each event creation within the series. Once those features are in place, we'll then extend the functionality to allow for editing the entire series flow for editing series of event will be like this: we'll implement a feature where we fetch events based on a specified start date and end date.Also When creating a series of events, I'll include an edit in the event model by adding a new field called |
|
OK. Merging |
|
Adding for cross referencing |
|
@Community-Programmer I am not able use this weekly recurring property even after adding a sample data in db for a weekly recurring event but in admin its showing its occurance as one. can you please me out here?? |
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-08.13-04-09.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