Page MenuHomePhabricator

Cargo: Dynamic tables - |order by= property ignored & UNIQ-QINU with |details fields= property
Open, Needs TriagePublic

Description

Running a cargo_query and setting format to dynamic table is causing two issues that I can confirm as bugs:

  1. The |order by= property is always ignored and the table is instead sorted based off of the first column. This creates an issue when trying to sort by data but instead the items appear with alphabetic sort.
  2. When using |details fields= property, one of my CONCAT fields throws a UNIQ-QINU error. I can confirm that I have added |no html in the template I'm calling and if I do hide the field with |details field= then the field is returned without errors.

Details

Related Gerrit Patches:
mediawiki/extensions/Cargo : masterFix for dynamic table format

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 12 2019, 3:10 AM

@Madrigal84 - I am unable to regenerate the first bug you mentioned. In my case, the table is being sorted according to the columns in order by property, not the first column.
And in the 2nd bug, what happens when you expand the details field column? What is being shown there?

@Nikhil-nk

When I try to sort by the Date a page was create, where page 1 was created first, and page 12 was created last, this is what I get: 1, 10, 11, 12, 3, 4, 5, 6, and so on, if pages is the first column. If Date is the first column then the pages are listed in the intended order: 1, 2, 3, 4, 5, 6, and so on.

When I expand the details, this is the error: ' " UNIQ-- -QINU" '
If I move that field to a column not listed |details field=, the template loads without issue and I do not see ' " UNIQ-- -QINU" '

@Nikhil-nk

Just tested |order by= with |format=table vs. |format=dynamic table and it worked, so I can confirm that there is a bug with |format=dynamic table.

@Madrigal84 - Okay, in the example you gave earlier, is date in your field list or not? Because, if it's not, then in dynamic table, order by will be ignored and the table will be sorted according to the first column.

Madrigal84 added a comment.EditedMar 14 2019, 8:41 PM

@Nikhil-nk yes, order_date=Date is my 2rd column; notice how 10, 11, and 12 were created after 3, 4, 5, 6, 7, 8, and 9 yet the order by Date is being ignored:

Sorry! But in my case, if Date is in the field list, then results are just as intended. If it's not, then whatever you're saying is happening (i.e. order by is being ignored).

{{#cargo_query:
tables=_pageData
|fields= _pageName=Page, _creationDate = Date
|order by= Date
|format=dynamic table
}}

For the above query, this is the result:

@Nikhil-nk

I was able to get it to work just like your query. The issue was the |details fields=, when I removed this property the |order by= took effect. This is also what is causing my second issue, so it is a bug with the |details fields= property.

@Madrigal84 - You're right. With details field, order by is being ignored.

Change 496833 had a related patch set uploaded (by Nikhil-nk; owner: Nikhil-nk):
[mediawiki/extensions/Cargo@master] Fix for dynamic table format

https://gerrit.wikimedia.org/r/496833

@Madrigal84 - I just created a patch for the first issue. It would be great if you check out the code, and see if it works for you.

And now to the 2nd issue, can you give me an example cargo_query where it's happening?

Change 496833 had a related patch set uploaded (by Yaron Koren; owner: Nikhil-nk):
[mediawiki/extensions/Cargo@master] Fix for dynamic table format

https://gerrit.wikimedia.org/r/496833

Change 496833 merged by jenkins-bot:
[mediawiki/extensions/Cargo@master] Fix for dynamic table format

https://gerrit.wikimedia.org/r/496833

@Nikhil-nk

I modified /ext.cargo.datatables.js to include the following code, but it still did not fix the order by issue when details fields property is used.

		var orderByFields = $(this).attr( 'data-order' );
		if ( detailsFields ) {
			if ( !orderByFields ) {
				params['order'] = [1, 'asc'];
			}

Did you also include the changes in the formats/CargoDynamicTableFormat.php file?

@Nikhil-nk

I now updated the .js and .php, and the patch works. Now the |oder by= property is respected when |details fields= is used. Still, unlike a regular Table query, the field used by |oder by= must actually be included in the query, which is fine but it would be better to be able to use a field for |oder by= without actually displaying it.

For the second bug, this is the field I am using:
CONCAT('{{CaseSlidesQuery{{#urldecode:%7C}}',_pageTitle,'}}')=View

If I do not hide it with the |details fields= property, I see exactly what I queried for in one of the Dynamic Table columns. If the |details fields= is applied for View, then I receive this error: "UNIQ --- QINU"

Aklapper renamed this task from Cargo: Dynamic tables - |order by= property ignored & UNIQ-QINU with |details fields= proerty to Cargo: Dynamic tables - |order by= property ignored & UNIQ-QINU with |details fields= property.Mar 23 2019, 6:49 PM

Still, unlike a regular Table query, the field used by |oder by= must actually be included in the query, which is fine but it would be better to be able to use a field for |oder by= without actually displaying it.

That's a good point. Try this.

Regarding the 2nd bug:
I don't know why this is happening, but it's not due to the dynamic table format.

@Yaron_Koren : What do you think about the 2nd bug?

Madrigal84 added a comment.EditedMar 23 2019, 10:46 PM

@Nikhil-nk

Wonderful, the |order by= property is working now!

For bug 2, if I do not use |details fields= for the View field, then it works perfectly, but when I hide it with the |details fields= property that is when I get the error. So to me that indicates that they query is working well, but the |details fields= property that is negatively affecting it.

Thank you for your help.

@Nikhil-nk

I have followed these instructions but still get the same UNIQ-QINU error:

no html - specifies that this query does not include any HTML in its results.
This is important for embedded queries (a #cargo_query call contained within another one, or within a different parser function). Such embedded calls often fail due to parsing.
For example, if your query results show 'UNIQ-item1-QINU' instead of the expected values, try placing 'no html' within the inner queries tends to fix the problem.

That note was for nested cargo queries, but your CONCAT field seemed like a template call. Or, are you using a different field now?

@Nikhil-nk

It is a template call to a template that runs a cargo-query. Is there a better way to run the query within the original query, rather than using a template?

I directly include the query in CONCAT function. I don't know if it's the best way or not, but it works for simple queries.

@Nikhil-nk

Just tried to directly include the query but I see the same behavior when I use the |details fields= property. If I do not use it, the query works perfectly; if I hide that field with |details fields= then I unfortunately get the UNIQ-QINU error.

Even with |no html? When I tried it, then I either got UNIQ-QINU error (without |no html) in both cases or in none of the cases.

Hi @Nikhil-nk, yes even with |no html I get the same error.

Ah, that's not good. If possible, can you please give me the exact query you are doing?

fields=CONCAT('{{slidesOSD{{#urldecode:%7C}}',_pageTitle,'}}')=View

slidesOSD goes to a template that then goes to another template, which is an image viewer. If I do not use the |details fields= property, I can see the image perfectly, but once I hide the View field with |details fields= I see the UNIQ-QINU error.

I thought you included an extra cargo query or some parser function in the CONCAT field, but it's the same one.
After reading your use case, I don't know if you would be able to include the template's content directly in CONCAT.
This is a mystery to me as to why there is not the UNIQ-QINU error in the first case (i.e. when not using |details fields).

As of now, I don't know the reasons why this is happening, but if I find anything useful on this, i'll let you know.

@Nikhil-nk

Thank you for your help. Hopefully the |details fields= bug is patched at some point.