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

Using Abstract classes? #138

Open
molt2020 opened this issue Nov 13, 2020 · 7 comments
Open

Using Abstract classes? #138

molt2020 opened this issue Nov 13, 2020 · 7 comments
Assignees
Labels
Question Questions about django-address

Comments

@molt2020
Copy link

molt2020 commented Nov 13, 2020

It would be really nice if the project would support using custom base class for models. This would allow custom extensions without creating additional tables - for example I want to add tenant_id or address_type (eg home vs office) to the Address. At the same time I may want to chose to use the default Country model as it can be shared without security/privacy issues. So it would be nice to be able to do something like this in settings.py

DJANGO_ADDRESS_ADDRESS_BASE = mymodel.TenantExtension

I think this would be useful feature to many as if you need to extend the Address model it's almost easier to roll your own then to work around the package.

Caveat: I am new to Django and Python so apologies if I have missed how this could be done easily otherwise.

@banagale
Copy link
Collaborator

@molt2020 Thanks for taking the time to share your suggestion, and welcome to Django.

Is this something that could be accomplished by subclassing the Address model?

For example: the following would use all of Django Address's primary model, Address, but tack on the two fields you provided in your example.

class MyPrivateAddress(Address):
    tenant_id = models.IntegerField()
    
    class AddressType(models.TextChoices):
        HOME = 'HO', 'Home'
        OFFICE = 'OF', 'Office'
        CAVE = 'CA', 'Cave'

    address_type = models.CharField(max_length=2, choices=AddressType.choices, null=True, blank=True)

@banagale banagale self-assigned this Nov 20, 2020
@banagale banagale added the Question Questions about django-address label Nov 20, 2020
@molt2020
Copy link
Author

I don't think this would do it - perhaps I am wrong as this is new to me. My thinking is:

  • Key benefit (for me at least) to using django-address is AddressField which can be included easily in my models
  • If I subclass Address (or any other models within django-address, e.g. locality) then AddressField would not pick that up (as it inherits address.Address) so I guess that's the key to my problem.
  • I guess I could work around this by creating an extra model that contains my own fields but this introduces more work than benefit

That's why I felt it would be ideal if base class or mixin class for key classes such as Address could be defined and then anyone can just extend the model that is provided by the package easily.

Maybe I am missing your point?

@banagale
Copy link
Collaborator

@molt2020 My apologies for the delay in responding to your feedback.

I did not realize that AddressField would pick up the fields you add in the subclass. I wonder if this also is preventing @xjlin0 from effectively using my subclass suggestion in #142.

Since your message, have you created a work around, or further explored a change to using an Abstract Class?

I need to learn more about the relationship between the model and the field to provide strong feedback. While I am maintaining this package, I am not an expert on all aspects of it yet.

If anyone else has feedback or a PR focused around this issue, please feel free to post it.

@xjlin0
Copy link
Contributor

xjlin0 commented Feb 25, 2021

When using subclass instead of using AddressField, I also experienced the "address not picked up" issue, thus it totally lost the point of using the library since it no longer calls Google API to verify the address. Here's my observation:

  • Using AddressField, it expects correct Google Map API key to pickup address, or it will fail. It does pick up address:
from address.models import AddressField

class Contact(UUIDModel, TimeStampedModel, SoftDeletableModel, Utility):
    address = AddressField(blank=True, null=True, on_delete=models.SET_NULL)

Screen Shot 2021-02-25 at 8 20 38 AM

Screen Shot 2021-02-25 at 8 23 17 AM

  • Using subclass, it does not even care about Google map API key, and address fields become plain input/CharField. It does NOT pick up address:
from address.models import Address

class Contact(Address, TimeStampedModel, SoftDeletableModel, Utility):
    address_extra = models.CharField(max_length=50, blank=True, null=True)

Screen Shot 2021-02-25 at 8 00 42 AM

Since there are actually quite some addresses need apartment number or extra field, how should I use the library with its API verification? Maybe someone can point out where did I go wrong?

@banagale
Copy link
Collaborator

@xjlin0 

If I understand you correctly, you're saying:

  • If you use the AddressField directly and do not supply an API key, it will report that you've failed to include it. (Which is correct behavior)
  • If you use the AddressField directly and provide an invalid API key, it will report that you've failed to include it. (Which is correct behavior)
  • However if you build a subclass off of the Address model, the auto complete behavior does not work at all.

I believe the third point is separate from the others.

This project is both Django stuff and some frontend code (mostly jquery) that hooks up form fields to the Google API.

Currently, that javascript does not handle looking at a formfield that does not come from AddressField.  

It is probably not hard to have the existing JS look at your custom address field's form field and think it is an AddressField, but I don't know the specific change you'd need to do off hand.

I recommend creating a working example using AddressField, then inspecting the HTML for that input element.

Then create another form field using your custom model, and compare the element attributes. Either by writing the form html manually in your template or using django's expected form creation method you can probably make this work.

That said, It does seem like many people use this library specifically to get the autocomplete feature. This puts some extra light on #139, and also possibly making it more flexible to handle subclassing as described ITT.

@xjlin0
Copy link
Contributor

xjlin0 commented May 8, 2021

Agree. However another observation using subclass is losing the ability to create instance with plain dictionary, which is easily done by using AddressField as described in the Readme, even without existing locality.

from address.models import AddressField

class Place(models.Model):
  address = AddressField(related_name='+', blank=True, null=True)

place.address = {'street_number': '1', 'route': 'Somewhere Ave', 'raw': 'whatever', 'locality': 'Northcote'.....}
place.save()  # success

Doing the same with the model subclassing Address will get complain. Still looking for a way to have address extra columns to store apt/unit #...

django address "Address.locality" must be a "Locality" instance

@banagale
Copy link
Collaborator

@xjlin0 Thanks for you continued feedback, Jack.

That sounds like a bug, but I'd need to test to be sure. 

I believe what you're saying is that when working with AddressField directly, and a string for Locality is specified, the Locality object instance will be created correctly.

However that does not work when subclassing Address

There is another ticket #109, which identifies some other issues in object instantiation in Address.

A variety of matters have kept me away from iterating on DA, but I'm hoping to make some time here. I'll try to think again on how to best provide for your additional field needs, even if as a stopgap in advance of the model architecture update I mentioned previously.

Thanks a lot for your patience and continued feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Question Questions about django-address
Projects
None yet
Development

No branches or pull requests

3 participants