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

Projects
None yet
3 participants
@b0bi79
Copy link
Member

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

@igitur
Copy link
Member

left a comment

Preliminary feedback

Show resolved Hide resolved 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
@igitur

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

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

@igitur

This comment has been minimized.

Copy link
Member

commented Jun 9, 2019

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

@b0bi79

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2019

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

@b0bi79

This comment has been minimized.

Copy link
Member Author

commented Jun 9, 2019

You are now an collaborator.

@igitur igitur force-pushed the b0bi79:two_pivot_fix_1195 branch from 9bbd39c to 39c76cc Jun 10, 2019

@igitur

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2019

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

@igitur

This comment has been minimized.

Copy link
Member

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 approved these changes Jun 10, 2019

@igitur igitur requested a review from Pankraty Jun 10, 2019

@igitur igitur merged commit 85916ea into ClosedXML:develop Jun 12, 2019

2 checks passed

WIP Ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@b0bi79 b0bi79 deleted the b0bi79:two_pivot_fix_1195 branch Jun 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.