[Bug]: v3 - Service callbacks onAccept & onReject are not called unless both are defined

[techfg]

Expected Behavior

If onAccept and/or onReject are defined, they should always be called when the service enters the respective state

Current Behavior

1. onAccept defined / onReject not defined - On accept is called the first time, but after rejecting and re-accepting, onAccept is not called (because service._enabled does not get set back to false)
2. onAccept not defined / onReject defined - onReject is never called (because service._enabled does not get set to true)

Steps to reproduce

1. Create a config with a service with onAccept defined but onReject not defined
2. Accept the service - onAccept will be fired
3. Reject the service
4. Accept the service - onAccept does not fire

Proposed fix or additional info.

Remove the check isFunction checks for onAccept/onReject to within the corresponding if...else blocks

Version

3

On which browser do you see the issue?

Firefox, Chrome, Microsoft Edge

[orestbida]

Hi @techfg, to me the current behavior is the more appropriate one.

A service can be enabled/accepted only if it is (now) disabled/rejected.
A service can be disabled/rejected only if it is (now) enabled/accepted.

1. There is no point in re-executing the onAccept callback since the service cannot be disabled (in the current session).
2. There is no point in executing the onReject callback since the service cannot be enabled (ever).

[techfg]

Hi @orestbida -

Thanks for reviewing! I believe the current behavior would allow a service to be enabled/accepted & disabled/rejected multiple times in a session , possibly I'm misunderstanding? Possibly you're referring to the scripts executing themselves vs. the callbacks?

For example, if you define onAccept & onReject callbacks, and then toggle a service multiple times back and forth, onAccept & onReject are called for each toggle (see here). I believe this is expected behavior. The issue I noticed is that if one (or the other) is not defined, then the defined callback is not invoked on every toggle to its respective state.

A use case would be Google Tag Manager with Consent Mode enabled - A user could accept or reject cookies multiple times within a session. On each transition the corresponding consent update needs to be sent to GTM.

[orestbida]

Yes, services are meant to be disabled/enabled multiple times in the current session, but only if there is an implementation of the 2 callbacks, that (should) do something.

A service can be enabled/accepted only if it is (now) disabled/rejected.
A service can be disabled/rejected only if it is (now) enabled/accepted.

This is what I mean: if the car is already running, you can't "re run/start" it again, unless you turn it off first. If the car is already off, you can't turn it off again.

By not implementing one of the 2 callback functions, you are (likely) not respecting the user's choice.

A use case would be Google Tag Manager with Consent Mode enabled - A user could accept or reject cookies multiple times within a session. On each transition the corresponding consent update needs to be sent to GTM.

I agree, but you would achieve this with both callbacks implemented, or am I missing something?

[techfg]

Agreed, believe we are on the same page here, in theory, you should be using both or none, not one or the other (although I guess there could be situations where you only want to do certain things in one or the other case).

The main concern I saw was tracking internal state service._enabled - if this were to be used elsewhere (in the future), it would not have the correct value unless the user defines both onAccept & onReject. The service state is also tracked in globalObj._state._acceptedServices so possibly service._enabled is strictly for tracking the usage of onAccept/onReject?

If so, possibly just the typings need to be updated so that someone can't define one and not the other (or just the docs updated to making it clear on usage)?

Thanks!

[orestbida]

so possibly service._enabled is strictly for tracking the usage of onAccept/onReject?

Yes, the ._enabled internal prop. is used to keep track of currently enabled (executed) services so that they aren't re-executed multiple times (unless they are disabled first).

Every property starting with _ means that it's internal only, not meant to be set by the user.

[techfg]

Understood _ variables are private and not intended to be used by consumer/user - concern was if library internals utilized/relied on this variable elsewhere, it could lead to other issues depending on what service._enabled is defined to represent. Possibly it is just the name that threw me off since I took enabled as meaning the same thing as accepted but after discussion here, I think you intended enabled to be specific to tracking onAccept/onReject functionality.

Understood all your comments and think it really comes down to how onAccept/onReject are defined/expected to be used and of course, this is up to you. If current behavior is maintained, only thing I would recommend is changing the typings so that the consumer knows that neither or both should be defined to get expected behavior - the typings allow one or the other (which is how I stumbled upon this in the first place when testing our v3 :)).

Possibly instead of:

service: {
  onAccept?: () => void
  onReject?: () => void
}

it could be:

service: {
  callbacks?: {
    onAccept: () => void
    onReject: () => void
  }

[orestbida]

They were left as optional because you can define the onAccept/onReject behavior using script tags.

I agree with you that the proposed change would make things clearer, but at the same time it's also a breaking change that I'd like to avoid. We could avoid the breaking change, by allowing both ways, but that would clutter the configuration and potentially confuse users.

I consider v3 stable; the only thing preventing me from officially announcing v3 is the state of the playground page, it needs a few extra touches.

[techfg]

I hear you :)

Interesting about the script tags and the run on disable (!) - so you could have neither, one of, or both of "run on enable" and "run on disable" script tags and whichever of them are defined, they will execute every time the user accepts/rejects that category/service, correct?

[orestbida]

Scripts configured as services follow the same logic as internal services:

If user accepts the service => enable the related "onAccept" script.
If the service is already accepted (and thus the "onAccept" related script has already been executed) => can run the "onReject" script.
If the service has never been accepted by the user => the "onReject" script will never be executed.

There is however 1 crucial difference between services implemented as scripts, and those implemented through the config. object: script tags are only executed/enabled once (unlike internally configured services, they can be enabled/disabled multiple times).

This is a design choice inherited from v2, and was kept the same in v3 so that users coming from v2 would "feel at home". Script tags are generally adopted by beginners, since they are the simplest most common way of handling code.

Services were introduced as a bonus feature, in a later moment, as this level of granularity/control was not a requirement for a compliant consent solution.

[techfg]

@orestbida -

All makes sense now, appreciate you providing the details/background. I updated #594 reverting the scripts code back to the previous and updating the tests to align with the expected behavior that you've described.

Will leave to decide to close out the PR or merge if you think the tests are helpful.

Thanks again for your work!