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

Fix error by two pivot with one source #1195 #1196

Merged
merged 8 commits into from
Jun 12, 2019

Conversation

b0bi79
Copy link
Member

@b0bi79 b0bi79 commented Apr 30, 2019

What's this PR do?

I created in MS Excel a file containing two Pivot on different sheets, the data source for which is a table with data. Then I opened this file in ClosedXML and changed the data in the data table and saved the file. When opening the received file, MS Excel displays an error:
Deleted component: Component /xl/pivotTables/pivotTable1.xml. (Summary Table View)
Deleted Records: PivotTable Report from Part /xl/workbook.xml (Book)

The reason that in the pivotCacheDefinition the TaxRate field was not filled in was that the file was generated based on a template in which two summary tables are constructed from data from the same data table. In this case Excel creates one pivotCacheDefinition file, and ClosedXML while caches the last Pivot overwrites the cache for the first Pivot.

How should this be manually tested?

The test ClosedXML_Tests.XLPivotTableTests.TwoPivotWithOneSourceTest()

Fixes #1195

Copy link
Member

@igitur igitur left a comment

Choose a reason for hiding this comment

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

Preliminary feedback

ClosedXML_Tests/Excel/PivotTables/XLPivotTableTests.cs Outdated Show resolved Hide resolved
ClosedXML_Tests/Excel/PivotTables/XLPivotTableTests.cs Outdated Show resolved Hide resolved
ClosedXML/Excel/XLWorkbook_Save.cs Outdated Show resolved Hide resolved
@igitur
Copy link
Member

igitur commented Jun 7, 2019

@b0bi79 You have to revert the old files in ClosedXMLL_Tests/Resource/Other/PivotTableReferenceFiles/* too for the tests to pass.

@igitur
Copy link
Member

igitur commented Jun 9, 2019

@b0bi79 Can you please give me access to push some changes to you b0bi79/two_pivot_fix_1195 branch?

@igitur
Copy link
Member

igitur commented Jun 9, 2019

To do that, I think you have to check this option (on this PR):
image

@b0bi79
Copy link
Member Author

b0bi79 commented Jun 9, 2019

This option is on. Now I will give full access to the repository.

@b0bi79
Copy link
Member Author

b0bi79 commented Jun 9, 2019

You are now an collaborator.

@igitur
Copy link
Member

igitur commented Jun 10, 2019

Thanks for the contribution. I made some additional changes to avoid having to clone elements. It's becoming clear that we'll need a big refactor of pivot tables to allow references to shared pivot sources. But for now, I think this PR is fine. Do you agree with my additional changes?

@b0bi79
Copy link
Member Author

b0bi79 commented Jun 10, 2019

Thank you, Francois. I checked it with my tests, all works.

@igitur
Copy link
Member

igitur commented Jun 10, 2019

@Pankraty Would you mind doing a quick check on this PR too? It will soon be refactored anyway.

commit history for this PR will be squashed.

@igitur igitur added this to the v0.95 milestone Jun 10, 2019
@igitur igitur added the bug label Jun 10, 2019
@igitur igitur requested a review from Pankraty June 10, 2019 16:03
@igitur igitur merged commit 85916ea into ClosedXML:develop Jun 12, 2019
@b0bi79 b0bi79 deleted the two_pivot_fix_1195 branch June 12, 2019 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The modified ClosedXML file opens in Excel with an error.
3 participants