Skip to content

Conversation

edwoodward
Copy link
Contributor

No description provided.

@edwoodward edwoodward self-assigned this May 12, 2022
@codecov
Copy link

codecov bot commented May 12, 2022

Codecov Report

Merging #1290 (ae91861) into main (ab95750) will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1290      +/-   ##
==========================================
+ Coverage   86.62%   86.87%   +0.25%     
==========================================
  Files         495      497       +2     
  Lines        7819     7817       -2     
==========================================
+ Hits         6773     6791      +18     
+ Misses       1046     1026      -20     

@edwoodward edwoodward requested a review from mwvolo May 12, 2022 19:47

# then we will get any new records
command = "SELECT Id, Accounts_UUID__c, Book_Text__c, Contact_Email__c, School_Name__c, Yearly_Students__c, Students__c, Type, Base_Year__c, IsWon from Opportunity WHERE OS_Accounts_ID__c != null AND Type = 'Renewal' AND Base_Year__c = {} AND IsWon = True".format(year)
command = "SELECT Id, Accounts_UUID__c, Book_Text__c, Contact_Email__c, School_Name__c, Yearly_Students__c, Students__c, Type, Base_Year__c, IsWon, Fall_Students__c, Spring_Students__c, Summer_Students__c from Opportunity WHERE Accounts_UUID__c != null AND Type = 'Renewal' AND Base_Year__c = {} AND IsWon = True".format(year)
Copy link
Member

Choose a reason for hiding this comment

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

You might want to verify with @denvergreene on if we should still check Type=Renewal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked with Denver and removed reference to type

opportunity_id=record['Id'],
defaults = {'account_uuid': record['Accounts_UUID__c'],
'book_name': record['Book_Text__c'],
'email': record['Contact_Email__c'],
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the email?

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 left most of the code and commented out code for our discussion on Monday. I have a list of questions which includes do we need all this data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed all unused fields

confirmed_yearly_students = models.IntegerField(null=True, blank=True,)
created = models.DateTimeField(auto_now_add=True)
last_update = models.DateTimeField(auto_now=True)
verified = models.BooleanField(default=False)
Copy link
Member

Choose a reason for hiding this comment

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

Probably don't need this anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed last_updated and verified

@action(methods=['get'], detail=True)
def list(self, request, account_id):
def list(self, request):
print(str(request.data))
Copy link
Member

Choose a reason for hiding this comment

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

remove debug

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed


return JsonResponse(data)

# @action(methods=['post'], detail=True)
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove commented code. We can always look at history if we need to reference it

return JsonResponse(data)
else:
return JsonResponse({'error': 'Must include adoption ID to update'})
queryset = AdoptionOpportunityRecord.objects.filter(account_uuid=account_uuid, verified=False)
Copy link
Member

Choose a reason for hiding this comment

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

I don't see setting verified anywhere, so probably remove unless I'm missing it.

@edwoodward edwoodward requested a review from mwvolo May 16, 2022 18:06
class AdoptionOpportunityRecord(models.Model):
opportunity_id = models.CharField(max_length=255, unique=True)
account_id = models.CharField(max_length=255, null=True, blank=True) # TODO: for deletion after switching to UUID
account_uuid = models.UUIDField(null=True)
Copy link
Member

Choose a reason for hiding this comment

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

wondering if it might help data consistency to have the contact id here too - since we do have that in the /api/user endpoint

@edwoodward edwoodward merged commit 5afb924 into main May 17, 2022
@edwoodward edwoodward deleted the renewal branch May 17, 2022 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants