Implement carbon values by locality and fix Location entries in database#1009
Implement carbon values by locality and fix Location entries in database#1009
Conversation
BradHN1
requested review from
Opoku-Agyemang,
abdullai-t,
archx3 and
frimpongopoku
|
|
||
| if not action: return 0 | ||
|
|
||
| if hasattr(action, "status"): |
There was a problem hiding this comment.
This condition will always pass if status is a field on the action model. hasattr doesn't check for the value but the key. I don't know if that's what you want.
There was a problem hiding this comment.
There is probably a better way to do this. I want to tell whether action is a UserActionRel or an Action, so I checked whether the object has a status attribute which would indicate it was a UserActionRel.
I first tried importing those models from database.models, but that led to a circular import, since database.models imports the carbon_calculator.models.
| for team in teams: | ||
| members = team.members.all() | ||
| for member in members: | ||
| team_member = TeamMember.objects.filter( |
There was a problem hiding this comment.
@BradHN1 considering member here is already from members, is the result of TeamMember.objects.filter() not going to return the same thing and hence redundant?
There was a problem hiding this comment.
This is old code which I haven't changed, just moved it from another place; it is there in case we need it.
The reason for this routine was that the TeamMember model was introduced at some point for some purpose, and needed to be filled in using the team.members. So team.members is redundant, and could probably be eliminated but it hasn't been a priority
Opoku-Agyemang
left a comment
Opoku-Agyemang
left a comment
There was a problem hiding this comment.
left a few nit comments
| return data | ||
|
|
||
|
|
||
| def backfill_locations(): |
There was a problem hiding this comment.
i dont know if this is old code or not but this is a bit much.
At best we need to break this down into smaller functions.
In fact, maybe we should have a separate file with this as a script with several smaller functions and then we can call the main part of that script here. this is a bit hard to review
| class Meta: | ||
| db_table = 'questions_cc' | ||
|
|
||
| #class Station(models.Model): |
There was a problem hiding this comment.
how does removing this affect our migration files?
| return options | ||
| # not usual: | ||
| # invalid location information, use default | ||
| print("carbon calculator: Locality type = "+str(type(loc))+ " loc = "+str(loc)) |
There was a problem hiding this comment.
maybe you wanna use the log.info() here? if its for local testing maybe we can remove the print statement
frimpongopoku
requested review from
Opoku-Agyemang
and removed request for
frimpongopoku
4 participants
Summary / Highlights
The purpose of this development is to allow for different carbon reduction values by location, in order to support communities outside Massachusetts or those within Massachusetts which decide to customize carbon values. This functionality is described in issue #960
In implementing this i encountered problems with the Location values both for Communities and RealEstateUnits. These are described in issue #1007
Details (Give details about what this PR accomplishes, include any screenshots etc)
This PR addresses both issue #960 and #1007.
Testing Steps (Provide details on how your changes can be tested)
I've tested this PR carefully on my local database, and will also do so on Dev when deployed.
Testing:
Step 1: create a Task "Backfill Database Models" and run it one-off. This task sends an email with csv file. Look through the CSV file and if any errors (particularly fatal errors), review with Brad to get them resolved.
Step 2: Verify that community portal loads quickly (not slowed up by the additional searching).
Step 3: With Brad's help add a default value for a community location, and see that the carbon value in a user profile who has done that action is changed (once API restarted).
Requirements (place an
xin each[ ])Transparency (Project board)