Add script to check tracking configuration

[moiikana]

• in the past it happens that we forgot to adjust all the test numbers/start dates for banners in the campaigns_info.toml file
  --> This commit adds a node command that runs a script to check the configuration file for consistency on those 2 issues.

• cleans up some codestyle in related files

• creates a "tools" folder for add-on scripts

• updates documentation

Tip

you could include npm run check-tracking-conf in your local git hooks (e.g. pre-commit)

[Sperling-0]

I tried it out. I like the feature. Having colorful response would be very nice (Green for Success, and Red for Faulty tracking config).

[Sperling-0]

I tried it out. I like the feature. Having colorful response would be very nice (Green for Success, and Red for Faulty tracking config).

Note

I added a commit for the colored response. I hope you don't mind.
Feel free to discard it if you don't like it.

[moiikana]

I tried it out. I like the feature. Having colorful response would be very nice (Green for Success, and Red for Faulty tracking config).

would be nice to give it a more "blocking" and hinting character, but I'm currently not sure if it's possible to properly achieve that with javascript scripts in bash

[gbirke]

Thank you for this valuable tool and the restructuring!

As a general approach in the code of the script, I'd prefer if we did not do small-scale regex matching and then checking strings. Instead, we should create additional value objects in campaign_config_types.ts for campaign names and tracking values that parse the strings in their constructor (and throw errors if the pattern is "broken") to make the patterns and contents very explicit and semantic. This will also help make the comparisons more meaningful.

Example code for the script

const campaignName = new CampaignName(configValues.campaign)
const ctrlTracking = new KeywordTrackingCode(configValues.ctrl.tracking)
const varTracking = new KeywordTrackingCode(configValues.var.tracking)
const campaignTracking = new CampaignTrackingCode(configValues.tracking)
if ( campaignName.testNumber !== varTracking.testNumber ||  campaignName.testNumber !== ctrlTracking.testNumber) {
  // error logging here
}
// Another example where the "business logic" for comparison is moved into the value objects (tell-don't-ask)
if ( !ctrlTracking.matchesDate( varTracking) || !ctrlTracking.matchesDate( campaignTracking ) ) {
 // error logging here
}

It would be even better to move the object creation into the Campaign class with getters:

class Campaign {
    // Properties here ...

    get campaignTracking() {
        return new CampaignTrackingCode( this.tracking);
    }
}

But then you'd have to change or duplicate campaignInfoToCampaignConfig in TypeScript to create the proper classes, so that's entirely optional

[moiikana]

const campaignName = new CampaignName(configValues.campaign)
const ctrlTracking = new KeywordTrackingCode(configValues.ctrl.tracking)
const varTracking = new KeywordTrackingCode(configValues.var.tracking)
const campaignTracking = new CampaignTrackingCode(configValues.tracking)
if ( campaignName.testNumber !== varTracking.testNumber || campaignName.testNumber !== ctrlTracking.testNumber) {
// error logging here
}
// Another example where the "business logic" for comparison is moved into the value objects (tell-don't-ask)
if ( !ctrlTracking.matchesDate( varTracking) || !ctrlTracking.matchesDate( campaignTracking ) ) {
// error logging here
}

@gbirke
I would have assumed that creating 4 objects ( each of the constructors has to parse in each loop the ""org-mob09-241118-ctrl" input string for example (to get the testNumber) + then doing the string comparisons slows this loop down terribly, but I can also do that.

[gbirke]

Some last suggestions for making the "business logic" side more explicit & separating it from the output and program flow. But I'm already approving as "good enough".

Thank your for your comment about performance of classes vs simple regexes. It's always a tradeoff between performance and readability. maintainability and developer experience. In our case the performance, even with a loop, is not too terrible, since we'll only have about 8 iterations (=number of channels).
But I must admit I have no hard-and-fast rules for making the tradeoff

[gbirke]

When I described these objects I was thinking of "simple value objects", without logging side effects and dependencies on console libraries. Could you throw exceptions instead and catch them when constructing? It'll be even less code since you have the process.exit in one place.

[gbirke]

Suggested change

	const testNumberRegexResult = campaignName.match( /_(\d{2})/ );
	const testNumberRegexResult = campaignName.match( /_(\d{2})$/ );

You could also do a split and use the parts, to make this class more applicable for future uses and would allow for more thorough checking.

const campaignParts = campaignName.trim().split( '_' )`;
if ( campaignParts.length < 5 ) {
   throw new Error( `Campaign name '${campaignName}' does not follow pattern, must have at least 5 parts, separated with underscores` );
}
const campaignNumber = campaignParts.slice( -1 );
if ( !/^\d{2}$/.test(campaignNumber) ) {
    throw new Error( `Cannot parse test number "${ campaignNumber }" of campaign "${ campaignName }". Test number must be a zero-padded, 2-digit numeric string` );
}
// const campaignYear = campaignParts[0].substr(1);
// etc ...

[gbirke]

Here you could also split at the - sign and examine the length and different parts with ^ and $ regex chars. I always get a bit uneasy when I see a Regex without these "string boundaries" because it theoretically it could match anywhere in the string. Also, using boundaries makes the regex much faster because the regex engine does not have to try out all possible matches