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

Guard null model fields - fix and tests #403

Merged
merged 4 commits into from
Oct 4, 2017

Conversation

thecjharries
Copy link

I ran into an issue with the reconcile method of TwoCellAnchorXform. I can't provide the spreadsheet to give you a working demo of the issue, but I did find the problem and built a unit test to display the issue. Basically, model.picture might be null, which throws an error. Once I added the guards, I was able to load the workbook as usual.

}
if (Number.isInteger(model.tl.row) && Number.isInteger(model.tl.col) && Number.isInteger(model.br.row) && Number.isInteger(model.br.col)) {
if (model.tl && Number.isInteger(model.tl.row) && Number.isInteger(model.tl.col) && Number.isInteger(model.br.row) && Number.isInteger(model.br.col)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth adding a test for model.br too? Obviously I don't know if it's possible for it to be null or not, but the code doesn't give any indication either way.

Copy link
Author

Choose a reason for hiding this comment

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

Great point. I went ahead and added a br guard and test. I should have done that originally.

Copy link
Author

Choose a reason for hiding this comment

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

I started with the tl guard because I was testing with model = {}, which threw an error on that conditional. I didn't have the time to debug everything enough to figure out where model was coming from and why its picture was null, so I'm as in the dark about whether or not br can be null without a null tl as you are. It doesn't hurt, so there's no reason not to.

@guyonroche guyonroche merged commit 4489266 into exceljs:master Oct 4, 2017
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.

4 participants