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

Add UBID relation to state #4074

Merged
merged 107 commits into from
Jul 9, 2023
Merged

Add UBID relation to state #4074

merged 107 commits into from
Jul 9, 2023

Conversation

perryr16
Copy link
Contributor

@perryr16 perryr16 commented Jun 2, 2023

Any background context you want to provide?

The PropertyState (and TaxLotState) both have a ubid field that aligns with a canonical column. Per some new guidelines States can have multiple ubids. To accommodate this a new model UbidModel has been created with a many-to-one relationship with PropertyState and TaxlotState. Note that the name of the new model could not be ubid to prevent a naming conflict with the existing field.

A UbidModel will have the following fields:

  • ubid: string
  • preferred: boolean
  • property: ForeignKey
  • taxlot: ForeignKey

PNNL has developed a number of sql functions that can be migrated into a postgres db. Reference. Some of these functions allow a user to calculate the overlap of 2 unique UBID's. This is refered to as the Jaccard Index and the functions returns a value from 0 to 1 representing the match quality.
image (3)

Now that UBIDs can be compared via the jaccard index, the matching and merging processes must be updated to use a threshold value instead of an exact match.

What's this PR do?

  • Creates a relationship between -State and UbidModel. -States will still have a ubid column representing the 'prefered' ubid model
  • Adds backend CRUD endpoints
  • Adds SQL migration to the DB to leverage PNNL functions. The PNNL Function we are most interested in is UBID_CodeArea_Jaccard. This function has a number of dependent functions required to run. These dependant functions are: UBID_Decode, UBID_Parse, UBID_CodeArea, UBID_ValidateLatitude, UBID_ValidateLongitude, pluscode_isfull, pluscode_isvalid, pluscode_isshort, pluscode_decode, pluscode_codearea
  • Generates a new Organization setting for a UBID threshold. The threshold is an org setting that must be between 0 and 1.0. If the jaccard index between an incoming ubid and an existing ubid is above the UBID threshold it will qualify as a match (if the other matching criteria are met)
  • Uses signals to generate UbidModels when a new property or taxlot is uploaded with a ubid
  • Uses signals to update -State.ubid if a saved UbidModel's "preferred" field is set to true
  • Adds frontend CRUD. A new ubid viewer and editor are available from the inventory list actions, inventory detail actions, or under a new UBID inventory detail nav button. The structure is loosley based on the derived column admin and editors. New or updated ubids are validated before they may be saved.
  • Adds a ubid jaccard comparison modal to the inventory list actions. It can be triggered when 0-2 properties are selected. If fewer than 2 properties are selected a user may use custom ubids for comparison.
  • Renames the ubid_modal to ubid_decode_modal, which allows users to decode a property's ubid
  • Allows properties to be merged together while preserving the ubid history.
  • Adds a map view to the inventory detail and inventory detail ubid pages highlighting the Properties ubid bounding box. If there is no preferred ubid the map is disabled. If the preferred ubid is updated the page and map reload.

How should this be manually tested?

Run tests, test for functionality.

Possible testing procedure:

  • upload a few properties with ubids
  • set org setting ubid threshold to something small like 0.1 to allow matching
  • upload the same properties with slightly different UBIDs (ex: 85FPPRR9+3C-0-0-0-0 ---> 85FPPRR9+3C-0-0-0-1)
  • properties and ubids should have been merged with the preferred set to the incoming
  • view Inventory Detail UBIDs page, verify map
  • change the preferred UBID
  • map should update
  • create a UBID, delete a UBID
  • set org setting ubid threshold to something high like 0.9 to prevent matching
  • upload the same properties with different UBIDS (ex: 85FPPRR9+3C-0-0-0-1 ---> 85FPPRR9+3C-0-0-0-5)
  • property list should have created new properties as the ubid threshold was not met
  • set org setting ubid threshold to something small like 0.1
  • upload same properties with different UBIDs
  • existing properties should have been merged into the incoming properties
  • ensure ubid history is preserved on Inventory Detail UBIDs
  • merge 2 properties together, ensure ubid history
  • unmerge property, check ubid history

What are the relevant tickets?

#3957
#4051
#4069
#4041
#4064

Screenshots (if appropriate)

Should this page include a graphic illustrating what a jaccard match looks like (as seen in PR background)?

Screenshot 2023-06-01 at 3 13 56 PM
Screenshot 2023-06-01 at 3 10 26 PM
Screenshot 2023-06-01 at 3 11 09 PM
Screenshot 2023-06-08 at 12 55 13 PM
Screenshot 2023-06-08 at 12 55 01 PM

@@ -0,0 +1,49 @@
# Generated by Django 3.2.19 on 2023-05-11 18:49
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This migration could be moved into 0196

@perryr16 perryr16 changed the title Add UBID Frontend Add UBID relation to state Jun 16, 2023

operations = [

migrations.RunSQL(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran into a few errors attempting to import, read, and execute an entire SQL file. I'd like to refactor this to leverage imports, but I didn't want to let it become a blocker.

@axelstudios axelstudios changed the base branch from ubid-model-working-branch to develop June 26, 2023 16:40
# Conflicts:
#	locale/en_US/LC_MESSAGES/django.mo
#	locale/en_US/LC_MESSAGES/django.po
#	locale/fr_CA/LC_MESSAGES/django.mo
#	locale/fr_CA/LC_MESSAGES/django.po
#	seed/static/seed/locales/en_US.json
#	seed/static/seed/locales/fr_CA.json
@axelstudios
Copy link
Member

@perryr16 I fixed TONS of issues in this PR:

  • Major improvements and bug fixes to the UBID editor and admin pages
  • Major simplification & streamlining of the UBID comparison dialog
  • Major improvements to functionality that was being called by both signal handlers and create/update functions
  • Complete overhaul of all Swagger endpoints, and fixed incorrect documentation
  • Reordered migrations
  • Included original SQL scripts to retain licenses and greatly simplify future updates
  • Fixed UBID threshold (it could be set to a value outside of the 0-1 range via the API)
    • Fixed 500 errors related to setting bad ubid threshold values
    • Changed the input to be numeric instead of text
  • Fixed handling when ubid_threshold is zero, removed option for ubid_threshold to be null
  • Fixed incorrect detail page nav titles (UBID page showed Tax Lot Detail Notes, etc)
  • Fixed bad API response that didn't actually return the errors: {"status": "failed", "errors": "serializer.errors"}
  • Fixed bug that allowed users to change UBIDs for records outside of their organization
  • Fixed bug that allowed creation of a UbidModel for a property and taxlot combined
  • Fixed bug where directly editing the UBID on a Property Detail page wouldn't update the preferred UBID
    • Fixed bug where directly editing the UBID to an empty string wouldn't update the UBID as non-preferred
  • Fixed bug where marking the preferred UBID as non-preferred did not remove the UBID from the PropertyState
  • Fixed bug where if an initially-imported record had multiple UBIDs, and then the record was directly edited on the detail page the non-preferred ubids wouldn't exist on the new state
  • Fixed bug that allowed duplicate ubids to be added to a property or taxlot via unique constraints via two partial indexes to the ubidmodel
  • Fixed bug where the property/taxlot id could be changed
  • Removed unimplemented pagination functionality from ubid API
  • Restricted the ability to associate UBIDs with initial DATA_STATE_IMPORT records
  • Removed PATCH ubid endpoint
  • Added client-side UBID validation via ubid directive for form inputs
  • Added a new ModelViewSetWithoutPatch viewset
  • Added a migration to rename the SEED column (not TaxLotState column) from ulid to ubid
  • Added a migration to rehash all Property and TaxLot states
  • Added a manage.py rehash command for testing
  • Fixed inventory detail nav ids
  • Fixed org nav sorting
  • Added autofocus to Create UBID field input
  • Fixed countless other 500 errors

Remaining Issues & Questions

  • It would be nice to reload the map without reloading the state of the parent page
  • Should changing the UBID threshold re-compare all records with UBIDs?
  • Do we even need the ubidmodel.preferred column if we can just infer the preferred one from the state.ubid value?
  • If UBID is not a matching field, and has merge-protection enabled, then importing a matching record with a different UBID won't create a non-preferred UBID and the information will be lost
  • Identical invalid UBIDs still match, I guess that's the expected behavior

@axelstudios axelstudios merged commit d02eda8 into develop Jul 9, 2023
7 checks passed
@axelstudios axelstudios deleted the 4064-ubid-map-view branch July 9, 2023 02:08
@axelstudios axelstudios mentioned this pull request Jul 9, 2023
dhaley pushed a commit that referenced this pull request Jul 21, 2023
* rename taxlot ulid to ubid

* precommit

* rename ULID to UBID and remove duplicates

* add UbidModel class and CRUD tests

* update default mapping

* remove ulid argument from called functions

* use signal to gerneate ubidmodels

* signals creating ubids

* precommit

* one giant sql migration

* base test

* refactor

* import jaccard index from ubid utils

* pre-commit

* simplifty test

* formatting

* comparing ubids through jaccard ui

* add new endpoint for ubid by property

* base for a ubid upsert modal

* upsert modal create and delete functional

* override create to prevent multiple preferred

* frontend crud functional

* cleanup

* alter state ubid post_save and pre_delete

* validate ubid before comparing

* refactor preferred toggle

* pre-commit

* refresh data post save for test

* debugging logs

* merge ubids on import and merge action

* improve ubid update and help text

* property merge functional

* define org setting ubid_threshold

* use org setting ubid_threshold to determine matching ubids

* refactor compare ubids to take 2 ubids instead of 2 views

* add ubid modal to inv detail and update org settings for ubid

* allow comparison of 0-2 properties

* remove tooltip

* bug fix for setting preferred

* read modal functional, edit modal pops up

* list or upsert ubid via 2 modals functional

* inv det ubid upsert modal refactor

* reuse ubid admin partial for inventory list, detial, and own nav

* pre-commit

* remove ubused controller

* allow property to have missing ubid

* backfill ubid_thresholds to avoid non-null constraints

* allow null ubid_threshold, treat as exact match

* precommit

* troubleshooting ol map views

* validate ubid before creating

* rename ubid modal to ubid decode modal

* decode ubid on preferred creation or preferred update

* precommit

* rename ubid upsert to ubid admin

* validate ubid before updating

* precommit

* comment cleanup

* add in translations for ubid work

* using bounding box over building point for map starter

* decode following state update

* option 1: map with controls

* option 2: map without controls

* change create ubid button location

* styles

* controller cleanup

* precommit

* remove unnecessary layer causing console error

* map working with  centroid and bb

* remove inventory service call

* remove records and use item_state

* return if no preferred ubid

* init function

* precommit

* update init condition

* controller cleanup

* comment for future dev

* taxlot bug

* decode on merge

* reverse old states before identifying preferred ubid

* compare taxlot ubid auto populate

* refactor to enable multiple comparisons in single session

* adjust width of compare ubid edit fields

* adjust width of compare ubid edit fields

* merge taxlot ubids

* comments

* move attribution to about page and use 2d map with better resolution

* translations

* translate bug fix

* formatting

* bug fix if imported property ubid is missing

* key error fix

* typo

* Reorder migrations, include original sql scripts, update translations

* Added UBID threshold constraint

* Major UBID improvements, hugely simplified UBID comparison dialog

* Minor fixes

* Rehash

* Final huge round of bug fixes

* Update tests

---------

Co-authored-by: Alex Swindler <Alex.Swindler@nrel.gov>
@RDmitchell
Copy link

@perryr16 -- I might need some help in terms of documenting this feature.

But to start, when you are in the Property Detail view and you click on the UBIDs link, there is a Create UBID button.

It looks like you already have to know the UBID for the property, right? SEED isn't actually generating the UBIDs, is it?

And then you can enter multiple UBIDs and then I guess SEED determines which is the "preferred" UBID?

@axelstudios
Copy link
Member

@RDmitchell Yes, that Create UBID button assumes you already know a valid UBID. SEED won't currently take a property_footprint or lat/long and turn that into a UBID, but it could.

When you're on that UBID tab you can edit each one individually and select one as preferred (changing the preferred UBID automatically unsets a previously-preferred UBID and updates the ubid value for the property/taxlot

@perryr16 perryr16 mentioned this pull request Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Add this label to new features. This will be reflected in the change log when generated.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants