fix (advertisements): fetch advertisements correctly with their completion status in infinite scroll #3444
Conversation
WalkthroughThis pull request introduces a new input type, Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant G as GraphQL Server
participant R as Advertisement Resolver
participant D as Database
C->>G: Query advertisements(where: { isCompleted: true/false })
G->>R: Forward query with pagination and where arguments
R->>R: Process args with createGraphQLConnectionWithWhereSchema
R->>D: Query database with filtered criteria
D-->>R: Return filtered advertisements
R-->>G: Deliver results
G-->>C: Send response
Assessment against linked issues
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)test/graphql/types/Organization/advertisement.test.ts (3)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (5)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
There isn’t an issue assigned to you for this PR. Please follow the guidelines in our PR_GUIDELINES.md file. We have the procedures in place so that everyone has a fair chance of contributing. I will be closing this pull request. Please follow the procedures and resubmit when ready. |
bandhan-majumder
changed the title
bandhan-majumder
changed the title
|
@Cioppolo14 @palisadoes can this pr be opened now? |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
schema.graphql(3 hunks)test/graphql/types/gql.tada.d.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (1)
test/graphql/types/gql.tada.d.ts (1)
265-266: File formatting improvement.Adding a newline at the end of the file is a good practice and adheres to common coding standards. This change has no functional impact but improves consistency in the codebase.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
test/routes/graphql/documentNodes.ts (1)
1-773: 💡 Verification agent🧩 Analysis chain
Fix the formatting issues identified by the pipeline.
The GitHub Actions workflow has identified formatting issues in the code. While the indentation changes are an improvement, there are still formatting issues that need to be addressed.
🏁 Script executed:
#!/bin/bash # Find the formatter used in the project and see if there are config files echo "Checking for formatter configuration files:" find . -maxdepth 2 -name "*.prettier*" -o -name "*.eslint*" -o -name "*.format*" # Check package.json for formatting scripts echo -e "\nChecking package.json for formatting scripts:" grep -A 3 '"format"' package.json || echo "No format script found" # Try to find documentation about formatting echo -e "\nChecking for documentation about formatting:" find . -maxdepth 2 -name "CONTRIBUTING.md" | xargs grep -l "format" || echo "No formatting documentation found"Length of output: 625
Action Required: Address Formatting Issues in test/routes/graphql/documentNodes.ts
The GitHub Actions workflow still flags formatting problems in this file. Although the indentation changes are an improvement, our verification shows that the project currently lacks any formatter configuration (no Prettier, ESLint, or custom formatting scripts in package.json, nor documentation in CONTRIBUTING.md). Please add the appropriate formatter configuration or manually reformat the code to adhere to our style guidelines.
🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] 6-684: Formatter would have printed the following content indicating formatting issues. Please run the formatter to fix the code style.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/routes/graphql/documentNodes.ts(18 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/routes/graphql/documentNodes.ts (2)
test/routes/graphql/gql.tada.d.ts (1)
introspection(6710-6716)src/graphql/scalars/index.ts (1)
ClientCustomScalars(30-36)
🪛 GitHub Actions: Pull request workflow
test/routes/graphql/documentNodes.ts
[error] 6-684: Formatter would have printed the following content indicating formatting issues. Please run the formatter to fix the code style.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (6)
test/routes/graphql/documentNodes.ts (6)
8-9: Formatting improvement looks good.The indentation changes make the code more consistent and readable.
13-40: Consistent indentation applied throughout document nodes.The formatting changes across all these GraphQL document nodes improve code readability while maintaining functionality. This is a good practice for maintaining a consistent code style.
Also applies to: 43-47, 50-74, 77-104, 107-131, 134-158, 187-189, 298-324, 327-331, 334-360, 371-377, 380-386, 389-393, 396-402, 405-411, 414-420, 450-467, 470-474, 477-490, 573-582, 677-679, 682-684
721-765: Great implementation of the query for fetching advertisements with filtering.This new
Query_advertisementsGraphQL query includes:
- The required organization ID parameter
- Standard pagination parameters (first, last, after, before)
- The new filtering parameter using
AdvertisementWhereInputwhich supports theisCompletedfield- A comprehensive response structure with all necessary advertisement fields
- Proper pagination metadata
This implementation effectively supports the PR objective of fetching advertisements with their completion status in infinite scroll.
767-773: Simple and effective mutation for creating advertisements.The
Mutation_createAdvertisementfollows the established pattern in this codebase for mutation definitions. It provides the minimum required feedback (the ID of the created advertisement) which is sufficient for most client-side use cases.
728-729:✅ Verification successful
Ensure the AdvertisementWhereInput type is properly defined in the schema.
The query uses
AdvertisementWhereInputas a filter parameter, which should include theisCompletedboolean field mentioned in the PR description. Make sure this type is properly defined in the schema file.
🏁 Script executed:
#!/bin/bash # Check if the AdvertisementWhereInput type exists in the schema echo "Checking for AdvertisementWhereInput in schema files:" find . -name "*.graphql" | xargs grep -l "AdvertisementWhereInput" | while read file; do echo "Found in $file:" cat "$file" | grep -A 10 "AdvertisementWhereInput" || echo "Not found in expected format" done # Check if there are resolver implementations for this input type echo -e "\nChecking for resolver implementations:" find . -name "*.ts" -o -name "*.js" | xargs grep -l "AdvertisementWhereInput" | grep -v "documentNodes.ts" || echo "No other files reference this type"Length of output: 1453
AdvertisementWhereInput Definition Verified
The schema file (
./schema.graphql) properly defines theAdvertisementWhereInputtype with the expectedisCompleted: Booleanfield. Additionally, the references in resolver implementations (notably in./src/graphql/inputs/QueryOrganizationInput.tsand./src/graphql/types/Organization/advertisements.ts) confirm that the input type is correctly integrated and in use.No changes are required regarding this type.
721-764:✅ Verification successful
Verify the integration with the frontend issue.
According to the PR description, this feature is necessary for addressing a frontend issue in the talawa-admin repository (#3557). Confirm that this implementation provides all the necessary data required by the frontend.
🌐 Web query:
What is infinite scroll pagination in GraphQL and how does it typically work with advertisements?💡 Result:
Infinite scroll pagination in GraphQL dynamically loads content as users scroll, avoiding traditional page navigation. It combines GraphQL's pagination capabilities with frontend techniques to fetch and append data continuously. Here's how it works and interacts with advertisements:
GraphQL Pagination Mechanics
- Offset-based pagination uses
first/limitandoffsetarguments to fetch chunks of data (e.g.,{ posts(first: 10, offset: 0) { ... } })[1][3]. Apollo Client'sfetchMore()method simplifies this by appending new results to existing data[7][9].- Cursor-based pagination relies on unique identifiers (e.g.,
after: "cursor") for efficient record tracking, often using Relay-compatible connections withedgesandpageInfo[6][10].Infinite Scroll Implementation
- Scroll detection:
- Scroll event listeners calculate proximity to the bottom using
scrollTop,clientHeight, andscrollHeight[1][7].- Modern approaches use
IntersectionObserverto trigger loads when a sentinel element becomes visible[1][9].- Data fetching:
- Each scroll triggers a GraphQL query for the next data chunk.
- Apollo Client caches and merges results automatically[4][7].
Advertisement Integration Challenges
- Ad visibility:
- Dynamically loaded ads risk low viewability as users scroll past them quickly. Unviewed ads dilute advertiser bids over time[2][8].
- Solutions include lazy-loading ads when they approach the viewport and using debounced refresh for sticky ad units[5].
- Performance impact:
- Continuous ad loading can slow page speed. GPT (Google Publisher Tag) optimizations like Single Request Architecture (SRA) help[5].
- Ad density must balance revenue goals with user experience to avoid "ad fatigue"[2][8].
Typical Workflow with Ads
// Example using IntersectionObserver for ads const adObserver = new IntersectionObserver((entries) => { entries.forEach(entry => { if (entry.isIntersecting) { googletag.pubads().refresh([adSlot]); } }); }); adObserver.observe(adPlaceholderElement); // GraphQL data fetch const { fetchMore } = useQuery(GET_POSTS, { variables: { offset: 0, limit: 10 }, });Key considerations:
- Use
rootMarginto preload ads before they enter the viewport[5][9].- Reset ad slots on new page segments to avoid duplicate impressions[5].
- Monitor Content-to-Ad ratios to maintain SEO and compliance[5][10].
While infinite scroll boosts engagement[8], publishers often see declining RPMs due to ad stacking and poor visibility[2][5]. Hybrid approaches—like combining scroll with occasional "Load More" buttons—can mitigate these issues while preserving user experience[8][10].
Citations:
- 1: https://blog.logrocket.com/infinite-scrolling-graphql/
- 2: https://www.ezoic.com/the-downsides-of-infinite-scroll/
- 3: https://daily.dev/blog/pagination-in-graphql
- 4: https://www.apollographql.com/blog/pagination-and-infinite-scrolling
- 5: https://headerbidding.co/gpt-infinite-scroll-pages-ads/
- 6: https://www.apollographql.com/blog/understanding-pagination-rest-graphql-and-relay
- 7: https://sysgears.com/articles/how-to-implement-infinite-scroll-with-graphql-and-react/
- 8: https://www.publift.com/blog/infinite-scroll-vs-load-more-buttons-which-is-better
- 9: https://dev.to/moruno21/graphql-infinite-scroll-4oan
- 10: https://crocoblock.com/blog/pagination-vs-infinite-scroll/
Action: Confirm GraphQL Query Provides Complete Infinite Scroll Data for Advertisements
The query for advertisements correctly implements cursor‑based pagination (using
first,last,after,before) and returns both theedges(with all essential advertisement fields) and the associatedpageInfo(includingstartCursor,endCursor,hasNextPage, andhasPreviousPage). These details align with typical infinite scroll pagination requirements, ensuring that the frontend in the talawa-admin repository (#3557) will receive the full set of data it needs for dynamically loading additional advertisements.
- Confirm that the frontend integration hooks into these pagination fields correctly.
- Verify that the filter argument (
where) is handled appropriately in practice.
palisadoes
added
the
ignore-sensitive-files-pr
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/graphql/types/Organization/advertisements.ts(6 hunks)test/graphql/types/Organization/advertisement.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/graphql/types/Organization/advertisement.test.ts (3)
test/graphql/types/client.ts (1)
mercuriusClient(7-9)test/server.ts (1)
server(12-27)test/routes/graphql/documentNodes.ts (2)
Query_advertisements(721-765)Mutation_createAdvertisement(767-773)
src/graphql/types/Organization/advertisements.ts (2)
src/utilities/defaultGraphQLConnection.ts (4)
createGraphQLConnectionWithWhereSchema(147-153)transformGraphQLConnectionArgumentsWithWhere(163-184)defaultGraphQLConnectionArgumentsSchema(41-62)ParsedDefaultGraphQLConnectionArgumentsWithWhere(28-36)src/graphql/inputs/QueryOrganizationInput.ts (1)
AdvertisementWhereInput(64-76)
🪛 GitHub Actions: Pull request workflow
test/graphql/types/Organization/advertisement.test.ts
[error] 56-56: Type '{ isCompleted: false; }' is not assignable to type 'null | undefined'.
[error] 110-110: Type '{ isCompleted: false; }' is not assignable to type 'null | undefined'.
[error] 162-162: Type '{ isCompleted: false; }' is not assignable to type 'null | undefined'.
[error] 232-232: Type '{ isCompleted: false; }' is not assignable to type 'null | undefined'.
[error] 298-298: Type '{ isCompleted: true; }' is not assignable to type 'null | undefined'.
[error] 486-486: Type '{ isCompleted: false; }' is not assignable to type 'null | undefined'.
[error] 668-668: Type '{ isCompleted: true; }' is not assignable to type 'null | undefined'.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (5)
test/graphql/types/Organization/advertisement.test.ts (1)
181-313: Verify Test Assertions for “Active” vs. “Completed”
The tests confirm that a banner advertisement with start/end dates in the future is considered active, and one with past end dates is considered completed. The logic and assertions look correct. Good job validating edges, page info, and ensuring completeness.🧰 Tools
🪛 GitHub Actions: Pull request workflow
[error] 232-232: Type '{ isCompleted: false; }' is not assignable to type 'null | undefined'.
[error] 298-298: Type '{ isCompleted: true; }' is not assignable to type 'null | undefined'.
src/graphql/types/Organization/advertisements.ts (4)
149-157: Well-Structured Argument Destructuring
Extractingcursor,isInversed,limit, andwherefromparsedArgsis clearly laid out and aids readability. Good job keeping the code maintainable.
183-197: Check Timestamps for Completed/Active Condition
You rely onlt(advertisementsTable.endAt, today)to gauge completion status. This is correct for events that ended before the current time, but confirm that the time zones are consistent and that you handle edge-cases (e.g., ifendAtis exactly now).
265-281: Double Check Future Ads
ForextendedArgs.isCompleted === false, you checkgt(advertisementsTable.endAt, today). Confirm that an advertisement that started in the past but ends in the future is considered ongoing (thus “active”), in line with your requirements.
26-57:❓ Verification inconclusive
Validate Cursor Schema Reference
You are mergingadvertisementWhereSchemainto the connection arguments. Verify the cursor schema references when mixingwheredata with pagination. Consider adding more thorough tests to confirm that cursors behave correctly ifwhere.isCompletedis set, while also ensuring that no query errors occur.
🏁 Script executed:
#!/bin/bash # Scan for any references to advertisementWhereSchema usage # to ensure all cursor transformations handle optional fields properly. rg -A 5 'advertisementWhereSchema'Length of output: 968
Cursor Schema and Pagination Validation
The merged usage ofadvertisementWhereSchemain the connection arguments appears correct, with its definition (an optional object includingisCompleted) properly incorporated. However, please verify that the cursor transformation logic behaves as expected when awhereclause (particularly withisCompletedset) is present. It would be beneficial to add tests that:
- Confirm the correct parsing of a valid cursor (and proper error handling for invalid cursors).
- Ensure that merging the
wheredata does not interfere with pagination logic or lead to unexpected query errors.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3444 +/- ##
====================================================
+ Coverage 59.07% 59.60% +0.52%
====================================================
Files 523 523
Lines 40674 40782 +108
Branches 1735 1786 +51
====================================================
+ Hits 24030 24307 +277
+ Misses 16635 16464 -171
- Partials 9 11 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
test/routes/graphql/documentNodes.ts (1)
767-773: 🧹 Nitpick (assertive)Fix inconsistency in mutation operation name.
There is a naming inconsistency in the GraphQL operation. The export constant is named
Mutation_createAdvertisement, but the operation inside the GraphQL string is namedMutation_createAd. For consistency and clarity, these names should match.- mutation Mutation_createAd($input: MutationCreateAdvertisementInput!) { + mutation Mutation_createAdvertisement($input: MutationCreateAdvertisementInput!) {test/graphql/types/Organization/advertisement.test.ts (4)
74-187: Consolidate “empty active ads” & “empty complete ads” tests.
These two tests are nearly identical except for theisCompletedflag. Consider merging them into a single parameterized test for clarity, in line with previous review suggestions.
189-329: DRY up repeated organization creation.
The code for creating a new organization is duplicated in multiple test blocks. Extract this into a helper function to comply with DRY (Don’t Repeat Yourself), as previously suggested.
331-704: Consider a parameterized test for pagination checks.
You repeat advertisement creation and pagination checks for completed and non-completed ads. Consolidate these into a parameterized test to reduce duplication, as noted in previous feedback.
25-72: 🧹 Nitpick (assertive)Test suite naming mismatch.
This suite is named"Organization.blockedUsers Field"but is testing advertisement queries. Consider renaming the suite to match the relevant advertisement functionality.- suite("Organization.blockedUsers Field", () => { + suite("Organization.advertisements Field", () => {src/graphql/types/Organization/advertisements.ts (1)
185-198: Factor out repeated completion checks.
The branching logic forextendedArgs.isCompletedis duplicated in both theisInversedand non-inversed branches. A small utility function for buildingwhereconditions could minimize repetition.Also applies to: 207-223, 227-260, 261-283
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
schema.graphql(2 hunks)src/graphql/types/Organization/advertisements.ts(6 hunks)test/graphql/types/Organization/advertisement.test.ts(1 hunks)test/graphql/types/gql.tada.d.ts(2 hunks)test/routes/graphql/documentNodes.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
test/graphql/types/Organization/advertisement.test.ts (3)
test/graphql/types/client.ts (1)
mercuriusClient(7-9)test/routes/graphql/documentNodes.ts (4)
Query_signIn(191-218)Mutation_createOrganization(370-377)Query_advertisements(721-765)Mutation_createAdvertisement(767-773)test/server.ts (1)
server(12-27)
src/graphql/types/Organization/advertisements.ts (3)
src/utilities/defaultGraphQLConnection.ts (4)
createGraphQLConnectionWithWhereSchema(147-153)transformGraphQLConnectionArgumentsWithWhere(163-184)defaultGraphQLConnectionArgumentsSchema(41-62)ParsedDefaultGraphQLConnectionArgumentsWithWhere(28-36)src/graphql/inputs/QueryOrganizationInput.ts (1)
AdvertisementWhereInput(64-76)src/drizzle/tables/advertisements.ts (1)
advertisementsTable(20-109)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (10)
schema.graphql (2)
143-147: Well-structured input type with good documentation.The
AdvertisementWhereInputinput type is properly defined with clear documentation and a well-namedisCompletedfield to support filtering advertisements by completion status. The documentation is clear and descriptive.
2896-2904: Good implementation of the where parameter in advertisements field.The
advertisementsfield in theOrganizationtype has been correctly updated to include the newwhereparameter of typeAdvertisementWhereInput. The changes follow the established pattern of other connection fields and the documentation clearly explains the purpose of the parameter.test/routes/graphql/documentNodes.ts (1)
721-765: Well-structured query for organization advertisements.The
Query_advertisementsis properly structured to fetch advertisements with all necessary fields and includes support for the newwhereparameter for filtering by completion status. The pagination parameters are also correctly implemented.test/graphql/types/gql.tada.d.ts (2)
11-11: Confirm optional status ofisCompleted.
The newly addedAdvertisementWhereInputtype correctly marksisCompletedas an optional boolean. This aligns well with the tests passing objects like{ isCompleted: false }.
61-61: UpdatedHasUserVotedschema looks good.
The revised structure with separatehasVotedandvoteTypefields is clearer and aligns with best practices for boolean flags.test/graphql/types/Organization/advertisement.test.ts (1)
13-23: Positive sign-in setup.
The shared sign-in approach is succinct and easily reused, setting a good foundation for authenticated tests.src/graphql/types/Organization/advertisements.ts (4)
20-24: Schema definition aligns with usage.
DefiningadvertisementWhereSchemaas an optional object with an optionalisCompletedboolean matches theAdvertisementWhereInputand test usage, eliminating past pipeline errors.
26-57: Robust argument transformation.
UsingcreateGraphQLConnectionWithWhereSchemawith a custom transform helps keep code organized. Ensure any future fields inAdvertisementWhereInputare handled similarly.
75-81: Inline “where” argument on field definition is coherent.
Definingwhere: t.arg({ ... })here keeps the schema consistent with the Zod transformation. Good job aligning front-end queries with back-end logic.
150-158: Improved type inference.
Casting parsed arguments toParsedDefaultGraphQLConnectionArgumentsWithWhereclarifies the shape ofextendedArgs, avoiding confusion around thewhereproperty.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
test/graphql/types/Organization/advertisement.test.ts (4)
251-251: Remove debug console.log statementThe console.log statement should be removed as it will clutter test output.
- console.log("Active advertisements are: ", activeAdvertisements);
76-95: 🛠️ Refactor suggestionExtract common organization creation logic
The organization creation code is duplicated across multiple test blocks. Consider extracting this into a helper function to improve maintainability and follow the DRY principle.
// Helper function to create a test organization async function createTestOrganization() { const createOrgResult = await mercuriusClient.mutate( Mutation_createOrganization, { headers: { authorization: `bearer ${authToken}` }, variables: { input: { name: `Advertisements Test Org ${faker.string.uuid()}`, description: "Org to test advertisements", countryCode: "us", state: "CA", city: "San Francisco", postalCode: "94101", addressLine1: "100 Test St", addressLine2: "Suite 1", }, }, }, ); const orgId = createOrgResult.data?.createOrganization?.id; assertToBeNonNullish(orgId); return orgId; }Also applies to: 132-151, 191-210, 333-352, 519-538
359-477: 🛠️ Refactor suggestionCreate helper for multiple advertisement creation
The pagination tests create multiple advertisements with almost identical code. Consider creating a helper function to reduce duplication and improve maintainability.
// Helper function to create multiple advertisements async function createMultipleAdvertisements( orgId: string, count: number, isActive: boolean = true ) { const endDate = isActive ? "2030-02-02" : "2025-02-02"; for (let i = 1; i <= count; i++) { await mercuriusClient.mutate(Mutation_createAdvertisement, { headers: { authorization: `bearer ${authToken}` }, variables: { input: { organizationId: orgId, name: `Linux session ${i}`, type: "banner", startAt: new Date("2025-01-01").toISOString(), endAt: new Date(endDate).toISOString(), description: "learn about basics", attachments: undefined, }, }, }); } }Also applies to: 545-663
74-187: 🧹 Nitpick (assertive)Consider merging "empty active ads" and "empty completed ads" tests
These two tests differ only in the
isCompletedflag value. Consider parameterizing the test to reduce duplication.[true, false].forEach(isCompleted => { test(`should return an empty ${isCompleted ? 'completed' : 'active'} ads connection`, async () => { // Test implementation with isCompleted as parameter }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/graphql/types/Organization/advertisement.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/graphql/types/Organization/advertisement.test.ts (3)
test/graphql/types/client.ts (1)
mercuriusClient(7-9)test/routes/graphql/documentNodes.ts (4)
Query_signIn(191-218)Mutation_createOrganization(370-377)Query_advertisements(721-765)Mutation_createAdvertisement(767-773)test/server.ts (1)
server(12-27)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run tests for talawa api
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (5)
test/graphql/types/Organization/advertisement.test.ts (5)
25-25: Test suite name is accurate and clearThe test suite name "Organization.advertisement Field" correctly describes the functionality being tested.
56-58: Good implementation of the AdvertisementWhereInput filterThe
whereparameter withisCompleted: falsecorrectly uses the new filter capability that this PR introduces.
112-114: Filter for active advertisements implemented correctlyUsing
isCompleted: falseproperly filters for active advertisements.
312-314: Correctly filtering for completed advertisementsThe filter parameter
isCompleted: trueis properly used to get completed advertisements.
504-506: The implementation correctly uses the new filter parameterThe
isCompletedfilter parameter is properly used in pagination tests for both active and completed advertisements.Also applies to: 690-692
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
test/graphql/types/Organization/advertisement.test.ts (1)
117-154:⚠️ Potential issueFix test name and filter mismatch.
In this test named "should return an empty complete ads connection", you're using
isCompleted: falsewhich would filter for active advertisements rather than completed ones.Change the filter to match the test name:
- where: { - isCompleted: false, - }, + where: { + isCompleted: true, + },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
test/graphql/types/Organization/advertisement.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/graphql/types/Organization/advertisement.test.ts (3)
test/graphql/types/client.ts (1)
mercuriusClient(7-9)test/routes/graphql/documentNodes.ts (4)
Mutation_createOrganization(370-377)Query_signIn(191-218)Query_advertisements(721-765)Mutation_createAdvertisement(767-773)test/server.ts (1)
server(12-27)
🪛 Biome (1.9.4)
test/graphql/types/Organization/advertisement.test.ts
[error] 488-488: Forbidden non-null assertion.
Unsafe fix: Replace with optional chain operator ?. This operator includes runtime checks, so it is safer than the compile-only non-null assertion operator
(lint/style/noNonNullAssertion)
🪛 GitHub Actions: Pull request workflow
test/graphql/types/Organization/advertisement.test.ts
[error] 488-488: lint/style/noNonNullAssertion: Forbidden non-null assertion. Replace with optional chain operator ?.
[error] 1-1: Some errors were emitted while running checks.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (2)
test/graphql/types/Organization/advertisement.test.ts (2)
260-312: Great test for unspecified completion status filter.This test correctly verifies that when
isCompletedis not specified, both active and completed advertisements are returned, which is crucial functionality for the PR.
607-659: Comprehensive field validation test.The test effectively validates all the essential advertisement fields when fetched, ensuring the GraphQL query returns the expected data structure.
|
@palisadoes @disha1202 PTAL |
palisadoes
merged commit 0fac5c2
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
feature
Issue Number:
Fixes #3443
Snapshots/Videos:
N/A
If relevant, did you update the documentation?
N/A
Summary
Allows advertisements to be fetched with completion status
Does this PR introduce a breaking change?
N/A
Checklist
CodeRabbit AI Review
Test Coverage
Other information
This is needed for the frontend issue PalisadoesFoundation/talawa-admin#3557
Have you read the contributing guide?
Summary by CodeRabbit