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

fix!: map columns correctly in query report via fieldnames #26500

Closed
wants to merge 1 commit into from

Conversation

phot0n
Copy link
Contributor

@phot0n phot0n commented May 21, 2024

This pr helps in mapping the report columns correctly as defined in the column table for query report type. Previously it used to map the columns in the sequence they're defined ..not corresponding with the fieldnames

before:

Screencast.2024-05-23.02.00.03.mp4

after:

Screencast.2024-05-23.01.58.27.mp4

This could be considered a breaking change if people have written queries without providing appropriate fieldnames in the column table.

@github-actions github-actions bot added the add-test-cases Add test case to validate fix or enhancement label May 21, 2024
@@ -146,7 +146,7 @@ def execute_query_report(self, filters):

check_safe_sql_query(self.query)

result = [list(t) for t in frappe.db.sql(self.query, filters)]
result = frappe.db.sql(self.query, filters, as_dict=1)
Copy link
Member

Choose a reason for hiding this comment

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

ummm, isn't this different?

Copy link
Member

@ankush ankush May 21, 2024

Choose a reason for hiding this comment

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

list[list[Any]] -> [list[dict[str, Any]]]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's the point 😆 - will update the description in a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the code automatically handles it down the line btw:

if result and isinstance(result[0], list | tuple):
for row in result:
row_obj = {}
for idx, column_name in enumerate(column_names):
row_obj[column_name] = row[idx]
data.append(row_obj)
else:
data = result

@phot0n
Copy link
Contributor Author

phot0n commented May 22, 2024

tested this with aliased columns as well as with stuff like po.name - seems to work fine

Screencast.2024-05-23.02.35.01.mp4

@phot0n
Copy link
Contributor Author

phot0n commented May 22, 2024

side note(s):

  • the filter table should have the "mandatory" field checked by default (maybe only on query report type) as when people use %(x)s notation in their queries they waste their time in figuring out why even the simple stuff doesn't work.

image

  • not sure why we need reference doctype and reference report field(s) shown for query report type?

image

  • show all fields in the columns table in the grid view so that people don't have to "discover" them by clicking on edit

@phot0n phot0n requested a review from ankush May 23, 2024 06:40
@phot0n phot0n changed the title fix: map columns correctly in query report via fieldnames fix!: map columns correctly in query report via fieldnames May 24, 2024
@phot0n phot0n marked this pull request as ready for review May 24, 2024 16:43
Copy link

stale bot commented Jun 13, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Jun 13, 2024
@stale stale bot closed this Jun 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add-test-cases Add test case to validate fix or enhancement inactive
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants