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

0000635: Indicators values not in order in long period project exports #15

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

manishvishnoi2
Copy link

@ghost
Copy link

ghost commented Mar 22, 2016

Hi @manishvishnoi2, thank you for this PR.

I am skeptical about the way you fixed the issue because you broke the immutability on the PivotTableData.Axis class. Can you try again to fix the issue, keeping the class immutable (keeping the setter declared private)?

You will have to pass label parameter to the Axis constructor, so you will have to find where the Axis is instantiated earlier in the generation process.

@ghost ghost assigned ghost and unassigned ghost Mar 22, 2016
@manishvishnoi2
Copy link
Author

@numero-six Hello sir,I will look at the code again and try to fix this.

@manishvishnoi2
Copy link
Author

@numero-six Plz look at the latest fix.It is done by working on Axis class creating a new function thus not breaking immutability.

@ghost
Copy link

ghost commented Apr 4, 2016

@manishvishnoi2 your second commit has the same problem, immutability is still broken. You just added another setter.

You should try to solve the problem just like if the label field was declared final. You will see that your only way become to change the label by passing it to the existing Axis constructor with 4 parameters. If you do a find usages in your IDE, you may find that the constructor is called only by PivotTableData.Axis#addChild and I guess that you have to find the call to addChild that should be fixed.

I see that one of them is in org.sigmah.server.report.model.generator.PivotGenerator.Populator#addMonths add that method compute a label by calling a method renderMonthLabel. I don't know if it is the exact place to fix but in my opinion but the fix should occur somewhere around there. It would make more sense that the Populator would be modified and the Axis not.

Please, look at the issue again, there should be a way.

@@ -218,6 +218,7 @@ private void createDetailSheet(final IndicatorDTO indicator) throws Throwable {
Map<String, Integer> columnIndexMap = new HashMap<String, Integer>();
for (PivotTableData.Axis axis : leaves) {
CalcUtils.putHeader(row, ++cellIndex, axis.getLabel());
axis.setLabel(String.valueOf(cellIndex));
Copy link
Member

Choose a reason for hiding this comment

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

@manishvishnoi2 : This needs an indentation fix before the pull request can be merged.

@spMohanty
Copy link
Member

@manishvishnoi2 : Just checking if you had time to fix the issue with the immutability on the PivotTableData.Axis class, as pointed out by @numero-six

@manishvishnoi2
Copy link
Author

@spMohanty Yes i am working on it.I will look at reduntant spaces and indentation.

@manishvishnoi2
Copy link
Author

@numero-six Hello sir, I looked at the problem again.I found out that populator is just filling by applying all the label parameter.Also, rather than using a constructor which has other parameters also , i think using a new method as i did is better.The reason i modified axis is because, to populate cells its using Month-Name string which repeats every 12th time.So we have to change the label which cannot be done at populator but at axis.
To my thoughts my method is best suitable but if you feel otherwise one more method i can suggest is to make a new constructor with only label parameter.

@ghost
Copy link

ghost commented Apr 20, 2016

Yeah, if you remove the setter and add a constructor with a label parameter, it would be a better option.

@ghost ghost removed their assignment Oct 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants