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

Multiple cycle upload #4073

Merged
merged 47 commits into from
Jul 3, 2023
Merged

Multiple cycle upload #4073

merged 47 commits into from
Jul 3, 2023

Conversation

anchapin
Copy link
Contributor

@anchapin anchapin commented Jun 2, 2023

Any background context you want to provide?

What's this PR do?

Allows you to upload a spreadsheet with data across multiple cycles.

How should this be manually tested?

Upload the file 'Sample_MultipleCycles.xlsx' and select the 'Multiple Cycle Upload' checkbox.

MultipleCycle Checkbox

Additionally, ensure that the SEED column 'year_ending' is mapped to the "Year Ending" column from the 'Date File Header'.

What are the relevant tickets?

#3960 #4053 #3181

Screenshots (if appropriate)

@anchapin anchapin added the Feature Add this label to new features. This will be reflected in the change log when generated. label Jun 2, 2023
seed/data_importer/match.py Outdated Show resolved Hide resolved
seed/data_importer/match.py Outdated Show resolved Hide resolved
Comment on lines 1378 to 1388
celery_chain(
_geocode_properties_or_tax_lots.si(file_pk, progress_data.key),
group(_map_additional_models.si(id_chunk, file_pk, progress_data.key) for id_chunk in id_chunks),
match_and_link_incoming_properties_and_taxlots.si(file_pk, progress_data.key, sub_progress_data.key),
finish_matching.s(file_pk, progress_data.key),
)()

sub_progress_data.total = 100
sub_progress_data.save()

return {'progress_data': progress_data.result(), 'sub_progress_data': sub_progress_data.result()}
Copy link
Contributor

Choose a reason for hiding this comment

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

This part and it's counterpart above can be moved out of the if/else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really? How would that work? Lines 1355 and 1380 are different and I didn't think it would work if they were moved out of the if/else.

@@ -1580,7 +1620,7 @@ def _get_field_from_obj(field_obj, field):


@shared_task
def _map_additional_models(ids, file_pk, progress_key):
def _map_additional_models(ids, file_pk, progress_key, cycle=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Idk if this whole function needs to be branched like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would you suggest? Should I make a helper function again like the match_and_link_incoming_properties_and_taxlots function in 'match.py'?

Comment on lines 1670 to 1681
id_chunks = [[obj.id for obj in chunk] for chunk in batch(ids, 100)]

progress_data.total = (
1 # geocoding
+ len(id_chunks) # map additional models tasks
+ 2 # match and link
+ 1 # finish
)
progress_data.save()

for id_chunk in id_chunks:
property_states = PropertyState.objects.filter(id__in=id_chunk).prefetch_related('building_files')
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these chunks are doing anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These chunks were existing and I wasn't sure how to check whether it was doing anything or not. Any suggestions?

@haneslinger
Copy link
Contributor

Just documenting a known problem: Should a property's year ending not fit into any existing cycles, the API 500s and the UI hangs.

@haneslinger
Copy link
Contributor

idk if this is intentional, but If you selected multi-cycle and a cycle, it will only upload the properties that are part of the selected cycle

image

>>> PropertyView.objects.values("cycle__name")
<QuerySet [{'cycle__name': '2018 Calendar Year'}]>

Copy link
Contributor

@haneslinger haneslinger left a comment

Choose a reason for hiding this comment

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

sweet!

seed/data_importer/tasks.py Outdated Show resolved Hide resolved
@anchapin
Copy link
Contributor Author

Note that tax lots don't work (in that they only get added to the first cycle listed in the upload file) but it doesn't break SEED. This seems sufficient for now since the first known customer for this feature indicated that they aren't planning to use tax lots initially.

There is an issue with "Year Ending" just being a year value like "2019" though. It won't find a matching cycle and always use the default cycle.

from datetime import date, datetime to match
other test files. Also added spreadsheet
to match the test
@axelstudios axelstudios force-pushed the multiple_cycle_upload branch 2 times, most recently from b5870f3 to 0e5cf14 Compare July 3, 2023 17:09
Added date verification to cycle admin page
@axelstudios axelstudios merged commit 41fa04b into develop Jul 3, 2023
7 checks passed
@axelstudios axelstudios deleted the multiple_cycle_upload branch July 3, 2023 21:32
dhaley pushed a commit that referenced this pull request Jul 21, 2023
* getting close to mvp just squashing bugs

* Fixed the bugs and have mvp ready

* Anonomized data

* Renamed sample file

* Pass celery state ids instead of states

* Resolve a review comment on the modal code

* Clean

* Fixes the hang issue

* unittest for multi cycle upload

* cleanup

* Cycle conversion to date

* update test for edge cases

* update cycle filter

* Update to multiple_cycle_upload changes

* Updated cycle filter again

* Finished other than modal bug

* Modal bug is fixed

* year ending using date over datetime

* precommit

* cycle start and end to date from datetime api test

* add test for cycle spanning a typical calendar year

* test formatting for clarity

* precommit

* cycle start and end to date from datetime unit test

* cycle date formatting without timezone

* Fixed the blank year_ending case and PR comments

* precommit

* revert cycle start and end to datetime

* convert year ending to awawre datetime

* add help text to multicycle checkbox

* translations

* add an info icon hover to display help text

* Migration

* Fixed issues for release

* Migration conflict

* Minor change to a comment

* Switched to import datetime instead of
from datetime import date, datetime to match
other test files. Also added spreadsheet
to match the test

* Converted cycle datetime fields to date fields
Added date verification to cycle admin page

* Improved model

* Performance improvement

* Consistent date usage in tests

---------

Co-authored-by: Hannah Eslinger <heslinge@nrel.gov>
Co-authored-by: Ross Perry <perryr16@gmail.com>
Co-authored-by: Alex Swindler <Alex.Swindler@nrel.gov>
@RDmitchell
Copy link

@anchapin -- do the all the cycles that correspond to the Year Ending field need to be defined in order for the records to get associated with those cycles?

@RDmitchell
Copy link

@anchapin -- also, in this test file, is the first record, with year ending set to 1/1/2021, supposed to be part of the 2020 cycle? or a 2021 cycle?
image

@RDmitchell
Copy link

@anchapin -- does it matter what the default cycle is set to when the Multiple Cycle box is checked?
image

@RDmitchell
Copy link

@anchapin -- I think never mind to the default cycle question -- it appears that is the cycle used if the Year Ending field is blank.

@RDmitchell
Copy link

@anchapin -- so I guess the date in the Year Ending field just has to be equal to or between the dates defined in the cycle, right? so the record with year ending of 1/1/2021 would be associated the the 2021 cycle if the cycle were defined as 01/01/2021 - 12/31/2021, correct? That seems to be the way the program is interpreting it. Just need to make sure I understand the mechanics so I can document it. Thx

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

5 participants