Skip to content

Conversation

@hdmariani
Copy link
Contributor

No description provided.

@hdmariani hdmariani reopened this Jan 21, 2026
template.js Outdated
const url = eventData.page_location || getRequestHeader('referer');
if (url && url.lastIndexOf('https://gtm-msr.appspot.com/', 0) === 0) return true;

if (getType(data.baseUrl) !== 'string' || getType(data.apiKey) !== 'string') return true;

Choose a reason for hiding this comment

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

No need to add it to all templates. Let the API response inform the user.

Copy link

@giovaniortolani giovaniortolani left a comment

Choose a reason for hiding this comment

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

Some changes needed. Check this commit with them.

template.js Outdated

if (getType(data.baseUrl) !== 'string' || getType(data.apiKey) !== 'string') return true;

if (!data.sender && !data.type.match('ID|EXTERNAL_ID|EMAIL|PHONE')) return true;
Copy link

@giovaniortolani giovaniortolani Jan 22, 2026

Choose a reason for hiding this comment

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

Use one of the two

!data.type.match('^(ID|EXTERNAL_ID|EMAIL|PHONE)$')
// or
['ID', 'EXTERNAL_ID', 'EMAIL', 'PHONE'].indexOf(data.type) === -1

Add a log message stating why the variable failed.

Choose a reason for hiding this comment

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

I added this validation to the UI. If users still pass an undefined variable, the API response will take care of the warning. We can remove the validation in the code.

Image

template.js Outdated
Comment on lines 141 to 143
apiBaseUrl = apiBaseUrl.match(protocolRegex)
? apiBaseUrl.replace(protocolRegex, 'https://')
: 'https://' + apiBaseUrl;

Choose a reason for hiding this comment

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

No need to worry about this. Consider https:// unless stated otherwise.

Choose a reason for hiding this comment

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

const apiBaseUrl = 'https://' + data.baseUrl.replace('https://', '').replace('http://', '');

template.js Outdated
? apiBaseUrl.replace(protocolRegex, 'https://')
: 'https://' + apiBaseUrl;

requestConfig = {

Choose a reason for hiding this comment

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

const requestConfig = { ... }

template.js Outdated
url: apiBaseUrl + apiPath + apiQueries,
options: {
headers: {
'Content-Type': 'application/json',

Choose a reason for hiding this comment

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

Can be removed because no request body is being sent.

template.js Outdated

function personLookupHandler(method) {
if (method === 'requestMethod') return 'GET';
if (method === 'path') return '/people/2/persons?';

Choose a reason for hiding this comment

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

'/people/2/persons' without '?' because this character is part of the query

template.js Outdated
templateDataStorage.setItemCopy(cacheKeyTimestamp, now);
}
return createReturningObject(objectToStore);
} else if (result.statusCode === 404) {

Choose a reason for hiding this comment

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

Use only else without any other conditions because the API can return other error status codes.

template.js Outdated
Comment on lines 90 to 96
log({
Name: 'InfobipLookup',
Type: 'Message',
EventName: chosenApi,
Message: 'Request failure',
Reason: parsedBody.errorMessage
});

Choose a reason for hiding this comment

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

No need to log again. The response log will already log basically the same thing.

template.js Outdated
Comment on lines 81 to 88
let objectToStore = {};
objectToStore = parsedBody;

if (data.storeResponse) {
templateDataStorage.setItemCopy(cacheKey, objectToStore);
templateDataStorage.setItemCopy(cacheKeyTimestamp, now);
}
return createReturningObject(objectToStore);

Choose a reason for hiding this comment

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

if (result.statusCode === 200) {
        const parsedBody = JSON.parse(result.body || '{}');
        if (data.storeResponse) {
          templateDataStorage.setItemCopy(cacheKey, parsedBody);
          templateDataStorage.setItemCopy(cacheKeyTimestamp, now);
        }
        return createReturningObject(parsedBody);
}

template.js Outdated
Comment on lines 39 to 58
data.expirationTime && makeInteger(data.expirationTime) * 60 * 60 * 1000;
const now = getTimestampMillis();

if (data.storeResponse) {
let cachedValues = templateDataStorage.getItemCopy(cacheKey);
const cachedValueTimestamp = templateDataStorage.getItemCopy(cacheKeyTimestamp);

if (data.expirationTime) {
if (
cachedValueTimestamp &&
now - makeInteger(cachedValueTimestamp) >= cacheExpirationTimeMillis
) {
cachedValues = '';
templateDataStorage.removeItem(cacheKey);
templateDataStorage.removeItem(cacheKeyTimestamp);
}
}
if (cachedValues)
return Promise.create((resolve) => resolve(createReturningObject(cachedValues)));
}

Choose a reason for hiding this comment

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

I improved this part a bit.

  const cacheExpirationTimeMillis = makeInteger(data.expirationTime || 12) * 60 * 60 * 1000;

  // ...

  if (data.storeResponse) {
    const cachedValues = templateDataStorage.getItemCopy(cacheKey);
    const cachedValueTimestamp = templateDataStorage.getItemCopy(cacheKeyTimestamp);

    if (
      cachedValueTimestamp &&
      now - makeInteger(cachedValueTimestamp) >= cacheExpirationTimeMillis
    ) {
      templateDataStorage.removeItem(cacheKey);
      templateDataStorage.removeItem(cacheKeyTimestamp);
    } else if (cachedValues) {
      return Promise.create((resolve) => resolve(createReturningObject(cachedValues)));
    }
  }

template.tpl Outdated
Comment on lines 26 to 28
"brand": {
"displayName": "stape.io"
}

Choose a reason for hiding this comment

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

I'm not sure if this is accepted by Google because the UI doesn't have a field to show it.

@giovaniortolani giovaniortolani dismissed their stale review January 22, 2026 21:07

Changes already done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants