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

DAH-2643 handle diff response #2289

Merged
merged 9 commits into from
Sep 12, 2024

Conversation

tallulahkay
Copy link
Contributor

@tallulahkay tallulahkay commented Sep 10, 2024

Description

When the change to a salesforce field is large, the event from Salesforce sends a diff instead of the whole content. This PR gets the content from the listing when a diff is received and uses the content from the listing to get the translation.

Jira ticket

DAH-2643

Checklist before requesting review

Version Control

  • branch name begins with angular if it contains updates to Angular code
  • branch name contains the Jira ticket number
  • PR name follows type: TICKET-NUMBER Description format, e.g. feat: DAH-123 New Feature

Code quality

Review instructions

  • instructions specify which environment(s) it applies to
  • instructions have already been performed at least once
  • instructions can be followed by PA testers
  • instructions specify if it can only be followed by an engineer

Request review

  • PR has needs review label
  • Use Housing Eng group to automatically assign reviewers, and/or assign specific engineers
  • If time sensitive, notify engineers in Slack

@hshaosf hshaosf temporarily deployed to dahlia-webapp-pr-2289 September 10, 2024 00:18 Inactive
@tallulahkay tallulahkay temporarily deployed to dahlia-webapp-pr-2289 September 10, 2024 16:12 Inactive
@tallulahkay tallulahkay added the needs review Pull request needs review label Sep 10, 2024
@tallulahkay tallulahkay marked this pull request as ready for review September 10, 2024 16:22
@tallulahkay tallulahkay temporarily deployed to dahlia-webapp-pr-2289 September 10, 2024 16:26 Inactive
@tallulahkay tallulahkay temporarily deployed to dahlia-webapp-pr-2289 September 10, 2024 16:35 Inactive
@tallulahkay tallulahkay force-pushed the DAH-2643-handle-diff-response-salesforce-events branch from 561c454 to 81bcd0a Compare September 10, 2024 16:35
@tallulahkay tallulahkay temporarily deployed to dahlia-webapp-pr-2289 September 10, 2024 16:35 Inactive
@tallulahkay tallulahkay temporarily deployed to dahlia-webapp-pr-2289 September 10, 2024 23:44 Inactive
return [] unless values&.values

listing = Request.new(parse_response: true).cached_get(
"/ListingDetails/#{CGI.escape(listing_id)}", nil, true
).first
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: This will update the cache for the listing, will there be any weird interactions with the behavior getting implemented in this PR: #2283 ?

Also, I'm a bit apprehensive about mixing concerns, i.e. updating the Listing cache from within this service.

Copy link
Contributor Author

@tallulahkay tallulahkay Sep 11, 2024

Choose a reason for hiding this comment

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

Yeah good point - we can just not do a get without worrying about the cache in this case

return [] unless values&.values

listing = Request.new(parse_response: true).cached_get(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we place the code for handling the diff in another function?

We shouldn't have to get the listing everytime in this function, only if we encounter a diff object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's tough because we only know if it's a diff when we look at the individual values. Having to check if there are diffs ahead of processing means we have to loop through all the values an extra time. I pushed an update so the listing gets called on the first diff only. Let me know what you think!

@tallulahkay tallulahkay force-pushed the DAH-2643-handle-diff-response-salesforce-events branch from f74573c to bab6bf8 Compare September 11, 2024 21:52
@tallulahkay tallulahkay force-pushed the DAH-2643-handle-diff-response-salesforce-events branch from ee483dd to 56b44cf Compare September 11, 2024 21:55
@tallulahkay tallulahkay force-pushed the DAH-2643-handle-diff-response-salesforce-events branch from 56b44cf to bab6bf8 Compare September 11, 2024 21:56
@tallulahkay tallulahkay temporarily deployed to dahlia-webapp-pr-2289 September 12, 2024 15:26 Inactive
@tallulahkay tallulahkay temporarily deployed to dahlia-webapp-pr-2289 September 12, 2024 16:12 Inactive
@tallulahkay tallulahkay temporarily deployed to dahlia-webapp-pr-2289 September 12, 2024 16:25 Inactive
@@ -24,11 +24,7 @@
},
'LastModifiedDate' => '2024-06-29T19:09:24Z',
'Name' => '1075 Market St Unit 206 Update 3',
'Credit_Rating__c' => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible for us to have a test that explicitly handles the diff case?

@tallulahkay tallulahkay force-pushed the DAH-2643-handle-diff-response-salesforce-events branch from 034485a to b2bab11 Compare September 12, 2024 18:14
@tallulahkay tallulahkay force-pushed the DAH-2643-handle-diff-response-salesforce-events branch from b2bab11 to 46472ca Compare September 12, 2024 18:15
@tallulahkay tallulahkay temporarily deployed to dahlia-webapp-pr-2289 September 12, 2024 18:15 Inactive
Copy link
Collaborator

@jimlin-sfgov jimlin-sfgov left a comment

Choose a reason for hiding this comment

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

LG

works for rich text too!

:ZH=>"<p><span style=\"color: rgb(68, 68, 68);\">除了符合收入和家庭规模规则的资格外,申请人还必须符合建筑物的规则</span></p><p><br></p><p><strong style=\"color: rgb(0, 0, 0); font-size: 16px; background-color: rgb(255, 255, 255); font-family: courier;\">照顾病人、遵守医生的要求很重要,但这是一个充满痛苦和磨难的时期。细究起来,任何人都不应该从事任何一种工作,除非他能从中得 到一些好处。不要在痛中怒,在训斥中在快感中,他要从痛中做一根毛,希望没有滋生。除非被情欲蒙蔽了双眼,否则就不会出来;放弃职责而软化内心的人就是过错,那就是辛劳。</strong></p><p><br></p><p style=\"text-align: right;\"> <em style=\"color: rgb(0, 0, 0); font-size: 9px; background-color: rgb(255, 255, 255); font-family: serif;\">照顾病人、遵守医生的要求很重要,但这是一个充满痛苦和磨难的时期。细究起来,任何人都不应该从事任何一种工作,除非他能从中得到一些好处。不要在痛中怒,在训斥中在快感中,他要从痛中做一根毛,希望没有滋生。除非被情欲蒙蔽了双眼,否则就不会出来;放弃职责而软化内心的人就是过错,那就是辛劳。</em></p><p style=\"text-align: right;\"><br></p><p style=\"text-align: center;\"> <strike style=\"color: rgb(0, 0, 0); font-size: 36px; background-color: rgb(255, 255, 255); font-family: verdana;\">照顾病人、遵守医生的要求很重要,但这是一个充满痛苦 和磨难的时期。细究起来,任何人都不应该从事任何一种工作,除非他能从中得到一些好处。不要在痛中怒,在训斥中在快感中,他要从痛中做一根毛,希望没有滋生。除非被情欲蒙蔽了双眼,否则就不会出来;放弃职责而软化内心的人就是过错,那就是辛劳。</strike></p><p style=\"text-align: center;\"><br></p><ol><li><span style=\"color: rgb(0, 0, 0); font-size: 14px; background-color: rgb(255, 255, 255);\">照顾病人、遵守医生的要求很重要,但这是一个充满痛苦和磨难的时期。细究起来,任何人都不应该从事任何一种工作,除非他能从中得到一些好处。不要在痛中怒,在训斥中在快感中,他要从痛中做一根毛,希望没有滋生。除非被情欲蒙蔽了双眼,否则就不会出来;放弃职责而软化内心的人就是过错,那就是辛劳。</span></li><li><span style=\"color: rgb(0, 0, 0); font-size: 14px; background-color: rgb(255, 255, 255);\">照顾病人、遵守医生的要求很重要,但这是一个充满痛苦和磨难的时期。细究起来,任何人都不应该从事任何一种工作,除非他能从中得到一些好处。不要在痛中怒,在训斥中在快感中,他要从痛中做一根毛,希望没有滋生。除非被情欲蒙蔽了双眼,否则就不会出来;放弃职责而软化内心的人就是过错,那就是辛劳。</span></li><li><span style=\"color: rgb(0, 0, 0); font-size: 14px; background-color: rgb(255, 255, 255);\">照顾病人、遵守医生的要求很重要,但这是一个充满痛苦和磨难的时期。细究起来,任何人都不应该从事任何一种工作,除非他能从中得到一些好处。不要在痛中怒,在训斥中在快感中,他要从痛中做一根毛,希望没有滋生。除非被情欲蒙蔽了双眼,否则就不会出来;放弃职责而软化内心的人就是过错,那就是辛劳。</span></li><li><span style=\"color: rgb(0, 0, 0); font-size: 14px; background-color: rgb(255, 255, 255);\">照顾病人、遵守医生的要求很重要,但这是一个充满痛苦和磨难的时期。细究起来,任何人都不应该从事任何一种工作,除非他能从中得到一些好处。不要在痛中怒,在训斥中在快感中,他要从痛中做一根毛,希望没有滋生。除非被情欲蒙蔽了双眼,否则就不会出来;放弃职责而软化内心的人就是过错,那就是辛劳。</span></li></ol>"

if listing.nil?
listing = Request.new(parse_response: true).get(
"/ListingDetails/#{CGI.escape(listing_id)}",
).first
Copy link
Collaborator

Choose a reason for hiding this comment

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

suggestion (non-blocking): Do a little extra computation to avoid nesting conditionals

if values.values.all? { |value| value.is_a?(String) }
  listing = nil
else
  listing = Request.new(parse_response: true).get(
    "/ListingDetails/#{CGI.escape(listing_id)}",
  ).first
end

values.each do |key, value|
  # ...
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The extra computation is exactly what I was trying to avoid - do you think the extra computation is ok in this case?

Copy link
Collaborator

@jimlin-sfgov jimlin-sfgov Sep 12, 2024

Choose a reason for hiding this comment

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

I think it's okay, this computation doesn't seem too resource-heavy.

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'm going to leave it as is - it seems the linter thinks that bumps the cyclomatic complexity up too high (probably bc it adds a second loop)

@tallulahkay tallulahkay temporarily deployed to dahlia-webapp-pr-2289 September 12, 2024 20:22 Inactive
@tallulahkay tallulahkay merged commit d9504f6 into main Sep 12, 2024
18 checks passed
@tallulahkay tallulahkay deleted the DAH-2643-handle-diff-response-salesforce-events branch September 12, 2024 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review Pull request needs review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants