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

Minor bug fixed in the Merge Dialogue #7415

Conversation

N-thony
Copy link
Collaborator

@N-thony N-thony commented Apr 25, 2022

Fixes #7414
@africanmathsinitiative/developers this is ready for review.

@lloyddewit
Copy link
Contributor

@N-thony Thank you for this PR.
I see that you have renamed some variables but I can't see what else has changed.
Which lines fix the bug?
Thanks

@rdstern
Copy link
Collaborator

rdstern commented Apr 26, 2022

@N-thony I assume this is the main prepare > reshape > merge dialogue, but I'm not sure what to do to check this is resolved? What data and example gace that error before?

@N-thony
Copy link
Collaborator Author

N-thony commented Apr 26, 2022

@N-thony I assume this is the main prepare > reshape > merge dialogue, but I'm not sure what to do to check this is resolved? What data and example gace that error before?

@rdstern while testing PR #7404 with the survey dataset, tried to open the Merge dialogue, you will get the bug that this PR attends to fix. To quickly avoid that bug in the merge version since it was easy to fix, I did the fix in this PR.
First test PR #7404 and in that same branch try to open Merge dialogue and you will get the bug.

Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

No problem with Merge here, which there was with the pull request @N-thony pointed me to. I hope all will still work when both are merged.
Ok from my side now. Over to @lloyddewit or @ChrisMarsh82

@lloyddewit
Copy link
Contributor

@N-thony Thank you for this PR. I see that you have renamed some variables but I can't see what else has changed. Which lines fix the bug? Thanks

I don't see any functional changes in the code, so I can't see how this PR has fixed a bug.
Am I missing something?

@lloyddewit
Copy link
Contributor

The functional change in this PR is that 'New' has been added to lines 22, 23, and 29.
This is a temporary fix for issue #7414 and part of #7438.
The real issue is that an event handler in this dialog is called before SetDefaults(). This issue was introduced by PR #7404.
Another complication is that this PR was branched from master before PR #7404 was merged so we cannot fully test this branch.
@N-thony and I discussed. On balance, we decided to merge this PR and address the root cause of the problem in issue #7438.

@lloyddewit lloyddewit merged commit 9d7f8cb into IDEMSInternational:master May 3, 2022
@lloyddewit lloyddewit added the bug label May 3, 2022
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.

Small Bug in the Merge Dialogue
3 participants