Problem/Motivation

Add a textarea to exclude certain paths (with useful defaults).

e.g. currently tracks /contextual/render maybe because we're using BigPipe?

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork page_analytics-3571787

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

anybody created an issue. See original summary.

firflant’s picture

firflant
Location Poland
commented
Assigned: Unassigned » firflant

That's a clever idea - it makes the module more personalizable and it follows the general convention. I will include it in a next release.

firflant’s picture

firflant
Location Poland
commented
Version: 1.x-dev » 1.0.0-beta5
Status: Active » Needs review

Excluding criteria configuration in added to version beta5 https://www.drupal.org/project/page_analytics/releases/1.0.0-beta5

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thanks @firflant

I'd vote to include typical exclusion paths like:

/contextual/render
/admin/*

by default?

firflant’s picture

firflant
Location Poland
commented

admin pages should be excluded by default - textfield should be autofilled after installing the module from scratch, but latest version also includes an update that sets this field, if empty. Any other paths that should be excluded?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thanks @firflant

In posthog module we're using:

/admin
/admin/*
/batch
/batch/*
/node/add
/node/*/*

But I guess you're already using
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Routing%2...
for admin routes?

firflant’s picture

firflant
Location Poland
commented
Status: Needs review » Needs work

I don't. And maybe we can stay with that one form of excluding paths, just for a transparency? The list that you provided is very handy. I will update the defaults soon.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Well the difference is that the list isn't that easy to maintain for all cases but should be kept for manual additions IMHO.

Admin routes are for example also node/*/* - (typically it's anything rendered in the admin theme).
But to be fair, similar to node/*/* there are paths for all kinds of entities like users, taxonomy terms, commerce products in contrib etc. plus ajax routed and others... so it might make sense to split this into the human defined list and the technical admin routes part... I think I'd try that.

Instead I'd personally remove the authenticated user checkbox and instead add a fieldset and list of roles to include / exclude like other similar modules do (or use the Context API). Maybe just sleep a night about it :)

And these are just my thoughts, you're the maintainer, you decide.

firflant’s picture

firflant
Location Poland
commented
Status: Needs work » Active
firflant’s picture

firflant
Location Poland
commented

You are right that this list of admin urls is unsafe. Now it seems that it is better to replace excluded paths textfield with an "Excude admin paths" checkbox.

Replacing authenticated checkbox with a roles selector - it might be worth to have it before the 1.0.0 release.

How about having a "Excluding" formgroup just like this:

EXCLUDING

[x] Exclude admin paths

Exclude following roles:
[ ] Authenticated
[ ] Moderator
[x] Admin
[ ] etc.

Maybe there is no need to bloat the page responses with additional page checks. I do not find it useful in this scenario.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

#10 is good, but I'd definitely keep the textarea to allow additional exclusions based on paths. This way users have a custom way to exclude paths from front - or backend if needed.

firflant’s picture

firflant
Location Poland
commented
Status: Active » Needs work

Final shape to develop:

EXCLUDING

Exclude admin paths (checkbox)

Exclude certain paths (textarea)

Exclude following roles (checkboxes):
- Authenticated
- Moderator
- Admin
- etc.
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Nice!! +1

firflant’s picture

firflant
Location Poland
commented
Version: 1.0.0-beta5 » 1.0.0-beta6
Status: Needs work » Needs review

Added in beta6 https://www.drupal.org/project/page_analytics/releases/1.0.0-beta6
Have a look

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

@firflant thanks. Maybe in the future let's better use MRs and review them prior to release?

Otherwise it might be hard to undo things already released.

But again, you're the maintainer, you decide. Just my 2 cents. :)

firflant’s picture

firflant
Location Poland
commented

Sure thing. If you have any suggestions to improve the existing flow (eg branching strategy, release strategy etc), feel free to share. That's my first project on drupal.org and I look forward to understand the standards there.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thanks @firflant great work, just tried it!

I created a MR with some minor changes to make the text clearer.
Additionally I was wondering if we should remove

unset($roles[RoleInterface::ANONYMOUS_ID]);

to also allow anonymous users to be selected, maybe there are cases for that e.g. in intranet? (Quite sure it's still an edge-case, just a thought to leave it to the site owner.)

anybody opened merge request !4

firflant’s picture

firflant
Location Poland
commented
Status: Needs review » Needs work

@anybody description change looks good. I haven't seen the use case for an anonymous role, but you are right about the intranet sites. We can turn the anonymous role on. Do you think you can do that in the same MR?

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs work » Needs review

@firflant yes, I did that!

firflant’s picture

firflant
Location Poland
commented
Status: Needs review » Reviewed & tested by the community

Available configuration options are optimal now. Marking issue as done.

firflant’s picture

firflant
Location Poland
commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • firflant committed aa45bb45 on 1.x authored by anybody 
    feat: #3571787 Add a textarea to exclude certain paths (with useful...
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Nice!! Thank you @firflant!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.