Google Ads - Create Report improvement #12519
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
WalkthroughThe updates introduce an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CreateReportAction as GoogleAdsCreateReport
participant Queries as GoogleAdsQueries
participant Utils as GoogleAdsUtils
participant Database as GoogleAdsDatabase
User->>CreateReportAction: Create Report
CreateReportAction->>Queries: listResources(objectFilter, dateRange)
Queries-->>CreateReportAction: SQL Query
CreateReportAction->>Database: Execute SQL Query
Database-->>CreateReportAction: Query Results
CreateReportAction->>Utils: getOption(mapping data)
Utils-->>CreateReportAction: Mapped Data
CreateReportAction-->>User: Report Generated
Poem
Tip AI model upgrade
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
components/google_ads/actions/create-report/create-report.mjs (1)
Line range hint
143-167: Refactor suggestion forbuildQuerymethod.The method handles multiple aspects of SQL query construction. Consider refactoring to improve readability and maintainability. Breaking down the method into smaller functions for each part of the query (fields, segments, metrics, filters) could help manage complexity.
+ function constructSelectClause(fields, segments, metrics, resource) { + const selection = [...fields, ...segments, ...metrics]; + return `SELECT ${selection.join(", ")} FROM ${resource}`; + } + + function constructWhereClause(objectFilter, resource) { + return objectFilter ? ` WHERE ${resource}.id IN (${objectFilter.join(", ")})` : ""; + } + + function constructOrderByClause(orderBy, direction, resource) { + return orderBy ? ` ORDER BY ${resource}.${orderBy} ${direction}` : ""; + } + - const selection = [...fields, ...segments, ...metrics]; - let query = `SELECT ${selection.join(", ")} FROM ${resource}`; - if (orderBy && direction) { - query += ` ORDER BY ${`${resource}.${orderBy}`} ${direction}`; - } - if (limit) { - query += ` LIMIT ${limit}`; - } - if (objectFilter) { - query += ` WHERE ${resource}.id IN (${objectFilter.join(", ")})`; - } + let query = constructSelectClause(fields, segments, metrics, resource) + + constructWhereClause(objectFilter, resource) + + constructOrderByClause(orderBy, direction, resource); + if (limit) { + query += ` LIMIT ${limit}`; + }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- components/google_ads/actions/add-contact-to-list-by-email/add-contact-to-list-by-email.mjs (1 hunks)
- components/google_ads/actions/create-customer-list/create-customer-list.mjs (1 hunks)
- components/google_ads/actions/create-report/create-report.mjs (4 hunks)
- components/google_ads/actions/send-offline-conversion/send-offline-conversion.mjs (1 hunks)
- components/google_ads/common/queries.mjs (1 hunks)
- components/google_ads/google_ads.app.mjs (1 hunks)
- components/google_ads/package.json (1 hunks)
- components/google_ads/sources/new-campaign-created/new-campaign-created.mjs (1 hunks)
- components/google_ads/sources/new-lead-form-entry/new-lead-form-entry.mjs (1 hunks)
Files skipped from review due to trivial changes (6)
- components/google_ads/actions/add-contact-to-list-by-email/add-contact-to-list-by-email.mjs
- components/google_ads/actions/create-customer-list/create-customer-list.mjs
- components/google_ads/actions/send-offline-conversion/send-offline-conversion.mjs
- components/google_ads/package.json
- components/google_ads/sources/new-campaign-created/new-campaign-created.mjs
- components/google_ads/sources/new-lead-form-entry/new-lead-form-entry.mjs
Additional comments not posted (2)
components/google_ads/actions/create-report/create-report.mjs (1)
46-69: Well-implemented dynamic filtering for report generation.The
objectFilterproperty is well-implemented, allowing dynamic fetching of filter options based on user input. It enhances the flexibility of the report generation feature by enabling users to specify which objects to include in the report.components/google_ads/google_ads.app.mjs (1)
190-196: Consistent implementation of resource listing.The
listResourcesmethod is implemented consistently with other similar methods in this file. It effectively uses thesearchmethod to execute the query generated byQUERIES.listResources.
| function listResources(resource) { | ||
| const name = resource === "customer" | ||
| ? "descriptive_name" | ||
| : "name"; | ||
| return `SELECT ${resource}.id, ${resource}.${name} FROM ${resource}`; | ||
| } |
There was a problem hiding this comment.
Simplify the conditional logic in listResources.
The function uses a ternary operator to determine the field name based on the resource type. Consider using a dictionary to map resource types to their respective field names for improved readability and maintainability.
- const name = resource === "customer"
- ? "descriptive_name"
- : "name";
+ const fieldNames = {
+ customer: "descriptive_name",
+ default: "name",
+ };
+ const name = fieldNames[resource] || fieldNames.default;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function listResources(resource) { | |
| const name = resource === "customer" | |
| ? "descriptive_name" | |
| : "name"; | |
| return `SELECT ${resource}.id, ${resource}.${name} FROM ${resource}`; | |
| } | |
| function listResources(resource) { | |
| const fieldNames = { | |
| customer: "descriptive_name", | |
| default: "name", | |
| }; | |
| const name = fieldNames[resource] || fieldNames.default; | |
| return `SELECT ${resource}.id, ${resource}.${name} FROM ${resource}`; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- components/google_ads/actions/create-report/common-constants.mjs (1 hunks)
- components/google_ads/common/resources/ad.mjs (1 hunks)
- components/google_ads/common/resources/adGroup.mjs (1 hunks)
- components/google_ads/common/resources/campaign.mjs (2 hunks)
- components/google_ads/common/resources/customer.mjs (2 hunks)
Additional comments not posted (5)
components/google_ads/actions/create-report/common-constants.mjs (1)
1-55: Excellent addition of predefined date range options.The introduction of
DATE_RANGE_OPTIONSprovides a clear and structured way to handle date ranges in reports, which enhances usability and maintainability. This is a solid implementation following best practices for constant definitions.components/google_ads/common/resources/adGroup.mjs (1)
Line range hint
1-57: Review of adGroup resource constants.The constants defined for
fields,segments, andmetricsare comprehensive and cover a wide range of attributes relevant to ad groups. This will facilitate detailed data fetching and manipulation in ad group-related operations. It's good to see such a detailed and thought-out list, which should help in maintaining and scaling the ad group functionalities.components/google_ads/common/resources/customer.mjs (1)
Line range hint
1-65: Review of customer resource constants.The constants for
fields,segments, andmetricsrelated to the customer resource are well-defined and comprehensive. This setup ensures that all necessary customer data points can be accessed and utilized effectively, supporting robust customer management and analysis capabilities.components/google_ads/common/resources/campaign.mjs (1)
Line range hint
1-68: Review of campaign resource constants.The constants for
fields,segments, andmetricsrelated to the campaign resource are thoroughly defined, ensuring a comprehensive approach to campaign data management. This will significantly aid in the detailed and effective management of campaigns across various dimensions and metrics.
[APROVED]components/google_ads/common/resources/ad.mjs (1)
Line range hint
169-180: Segments array updated to remove time-related fields.The removal of specific time-related segments such as "date", "day_of_week", etc., is consistent with the PR's objectives to streamline data processing. However, it's crucial to ensure that these changes do not adversely impact other functionalities that might depend on these segments.
#!/bin/bash # Description: Verify that the removal of time-related segments does not break existing functionalities. # Test: Search for usage of removed segments. Expect: No occurrences that could lead to runtime errors. rg --type mjs --glob '*.*mjs' 'date|day_of_week|month|month_of_year|quarter|week|year'
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- components/google_ads/actions/create-report/create-report.mjs (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- components/google_ads/actions/create-report/create-report.mjs
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- components/google_ads/actions/create-report/create-report.mjs (5 hunks)
- components/google_ads/common/queries.mjs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- components/google_ads/actions/create-report/create-report.mjs
- components/google_ads/common/queries.mjs
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- components/google_ads/actions/create-report/create-report.mjs (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- components/google_ads/actions/create-report/create-report.mjs
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- components/google_ads/actions/create-report/create-report.mjs (5 hunks)
- components/google_ads/common/resources/ad.mjs (1 hunks)
- components/google_ads/common/resources/adGroup.mjs (1 hunks)
- components/google_ads/common/resources/campaign.mjs (2 hunks)
- components/google_ads/common/resources/customer.mjs (2 hunks)
Files skipped from review as they are similar to previous changes (5)
- components/google_ads/actions/create-report/create-report.mjs
- components/google_ads/common/resources/ad.mjs
- components/google_ads/common/resources/adGroup.mjs
- components/google_ads/common/resources/campaign.mjs
- components/google_ads/common/resources/customer.mjs
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- components/google_ads/actions/create-report/create-report.mjs (5 hunks)
- components/google_ads/common/queries.mjs (1 hunks)
- components/google_ads/google_ads.app.mjs (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- components/google_ads/actions/create-report/create-report.mjs
- components/google_ads/google_ads.app.mjs
Additional comments not posted (1)
components/google_ads/common/queries.mjs (1)
97-110: Refactor to simplify conditional logic and address potential security risk.The function
listResourcesuses ternary operators to determine fields based on the resource type. Consider using a dictionary for cleaner and more maintainable code. Additionally, ensure thequeryparameter is sanitized to prevent SQL injection.- const name = resource === "customer" - ? "descriptive_name" - : "name"; - const fieldResource = resource === "ad_group_ad" - ? "ad_group_ad.ad" - : resource; + const fieldNames = { + customer: "descriptive_name", + ad_group_ad: "ad_group_ad.ad", + default: "name", + }; + const name = fieldNames[resource] || fieldNames.default; + const fieldResource = fieldNames[resource] || resource; + let result = `SELECT ${fieldResource}.id, ${fieldResource}.${name} FROM ${resource}`; + if (query) { + const safeQuery = sanitize(query); // Ensure `sanitize` properly escapes SQL characters + result += ` WHERE ${fieldResource}.${name} LIKE '%${safeQuery}%'`; + }To verify the sanitization, please ensure that a
sanitizefunction exists and is robust enough to handle SQL injection attempts.
dylburger
added
the
triaged
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- components/google_ads/actions/create-report/create-report.mjs (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- components/google_ads/actions/create-report/create-report.mjs
|
Hello everyone, I have tested this PR and there're some test cases failed or needed improvement. Please check the test report below for more information |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- components/google_ads/actions/create-report/create-report.mjs (5 hunks)
- components/google_ads/common/queries.mjs (3 hunks)
- components/google_ads/common/resources/ad.mjs (4 hunks)
- components/google_ads/common/resources/adGroup.mjs (4 hunks)
- components/google_ads/common/resources/campaign.mjs (5 hunks)
- components/google_ads/common/resources/customer.mjs (5 hunks)
- components/google_ads/common/utils.mjs (1 hunks)
- components/google_ads/sources/new-campaign-created/new-campaign-created.mjs (2 hunks)
Files skipped from review as they are similar to previous changes (5)
- components/google_ads/common/queries.mjs
- components/google_ads/common/resources/ad.mjs
- components/google_ads/common/resources/adGroup.mjs
- components/google_ads/common/resources/customer.mjs
- components/google_ads/sources/new-campaign-created/new-campaign-created.mjs
Additional comments not posted (12)
components/google_ads/common/utils.mjs (1)
34-39: LGTM!The
getOptionfunction is simple and clear. It takes a label and a prefix, returning an object with the label and a value that concatenates the prefix and the label.components/google_ads/actions/create-report/create-report.mjs (8)
7-7: LGTM!The import statement for
DATE_RANGE_OPTIONSis necessary for the newdateRangeproperty.
21-21: LGTM!The version update to "0.1.0" reflects the significant changes to the functionality.
47-76: LGTM!The
objectFilterproperty allows filtering objects for the report, enhancing the functionality.
77-96: LGTM!The
dateRangeproperty allows selecting a date range for the report, enhancing the functionality.
99-103: LGTM!The
fieldsproperty allows selecting fields for the report, enhancing the functionality.
107-113: LGTM!The
segmentsproperty allows selecting segments for the report, enhancing the functionality.
118-122: LGTM!The
metricsproperty allows selecting metrics for the report, enhancing the functionality.
201-237: LGTM!The
buildQuerymethod has been updated to handle object filtering, date range selection, and query construction, enhancing the functionality.components/google_ads/common/resources/campaign.mjs (3)
1-1: LGTM!The import statement for
getOptionis necessary for the new functionality.
98-98: LGTM!The
fieldsarray has been updated to use thegetOptionfunction, enhancing the data processing logic.
131-131: LGTM!The
segmentsarray has been updated to use thegetOptionfunction, enhancing the data processing logic.
|
Hello everyone, I have tested this PR and there're some test cases failed or needed improvement. Please check the test report below for more information |
|
@vunguyenhung I cannot reproduce that error, but I believe it's related to the previous default value ("date") remaining in the optional prop after updating the component (the new default value is "segments.date"). Note: that default value is not present in the live version, so this wouldn't occur to users updating their components after releasing. Btw, your setup gives me a business rule error where "metrics cannot be requested for a manager account", although this is a dynamic business rule on Google's end and not an error in the component (you'll have to either select a managed account, or not request metrics). |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- components/google_ads/actions/create-report/create-report.mjs (5 hunks)
- components/google_ads/common/utils.mjs (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- components/google_ads/actions/create-report/create-report.mjs
- components/google_ads/common/utils.mjs
|
Hi @GTFalcao, I think it's fine if it's business logic error. I faced the same issue, which I assume all other cases works |
|
Hi everyone, all test cases are passed! Ready for release! Test report |
|
/approve |
Summary by CodeRabbit
New Features
Bug Fixes
Improvements