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

Update hd_ticket.py #1556

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Update hd_ticket.py #1556

wants to merge 2 commits into from

Conversation

ovresko
Copy link

@ovresko ovresko commented Sep 12, 2023

frappe.get_value does not guarantee returned fields order from database, we have situations where values are reversed between enable_restrictions and ignore_restrictions (one's take the other field value)

`value = get_list( <============== IN FRAPPE THIS DOES NOT GUARANTEE FIELDS ORDER
doctype,
filters=filters,
fields=fields,
debug=debug,
limit_page_length=1,
parent=parent,
as_dict=as_dict,
)

if as_dict:
	return value[0] if value else {}

if not value:
	return

return value[0] if len(fields) > 1 else value[0][0]

`

fixing by reading the result

frappe.get_value does not guarantee returned  fields order from database, we have situations where values are reversed between enable_restrictions and ignore_restrictions (one's take the other field value)

`value = get_list(  <============== IN FRAPPE THIS DOES NOT GUARANTEE FIELDS ORDER
			doctype,
			filters=filters,
			fields=fields,
			debug=debug,
			limit_page_length=1,
			parent=parent,
			as_dict=as_dict,
		) 

	if as_dict:
		return value[0] if value else {}

	if not value:
		return

	return value[0] if len(fields) > 1 else value[0][0]
`

fixing by reading the result
@ankush
Copy link
Member

ankush commented Dec 12, 2023

frappe.get_value does not guarantee returned fields order from database, we have situations where values are reversed between enable_restrictions and ignore_restrictions (one's take the other field value)

This isn't possible. Can you share a reproducible example?

@ovresko
Copy link
Author

ovresko commented Dec 12, 2023

get_value internally uses get_list
reference: https://github.com/frappe/frappe/blob/7edb80bf5cb96700aaa14a2f6a7abe09afde4bbc/frappe/client.py#L129

get_list does not guarantee fields order since they are rows in "signle" table
that in turns uses get_values_from_single since it's a single doctype.
so a is_single is one table and fields are not columns but rather rows inside it,
in this context (multiple fields requested) no order_by is used, no sql server does guarantee the order of the rows (remember rows since this is single not table where they are columns) when no order by is used,

no "order by" could be used here that is useful in single table except field name maybe
https://github.com/frappe/frappe/blob/8242b3ee316959501ee9cacd7fd11547f978222e/frappe/client.py#L127

this is a problem in frappe itself, since it implemented singles as one table and allowed get_value on >1 fields expecting "column" order.

my solution return as_dict then fetch the fields from it.

reproduce:
any instance, set enable_restrictions to True, create 2 teams login as one and create ticket for the other you'll see the ticket in both teams dashboard.
enable_restrictions taking ignore_restrictions's value and ignore_restrictions taking enable_restrictions's value

So IT IS POSSIBLE

@ankush
Copy link
Member

ankush commented Dec 12, 2023

@ovresko this bug should really be fixed in framework. I'll try it out. Thanks for explanation 👍

Fix spelling "restrictions"
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

Successfully merging this pull request may close these issues.

3 participants