Skip to content

Commit

Permalink
Added invalid import record errors and counts
Browse files Browse the repository at this point in the history 
no issue

- This new format allows to return additional metadata with failed import records. The data for invalid records is returned in following format:
```
{
    count: {count_of_invalid_records},
    errors: [{
      message:	"Members not imported. Members with duplicate Stripe customer ids are not allowed." // message field of the error
     context:	"Attempting to import members with duplicate Stripe customer ids." // context field of the error
     help:	"Remove duplicate Stripe customer ids from the import file, and re-run the import." // help field of the error
     count:	2 // count of this specific error
    }]
};
- Errors are grouped by their context fields because message fields sometimes can contain unique information like Stripe customer id, which would produce too many errors in case of bigger datasets.
  • Loading branch information
@naz
naz committed Jun 12, 2020
1 parent 589d826 commit 7904c30
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 29 deletions.
71 changes: 53 additions & 18 deletions core/server/api/canary/members.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -387,9 +387,14 @@ const members = {
},
async query(frame) {
let filePath = frame.file.path;
let fulfilled = 0;
let invalid = 0;
let duplicates = 0;
let imported = {
count: 0
};
let invalid = {
count: 0,
errors: []
};
let duplicateStripeCustomerIdCount = 0;

const columnsToExtract = [{
name: 'email',
Expand Down Expand Up @@ -427,7 +432,16 @@ const members = {
columnsToExtract: columnsToExtract
}).then((result) => {
const sanitized = sanitizeInput(result);
invalid += result.length - sanitized.length;
duplicateStripeCustomerIdCount = result.length - sanitized.length;
invalid.count += duplicateStripeCustomerIdCount;

if (duplicateStripeCustomerIdCount) {
invalid.errors.push(new errors.ValidationError({
message: i18n.t('errors.api.members.duplicateStripeCustomerIds.message'),
context: i18n.t('errors.api.members.duplicateStripeCustomerIds.context'),
help: i18n.t('errors.api.members.duplicateStripeCustomerIds.help')
}));
}

return Promise.map(sanitized, ((entry) => {
const api = require('./index');
Expand Down Expand Up @@ -465,30 +479,51 @@ const members = {
}), {concurrency: 10})
.each((inspection) => {
if (inspection.isFulfilled()) {
fulfilled = fulfilled + 1;
imported.count = imported.count + 1;
} else {
const error = inspection.reason();
if (error instanceof errors.ValidationError && !(error.message.match(/Stripe/g))) {
duplicates = duplicates + 1;

// NOTE: if the error happens as a result of pure API call it doesn't get logged anywhere
// for this reason we have to make sure any unexpected errors are logged here
if (Array.isArray(error)) {
logging.error(error[0]);
} else {
// NOTE: if the error happens as a result of pure API call it doesn't get logged anywhere
// for this reason we have to make sure any unexpected errors are logged here
if (Array.isArray(error)) {
logging.error(error[0]);
} else {
logging.error(error);
}

invalid = invalid + 1;
logging.error(error);
}

invalid.count = invalid.count + 1;

invalid.errors.push(error);
}
});
}).then(() => {
// NOTE: grouping by context because messages can contain unique data like "customer_id"
const groupedErrors = _.groupBy(invalid.errors, 'context');
const uniqueErrors = _.uniq(invalid.errors, 'context');

const outputErrors = uniqueErrors.map((error) => {
let errorGroup = groupedErrors[error.context];
let errorCount = errorGroup.length;

if (error.message === i18n.t('errors.api.members.duplicateStripeCustomerIds.message')) {
errorCount = duplicateStripeCustomerIdCount;
}

// NOTE: filtering only essential error information, so API doesn't leak more error details than it should
return {
message: error.message,
context: error.context,
help: error.help,
count: errorCount
};
});

invalid.errors = outputErrors;

return {
meta: {
stats: {
imported: fulfilled,
duplicates: duplicates,
imported: imported,
invalid: invalid
}
}
Expand Down
5 changes: 5 additions & 0 deletions core/server/translations/en.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,11 @@
"stripeCustomerNotFound": {
"context": "The customer does not exist in currently linked Stripe account.",
"help": "Connect the correct Stripe account, and re-run the import."
},
"duplicateStripeCustomerIds": {
"message": "Members not imported. Members with duplicate Stripe customer ids are not allowed.",
"context": "Attempting to import members with duplicate Stripe customer ids.",
"help": "Remove duplicate Stripe customer ids from the import file, and re-run the import."
}
},
"tags": {
Expand Down
5 changes: 2 additions & 3 deletions test/api-acceptance/admin/members_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -316,9 +316,8 @@ describe('Members API', function () {
should.exist(jsonResponse.meta);
should.exist(jsonResponse.meta.stats);

jsonResponse.meta.stats.imported.should.equal(2);
jsonResponse.meta.stats.duplicates.should.equal(0);
jsonResponse.meta.stats.invalid.should.equal(0);
jsonResponse.meta.stats.imported.count.should.equal(2);
jsonResponse.meta.stats.invalid.count.should.equal(0);
});
});

Expand Down
13 changes: 5 additions & 8 deletions test/regression/api/canary/admin/members_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,8 @@ describe('Members API', function () {
should.exist(jsonResponse.meta);
should.exist(jsonResponse.meta.stats);

jsonResponse.meta.stats.imported.should.equal(2);
jsonResponse.meta.stats.duplicates.should.equal(0);
jsonResponse.meta.stats.invalid.should.equal(0);
jsonResponse.meta.stats.imported.count.should.equal(2);
jsonResponse.meta.stats.invalid.count.should.equal(0);
})
.then(() => {
return request
Expand Down Expand Up @@ -202,9 +201,8 @@ describe('Members API', function () {
should.exist(jsonResponse.meta);
should.exist(jsonResponse.meta.stats);

jsonResponse.meta.stats.imported.should.equal(2);
jsonResponse.meta.stats.duplicates.should.equal(0);
jsonResponse.meta.stats.invalid.should.equal(0);
jsonResponse.meta.stats.imported.count.should.equal(2);
jsonResponse.meta.stats.invalid.count.should.equal(0);
})
.then(() => {
return request
Expand Down Expand Up @@ -251,8 +249,7 @@ describe('Members API', function () {
should.exist(jsonResponse.meta);
should.exist(jsonResponse.meta.stats);

jsonResponse.meta.stats.imported.should.equal(0);
jsonResponse.meta.stats.duplicates.should.equal(0);
jsonResponse.meta.stats.imported.count.should.equal(0);
jsonResponse.meta.stats.invalid.should.equal(2);
});
});
Expand Down

0 comments on commit 7904c30

Please sign in to comment.