Please criticise use of global keyword in Admin

[Mike Dewhirst]

The code below appears to work perfectly but it worries me because I
have never used the global keyword before.

Is there a generous guru who will criticise the code constructively for
me please?

The use case is Admin review of payment gateway receipt records.

Users in the manager group can have readonly access to the transaction
half of the information while all payment gateway cost info is hidden.

Users in the consultant group get readonly access to the gateway info as
well.

Members of the operator group can do manual invoices so they need write
access to a couple of fields.

Many thanks

Mike

#admin.py for the billing app

rec_fields = [
"company",
"chemical",
"invoice",
"currency",
"currency_amount",
"gst_amount",
"gst_rate",
"gst_name",
"amount_due",
"amount_paid",
"paid_by",
"date_paid",
]


cons_fields = [
"gateway_flagfall",
"gateway_rate",
"gateway_amount",
"gateway_gst_amount",
"net_amount",
]

receipt_fields = rec_fields[:]
receipt_fields_ro = rec_fields[:]
consultant_fields = cons_fields[:]

def trim_fields():
global receipt_fields, receipt_fields_ro, consultant_fields #<<<<<<<<<<
if "gateway_flagfall" in receipt_fields:
for field in consultant_fields:
receipt_fields.remove(field)
receipt_fields_ro = receipt_fields[:]


def add_fields(operator=False):
global receipt_fields, receipt_fields_ro, consultant_fields #<<<<<<<<<<
if "gateway_flagfall" not in receipt_fields:
receipt_fields.extend(consultant_fields)
receipt_fields_ro.extend(consultant_fields)
if operator:
receipt_fields_ro.remove("amount_due")
receipt_fields_ro.remove("amount_paid")
receipt_fields_ro.remove("date_paid")


class ReceiptAdmin(admin.ModelAdmin):

global receipt_fields, receipt_fields_ro #<<<<<<<<<<<<

def has_add_permission(self, request, obj=None):
return is_operator(request.user)

def has_change_permission(self, request, obj=None):
return is_operator(request.user)

# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #
#
def get_queryset(self, request):
qs = super().get_queryset(request)
if qs:
if is_operator(request.user):
add_fields(operator=True) #<<<<<<<<<<<<<
return qs
coy = get_user_company(request)
if coy:
qs = qs.filter(company_id=coy.id)
if is_consultant(request.user):
add_fields() #><<<<<<<<<<<<<
return qs
if is_manager(request.user):
trim_fields() #><<<<<<<<<<<<<
return qs
#
# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #

model = Receipt
readonly_fields = receipt_fields_ro
fieldsets = (
(
"Detail for this Invoice/Receipt",
{
"fields": receipt_fields,
},
),
)


--
Signed email is an absolute defence against phishing. This email has
been signed with my private key. If you import my public key you can
automatically decrypt my signature and be sure it came from me. Just
ask and I'll send it to you. Your email software can handle signing.

[Michael Thomas]

This has a pretty bad "smell", and I could imagine it being quite brittle unless developers tip-toe around the code.

I'd suggest replacing this by implementing get_readonly_fields(), which returns a fixed list based on the user type. It's much simpler, and much more difficult to get wrong, in my opinion at least!

Kind Regards,

Michael Thomas

--
You received this message because you are subscribed to the Google Groups "Django users" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-users...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-users/a8fc7ec5-96a9-87f8-c3ba-f89312cdbad9%40dewhirst.com.au.

[Antonis Christofides]

Hi,

First of all, you should type REC_FIELDS and CONS_FIELDS, in capitals, as these are constants (this is not a Python language requirement but widely used style, also suggested by PEP8 iirc).

Then, it seems to me that most of the times you don't need the "global" keyword in your code, because you aren't assigning to "receipt_fields". "receipt_fields" is a name that points to an array, and you are only manipulating the array to which "receipt_fields" is pointing, you aren't actually changing what the name is pointing to. For example:

    def hello(field):
        b = receipt_fields
        b.remove(field)

This will effectively remove field from receipt_fields, because "b" and "receipt_fields" are names that point to the same thing.

There is one exception in your code: at some point you are assigning to "receipt_fields_ro" inside a method. In that case, the "global" statement is necessary. However, the code looks quite bad to me, but I'm not looking into it further in order to not cover too many issues in this discussion.

Regardless whether you need to use the "global" keyword or not, you are using global variables, and this is a bad idea. It will work in some cases but eventually it will cause you headaches when you are least expecting them. I think it will work as long as each Django processes is serving a single HTTP request at a time. But one day you will use gunicorn's gevent worker, or something that runs Django in threads instead of processes, or async, and you will be in a huge mess, because when two users access the site at the same time the Django thread that is serving one user will be altering the global variables that are being used by the Django thread serving the other user. It will be a real pain.

Besides that, this style is quite unusual.

As for a proper solution, I don't remember exactly how the admin works, but if you can verify that ReceiptAdmin is instantiated in each request (rather than when Django starts), then you can override its __init__() method and set the variables you need in the object:

    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.receipt_fields = REC_FIELDS[:]
        ...

If ReceiptAdmin is instantiated when Django starts and the same object is thus used for many requests, you could hijack the "request" object, which might not be the best solution but will work way better:

    def get_queryset(self, request):
        qs = super().get_queryset(request)

        request.receipt_fields = REC_FIELDS[:]
        ...

Regards,

Antonis

[Mike Dewhirst]

On 19/05/2022 11:39 pm, Antonis Christofides wrote:
>
> Hi,
>
> First of all, you should type REC_FIELDS and CONS_FIELDS, in capitals,
> as these are constants (this is not a Python language requirement but
> widely used style, also suggested by PEP8 iirc).
>

Agreed

And furthermore I agree with you and Michael that the code smells.
That's why I asked for help. Thank you very much for taking the trouble
to review it.

Aaaaargh!

Just looked at the Admin source and I see it has exactly what I want -
get_fields(self, request, obj=None) and get_readonly_fields(self,
request, obj=None)

Sorry to have wasted your time.

Thanks again.

Mike

> Then, it seems to me that most of the times you don't need the
> "global" keyword in your code, because you aren't assigning to
> "receipt_fields". "receipt_fields" is a name that points to an array,
> and you are only manipulating the array to which "receipt_fields" is
> pointing, you aren't actually changing what the name is pointing to.
> For example:
>
>     def hello(field):
>         b = receipt_fields
>         b.remove(field)
>

> This will effectively remove field from *receipt_fields*, because "b"

> --
> You received this message because you are subscribed to the Google
> Groups "Django users" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to django-users...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/django-users/c5baa1c9-abbb-e156-c501-ada21363ba15%40antonischristofides.com
> <https://groups.google.com/d/msgid/django-users/c5baa1c9-abbb-e156-c501-ada21363ba15%40antonischristofides.com?utm_medium=email&utm_source=footer>.

[Mike Dewhirst]

Thank you Antonis and Michael - this is how it ended up ...

Cheers

Mike

REC_FIELDS = [
"invoice",
"amount_due",
"gst_amount",
"amount_paid",
"amount_owing",
"paid_by",
"date_paid",
]


CONS_FIELDS = [

"gateway_flagfall",
"gateway_rate",
"gateway_amount",
"gateway_gst_amount",
"net_amount",
]

class ReceiptAdmin(admin.ModelAdmin):

model = Receipt

def get_readonly_fields(self, request, obj=None):
receipt_fields_ro = REC_FIELDS[:]
receipt_fields_ro.extend(CONS_FIELDS[:])
if is_operator(request.user):
receipt_fields_ro.remove("amount_due")
receipt_fields_ro.remove("gst_amount")

receipt_fields_ro.remove("amount_paid")
receipt_fields_ro.remove("date_paid")

return receipt_fields_ro

def get_fields(self, request, obj=None):
receipt_fields = REC_FIELDS[:]
if is_operator(request.user) or is_consultant(request.user):
for field in CONS_FIELDS:
receipt_fields.append(field)
return receipt_fields

# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #
#
def get_queryset(self, request):
qs = super().get_queryset(request)

if is_operator(request.user):

return qs
coy = get_user_company(request)

if is_consultant(request.user) or is_manager(request.user):
return qs.filter(company_id=coy.id)
return qs.filter(company_id=-1)

#
# # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # # #

def get_fieldsets(self, request, obj=None):
return [

(
"Detail for this Invoice/Receipt",
{

'fields': self.get_fields(request, obj)
}
)
]