Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CUSTESC-36595: Try to prioritize active zone over non-active ones #200

Merged
merged 2 commits into from
Mar 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/actions/zoneProvision.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import * as ActionTypes from '../constants/ActionTypes';
import { asyncZoneSetActiveZone } from './activeZone';
import { normalizeZoneGetAll } from '../constants/Schemas';
import { zoneFetch, zoneFetchSuccess } from './zones';
import { deduplicateOnActiveZones } from '../utils/utils';

/*
* Zone Provision actions still use reducers/zones.js as the reducer
Expand Down Expand Up @@ -136,7 +137,8 @@ function asyncSetHostAPIProvisionedDomainActive(domainName) {
zoneGetAll(function(error, response) {
if (response) {
dispatch(zoneFetchSuccess(response.body.result));
let normalizedZoneList = normalizeZoneGetAll(response.body.result);
const zoneList = deduplicateOnActiveZones(response.body.result);
let normalizedZoneList = normalizeZoneGetAll(zoneList);
dispatch(
asyncZoneSetActiveZone(normalizedZoneList.entities.zones[domainName])
);
Expand Down
7 changes: 6 additions & 1 deletion src/actions/zones.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,12 @@ export function asyncFetchZones() {
zoneGetAll(function(error, response) {
if (response) {
dispatch(zoneFetchSuccess(response.body.result));
if (response.body.result[0]) {
const activeZone = response.body.result.find(
zone => zone.status === 'active'
);
if (activeZone) {
dispatch(zoneSetActiveZoneIfEmpty(activeZone));
} else if (response.body.result[0]) {
dispatch(zoneSetActiveZoneIfEmpty(response.body.result[0]));
}
} else {
Expand Down
6 changes: 6 additions & 0 deletions src/reducers/zoneEntitlements.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as ActionTypes from '../constants/ActionTypes';
import { normalizeZoneEntitlements } from '../constants/Schemas';

const initialState = {
entities: {},
Expand All @@ -12,7 +13,12 @@ export function zoneEntitlementsReducer(state = initialState, action) {
isFetching: true
});
case ActionTypes.ZONE_ENTITLEMENTS_SUCCESS:
let normalizedZoneEntitlements = normalizeZoneEntitlements(
action.zoneEntitlements
);
let newEntities = Object.assign({}, state.entities);
newEntities[action.zoneId] =
normalizedZoneEntitlements.entities.entitlements;
return Object.assign({}, state, {
entities: newEntities,
isFetching: false
Expand Down
54 changes: 54 additions & 0 deletions src/test/utils/deduplicateZonesTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { deduplicateOnActiveZones } from '../../utils/utils';
import expect from 'expect';

describe('deduplicateZones', () => {
it('should not change a list of zones without any duplicate', () => {
expect(
deduplicateOnActiveZones([
{ id: '1', name: 'cloudflare.com', status: 'active' },
{ id: '2', name: 'blog.cloudflare.com', status: 'active' }
])
).toEqual([
{ id: '1', name: 'cloudflare.com', status: 'active' },
{ id: '2', name: 'blog.cloudflare.com', status: 'active' }
]);
});

it('should remove non-active zone in case of duplicates', () => {
expect(
deduplicateOnActiveZones([
{ id: '1', name: 'cloudflare.com', status: 'active' },
{ id: '2', name: 'blog.cloudflare.com', status: 'active' },
{ id: '3', name: 'cloudflare.com', status: 'purged' }
])
).toEqual([
{ id: '1', name: 'cloudflare.com', status: 'active' },
{ id: '2', name: 'blog.cloudflare.com', status: 'active' }
]);
});

it('should not remove duplicates when there is no active zone', () => {
expect(
deduplicateOnActiveZones([
{ id: '1', name: 'cloudflare.com', status: 'pending' },
{ id: '2', name: 'blog.cloudflare.com', status: 'active' },
{ id: '3', name: 'cloudflare.com', status: 'purged' }
])
).toEqual([
{ id: '1', name: 'cloudflare.com', status: 'pending' },
{ id: '2', name: 'blog.cloudflare.com', status: 'active' },
{ id: '3', name: 'cloudflare.com', status: 'purged' }
]);
});

it('should remove multiple duplicates when there is one active zone', () => {
expect(
deduplicateOnActiveZones([
{ id: '1', name: 'cloudflare.com', status: 'pending' },
{ id: '2', name: 'cloudflare.com', status: 'purged' },
{ id: '3', name: 'cloudflare.com', status: 'active' },
{ id: '4', name: 'cloudflare.com', status: 'moved' }
])
).toEqual([{ id: '3', name: 'cloudflare.com', status: 'active' }]);
});
});
28 changes: 28 additions & 0 deletions src/utils/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,31 @@ export function formatMessageForIntegration(

return formatMessage({ id: messageId });
}

// deduplicateOnActiveZones was introduced as a potential fix for CUSTESC-36595. The goal of this function is to
// deduplicate the list of zones returned by the Cloudflare API based on the status of the zone. This way, we hope to
// guarantee that when normalizing the list of zones, only the active zone would show up in case of duplicates. For
// instance, in the mentioned tickets, two zones were potentially returned: a purged as well as an active zone.
export function deduplicateOnActiveZones(zones) {
let filteredZones = [];
const isActive = z => z.status === 'active';
const isSameZone = (z1, z2) => z1.name === z2.name;

Choose a reason for hiding this comment

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

should we do a falsy check here too? z1.name && z1.name === z2.name ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name field is the raw result coming from the Cloudflare API, so I assume it's properly populated to be honest. And nonetheless undefined === 'something' is falsy.


zones.forEach(zone => {
if (isActive(zone)) {
// If the zone is active, we first remove all existing duplicates and then insert the active zone.
filteredZones = filteredZones.filter(fZone => !isSameZone(zone, fZone));
filteredZones.push(zone);
} else {
// If the zone is not active, we only insert it if there is no existing active zone.
const alreadyHasActiveZone = filteredZones.some(
fZone => isSameZone(zone, fZone) && isActive(fZone)
);
if (!alreadyHasActiveZone) {
filteredZones.push(zone);
}
}
});

return filteredZones;
}
Loading