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

Row count incorrect on nested subform #22441

Closed
dawe78 opened this issue Oct 1, 2018 · 12 comments
Closed

Row count incorrect on nested subform #22441

dawe78 opened this issue Oct 1, 2018 · 12 comments

Comments

@dawe78
Copy link

dawe78 commented Oct 1, 2018

I updated to 3.9.0 beta3 on saturday and startet testing my component using nested subform, but I got an issue...

Steps:

  1. First subform name "order_articles"
  2. inside subform order_articles, nested subform "order_addons"

Click add-row for new article: row created width id "jform_order_articles__order_articles0__[nameofFields]"
Click add-row for new addon inside article: row created "jform_order_articles__order_articles1__order_addons__order_addons0__addon"

As you can see, nested subform count up article row num; subform should be jform_order_articles__order_articles0_..., but it creates jform_order_articles__order_articles1_...

I tested this behaviour only with my own component form until now, but I will do this test with a blank test form as soon as possible and report the results. Maybe someone has the same issue?

Originally posted by @dawe78 in #17552 (comment)

@dawe78
Copy link
Author

dawe78 commented Oct 1, 2018

I also tested on blank form, same issue... Nested subform starts counting on parent subform row num, eg row "order_articles2" subform starts at "order_articles3__order_addons__order_addons0".

Additional information: after saving form (only parent subform rows saved) adding nested subform rows results in correct row count. So this issue occurs only on new added subform rows, after saving subform row adding nested subform rows works fine.

@continga
Copy link

continga commented Oct 1, 2018

Hi, thanks for reporting this. I am not completely sure that I understand the problem, so just to be sure:

You are talking about the id's of the subform fields, correct? And the problem is that the numbering of the subform rows is incorrect? I am not completely sure what you mean with that...

Can you provide more information, e.g. a screenshot of the DOM-inspector of the fields, with the subforms visible, and the incorrect id's, and how the id's should be instead, from your point of view?

Additionally: Why is the naming of the id's of the fields relevant to you?

edit: Additionally, can you please add a short testing instruction (maybe include relevant form xml definition) so we can reproduce the issue easily, and see where exactly things are not getting numbered correctly

@dawe78
Copy link
Author

dawe78 commented Oct 1, 2018

Hi cotinga,
thanks for your reply.

Its not easy to explain what happens, so I put some code here as you suggested. Its not the id only, the issue affects name too - so saving fails. The name of the parent subform is

jform[order_articles][order_articles0][article],

for addons in the first row nested subform it should be

jform[order_articles][order_articles0][order_addons][order_addons0][addon], but is
jform[order_articles][order_articles1][order_addons][order_addons0][addon].

Here is the code example:

<tbody class="rows-container-sr-0 ui-sortable">
<tr class="subform-repeatable-group subform-repeatable-group-sr-0" data-base-name="order_articles" data-group="order_articles0" data-new="true">
  <td data-column="Article"><div class="control-group">
	  <div class="controls"><input name="jform[order_articles][order_articles0][article]" id="jform_order_articles__order_articles0__article" value="" type="text"></div>
  </div></td>
  <td data-column="Addons"><div class="control-group">
     <div class="controls"><input name="jform[order_articles][order_articles0][order_addons]" value="" id="jform_order_articles__order_articles0__order_addons" type="hidden"><div class="row-fluid">
    <div class="subform-repeatable-wrapper subform-table-layout subform-table-sublayout-section">
<div class="subform-repeatable" data-bt-add="a.group-add-sr-1" data-bt-remove="a.group-remove-sr-1" data-bt-move="a.group-move-sr-1" data-repeatable-element="tr.subform-repeatable-group-sr-1" data-rows-container="tbody.rows-container-sr-1" data-minimum="0" data-maximum="1000">
    <table class="adminlist table table-striped table-bordered"><thead>
      <tr><th>Addon<br><small style="font-weight:normal"></small></th><th style="width:8%;"><div class="btn-group"><a class="btn btn-mini button btn-success group-add-sr-1" aria-label="Hinzufügen"><span class="icon-plus" aria-hidden="true"></span></a></div></th></tr>     
    </thead><tbody class="rows-container-sr-1 ui-sortable">
      <tr class="subform-repeatable-group subform-repeatable-group-sr-1" data-base-name="order_addons" data-group="order_addons0" data-new="true">
        <td data-column="Addon"><div class="control-group">
	   <div class="controls"><input name="jform[order_articles][order_articles1][order_addons][order_addons0][addon]" id="jform_order_articles__order_articles1__order_addons__order_addons0__addon" value="0" type="text"></div>
           </div></td>

@dawe78
Copy link
Author

dawe78 commented Oct 1, 2018

How can I put code in there? Dont get it... :-(

@tonypartridge
Copy link
Contributor

Can you post a copy of your xml? I tested this working fine, although didn’t check the counting it did save and reload without issue with multi-deep nested forms

@dawe78
Copy link
Author

dawe78 commented Oct 1, 2018

testform.xml ist main form, subform_1.xml first subform, subform_2.xml nested subform...
testform.zip

@gaelicwinter
Copy link

More information on this at #22690

@gaelicwinter
Copy link

I have a fix for this, I just have to figure out how to do a pull request. The problem stems from when a new row is added to the nested subform, it uses the count of the nested subform to determine a suffix for the parent subform selector, which effectively adds a new parent row instead of a child row.

In media/system/js/subform-repeatable-uncompressed.js, I've changed this:

        // fix names and ids for fields in $row
	$.subformRepeatable.prototype.fixUniqueAttributes = function(
		$row, // the jQuery object to do fixes in
		_count, // existing count of rows
		_group, // current group name, e.g. 'optionsX'
		_basename // group base name, without count, e.g. 'options'
	) {
		var group = (typeof _group === 'undefined' ? $row.attr('data-group') : _group),
			basename = (typeof _basename === 'undefined' ? $row.attr('data-base-name') : _basename),
			count    = (typeof _count === 'undefined' ? 0 : _count),
			countnew = Math.max(this.lastRowNum, count),
			groupnew = basename + countnew;

		this.lastRowNum = countnew + 1;
		$row.attr('data-group', groupnew);

to this:

        // fix names and ids for fields in $row
	$.subformRepeatable.prototype.fixUniqueAttributes = function(
		$row, // the jQuery object to do fixes in
		_count, // existing count of rows
		_group, // current group name, e.g. 'optionsX'
		_basename // group base name, without count, e.g. 'options'
	) {
		var group = (typeof _group === 'undefined' ? $row.attr('data-group') : _group),
			basename = (typeof _basename === 'undefined' ? $row.attr('data-base-name') : _basename),
			count    = (typeof _count === 'undefined' ? 0 : _count),
			subform_selector = this.getRowSelector($row[0]), //select this subform
			countnew = Math.max($row.closest('.subform-repeatable').find(subform_selector).length - 1, count), //and count its rows
			groupnew = basename + countnew;

		this.lastRowNum = countnew + 1;
		$row.attr('data-group', groupnew);

and I've added this function:

    //Return a row selector for this particular subform or subform template
    $.subformRepeatable.prototype.getRowSelector = function(row) {
    	let select = row.querySelector('.subform-repeatable-group'), row_classlist;
    	if(select === null) {
    		// DOMElement
                row_classlist = row.classList;
        }
	else {
		// documentFragment
                row_classlist = select.classList;
	}
        var selector = [];
        for(let k = 0, len = row_classlist.length; k < len; k++) {
            selector[k] = row_classlist[k];
        }
        selector.unshift('');
        return selector.join('.');
    };`

The problem observed now disappears, and the field names and ids are correct.
subform-repeatable-uncompressed.zip

@continga
Copy link

I will check this this weekend. I need to take a look at it, I am not sure whether I really understand the problem yet.

Your fix seems a bit like an overkill though. Each subform should keep its own count, and not mix it up with other instances. Might be a hint, that something is broken on another place. But, as I said, I will take a look

@gaelicwinter
Copy link

I am well known for my overkill fixes.

continga pushed a commit to continga/joomla-cms that referenced this issue Oct 21, 2018
We were incorrectly using the lastRowNum property inside of the subformRepeatable object due to
legacy reasons (the logic using a lastRowNum property has been overtaken from history, see commit
f2a9874 ).

What happened is that we always were incrementing the lastRowNum property by 1 inside of the
fixUniqueAttributes method, whilest we call that method multiple times recursively, when we have
nested subforms. So having a 2-level subform meant we would increment the lastRowNum twice for every
added row, effectively meaning we would skip certain indices and hence totally lose the connection
between the actual count of existing rows and their index. Actually we do not need the lastRowNum
property, as the current logic works well without it just by calculating the count of rows directly
in the addRow method. So removing the lastRowNum property is the best solution.

See also issue joomla#22690
@continga
Copy link

Can you guys please check PR #22757 which should fix this?

@joomla-cms-bot
Copy link

Set to "closed" on behalf of @Quy by The JTracker Application at issues.joomla.org/joomla-cms/22441

mbabker pushed a commit that referenced this issue Nov 5, 2018
* This commit fixes issue #22441

We were incorrectly using the lastRowNum property inside of the subformRepeatable object due to
legacy reasons (the logic using a lastRowNum property has been overtaken from history, see commit
f2a9874 ).

What happened is that we always were incrementing the lastRowNum property by 1 inside of the
fixUniqueAttributes method, whilest we call that method multiple times recursively, when we have
nested subforms. So having a 2-level subform meant we would increment the lastRowNum twice for every
added row, effectively meaning we would skip certain indices and hence totally lose the connection
between the actual count of existing rows and their index. Actually we do not need the lastRowNum
property, as the current logic works well without it just by calculating the count of rows directly
in the addRow method. So removing the lastRowNum property is the best solution.

See also issue #22690

* Add the corectly minified version for my previous commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants