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 broken Relation View #14010

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@shucon
Contributor

shucon commented Feb 14, 2018

Signed-off-by: Saksham Gupta shucon01@gmail.com

Before submitting pull request, please check that every commit:
Fixes: #14009

  • Has proper Signed-Off-By
  • Has commit message which describes it
  • Is needed on it's own, if you have just minor fixes to previous commits, you can squash them
  • Any new functionality is covered by tests
Fix broken Relation View
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
@codecov

This comment has been minimized.

codecov bot commented Feb 14, 2018

Codecov Report

Merging #14010 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #14010      +/-   ##
============================================
+ Coverage     55.43%   55.44%   +<.01%     
  Complexity    14360    14360              
============================================
  Files           494      494              
  Lines         70457    70456       -1     
============================================
  Hits          39061    39061              
+ Misses        31396    31395       -1
@nijel

I don't think this fix is sufficient. Actually whole loop makes little sense in case $foregign_field is string:

$empty_fields = false;
foreach ($master_field as $key => $one_field) {
if ((! empty($one_field) && empty($foreign_field[$key]))
|| (empty($one_field) && ! empty($foreign_field[$key]))
) {
$empty_fields = true;
}
if (empty($one_field) && empty($foreign_field[$key])) {
unset($master_field[$key]);
unset($foreign_field[$key]);
}
}

So either the string value is not expected here, or the logic is completely wrong.

Fix foreign_field variable to Array
Signed-off-by: Saksham Gupta <shucon01@gmail.com>
@shucon

This comment has been minimized.

Contributor

shucon commented Mar 22, 2018

@nijel I've made the relevant changes and the loop now makes sense.

@mauriciofauth mauriciofauth self-assigned this Mar 27, 2018

@shucon

This comment has been minimized.

Contributor

shucon commented Mar 27, 2018

@mauriciofauth I think this is unrelated to the other PR.

@mauriciofauth

This comment has been minimized.

Member

mauriciofauth commented Mar 27, 2018

@shucon No. Issues #13941, #14009 and #14030 were about the same bug, which was created by commit 01bd6d1.

mauriciofauth added a commit that referenced this pull request Mar 27, 2018

Fixes error in foreign key sql generation + drop foreign key contraint (
#14031)

* Fixes error in foreign key sql genration, issue #14030
Changed:
	templates/table/relation/foreign_key_row.twig
The destination_column_names were getting passed as a 1d array, the names when extracted gave a string. Passing it as a 2d array (see templates/table/relation/foreign_key_row.twig lines 123,133) fixes the issue.

Sign-Off-By: Lakshay arora (b16060@students.iitmandi.ac.in)

* Fixes error in foreign key sql generation + drop foreign key contraint, issue #14030
Changed:
	templates/table/relation/foreign_key_row.twig

Fix #13941
Fix #14009
Close #14037
Close #14010

Signed-off-by: Lakshay arora <b16060@students.iitmandi.ac.in>
(cherry picked from commit 00b5962)
Signed-off-by: Maurício Meneghini Fauth <mauriciofauth@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment