Skip to content

Conversation

@nsineok
Copy link
Contributor

@nsineok nsineok commented May 4, 2025

Checklist before requesting a review

  • Subject includes ticket number as [#123] Verb in past tense.
  • Ticket number #123 added to description
  • Added context in Changed section
  • Self-reviewed code and commented in commented complex areas.
  • Added tests for fix/feature.
  • Relevant tests run and passed locally.

Changed

  1. Added the tracking.feature file with a behat scenario for testing GA presence.

Screenshots

Summary by CodeRabbit

  • Tests
    • Added tests to verify conditional display of the Google Analytics 4 script based on user authentication status.
  • Chores
    • Updated configuration to manage user roles and event ordering for tracking purposes.

@coderabbitai
Copy link

coderabbitai bot commented May 4, 2025

Walkthrough

A new Behat feature test file was added to verify the conditional injection of the Google Analytics 4 script on the homepage based on user authentication status. It tests that an anonymous user receives the GA4 script in the response, while a logged-in user with the "authenticated" role does not. Additionally, the Google Tag Manager configuration was updated to add a dependency on the "user" module, introduce a "user_role" condition for anonymous users, and reorder event definitions including adding placeholders for "search" and "custom" events. No exported or public code entities were changed.

Changes

File(s) Change Summary
tests/behat/features/tracking.feature Added a new Behat feature file testing presence/absence of GA4 script on homepage for anonymous vs authenticated users.
config/default/google_tag.container*.yml Updated config to add "user" module dependency, introduced "user_role" condition for anonymous users, reordered events, and added "search" and "custom" event placeholders.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant WebApp
    participant GoogleTagManager

    User->>WebApp: Visit homepage (anonymous)
    WebApp-->>User: Serve homepage HTML (includes GA4 script)
    User->>GoogleTagManager: (Script loads) Request gtag.js
    GoogleTagManager-->>User: Serve gtag.js

    Note over User,WebApp: For authenticated users:
    User->>WebApp: Visit homepage (authenticated)
    WebApp-->>User: Serve homepage HTML (no GA4 script)
Loading

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@nsineok nsineok marked this pull request as ready for review May 4, 2025 00:03
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🔭 Outside diff range comments (1)
tests/behat/features/tracking.feature (1)

10-11: ⚠️ Potential issue

Fix syntax error: missing trailing quote

There appears to be a syntax error at the end of the file - the last line is missing a closing double quote.

-    And the response should contain "https://www.googletagmanager.com/gtag/js?id=G-"
+    And the response should contain "https://www.googletagmanager.com/gtag/js?id=G-"
11
📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite

📥 Commits

Reviewing files that changed from the base of the PR and between fec569a and 26af0da.

📒 Files selected for processing (1)
  • tests/behat/features/tracking.feature (1 hunks)
🔇 Additional comments (1)
tests/behat/features/tracking.feature (1)

1-5: Feature declaration looks good!

The feature is clearly defined with appropriate tags and follows the standard user story format.

Comment on lines 6 to 10
@api @javascript
Scenario: Check Google Analytics 4 script on homepage
Given I am an anonymous user
When I am on the homepage
And the response should contain "https://www.googletagmanager.com/gtag/js?id=G-"
Copy link

@coderabbitai coderabbitai bot May 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Good implementation of the GA4 tracking check

The scenario correctly verifies the presence of the Google Analytics 4 script on the homepage. Some considerations for future enhancements:

  1. You might want to check for the complete tracking ID if it's known (e.g., "G-XXXXXXXXXX") rather than just the URL prefix.
  2. If GA tracking should be present site-wide, consider adding tests for other important pages.
  3. For more comprehensive testing, you could verify the proper initialization of GA4 (checking for the gtag function or data layer).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test that tag is not set for logged in users

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexSkrypnyk Here's a test scenario that verifies the Google Analytics tag is not set for logged-in users:

@api @javascript
Scenario: Google Analytics 4 script should not be present for logged in users
  Given I am logged in as a user with the "authenticated" role
  When I am on the homepage
  Then the response should not contain "https://www.googletagmanager.com/gtag/js?id=G-"

You can add this scenario to the existing tracking.feature file right after the anonymous user test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@nsineok nsineok requested a review from AlexSkrypnyk May 4, 2025 00:09
@AlexSkrypnyk AlexSkrypnyk temporarily deployed to PR-31 May 4, 2025 00:27 Inactive
As a site user
I want to ensure Google Analytics script is present on the page

@api @javascript
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nsineok does this need to be a JS test ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexSkrypnyk, yes it does because GA script is injected via another JS file. modules/contrib/google_tag/js/gtag.js

Comment on lines 6 to 10
@api @javascript
Scenario: Check Google Analytics 4 script on homepage
Given I am an anonymous user
When I am on the homepage
And the response should contain "https://www.googletagmanager.com/gtag/js?id=G-"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test that tag is not set for logged in users

@AlexSkrypnyk AlexSkrypnyk added the Requires more work Pull request was reviewed and reviver(s) asked to work further on the pull request label May 4, 2025
… GA container - GA works only for anonymous.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite

📥 Commits

Reviewing files that changed from the base of the PR and between 26af0da and 47a1387.

📒 Files selected for processing (2)
  • config/default/google_tag.container.G-LCQ5CZ3H3W.680a169398dd50.88125398.yml (2 hunks)
  • tests/behat/features/tracking.feature (1 hunks)
🔇 Additional comments (4)
config/default/google_tag.container.G-LCQ5CZ3H3W.680a169398dd50.88125398.yml (2)

4-6: Appropriate module dependency added.

Good addition of the "user" module dependency, which is required for the user role condition implementation below.


15-22: Well-implemented user role condition.

The condition properly targets anonymous users only, which aligns with privacy best practices by not tracking authenticated users. The context mapping is correctly configured to use the current user context.

tests/behat/features/tracking.feature (2)

6-10: Well-implemented test for anonymous users.

This scenario correctly tests that the Google Analytics script is present for anonymous users. The @javascript tag is appropriate since the GA script is injected via JavaScript.


12-16: Correctly implemented test for authenticated users.

This scenario properly verifies that the Google Analytics script is not present for authenticated users, as requested in the previous review comments. This is an important privacy consideration and complements the first test case.

Comment on lines +26 to +27
search: { }
custom: { }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Empty placeholder events added.

The "search" and "custom" empty event placeholders have been added. Consider adding a comment about their intended future use or create tickets to implement these events if they're needed.

Comment on lines +1 to +5
@tracking @google
Feature: Google Analytics is injected
As a site user
I want to ensure Google Analytics script is present on the page

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Well-structured feature description.

The feature is properly tagged and includes a clear description of the functionality being tested. Consider enhancing the description to mention the conditional nature of the tracking (i.e., only for anonymous users).

 @tracking @google
 Feature: Google Analytics is injected
   As a site user
-  I want to ensure Google Analytics script is present on the page
+  I want to ensure Google Analytics script is present for anonymous users and absent for authenticated users
📝 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.

Suggested change
@tracking @google
Feature: Google Analytics is injected
As a site user
I want to ensure Google Analytics script is present on the page
@tracking @google
Feature: Google Analytics is injected
As a site user
I want to ensure Google Analytics script is present for anonymous users and absent for authenticated users

@nsineok nsineok requested a review from AlexSkrypnyk May 4, 2025 08:46
@AlexSkrypnyk AlexSkrypnyk merged commit 36a95d7 into develop May 4, 2025
7 checks passed
@AlexSkrypnyk AlexSkrypnyk deleted the feature/ga_gtag_test branch May 4, 2025 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Requires more work Pull request was reviewed and reviver(s) asked to work further on the pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants