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

Check the value of a global account manager before it is transformed #2474

Merged
merged 3 commits into from
Mar 5, 2020

Conversation

cgsunkel
Copy link
Contributor

@cgsunkel cgsunkel commented Mar 5, 2020

Description of change

This PR fixes a bug that causes Data Hub to hang and eventually leads to a 504 error when a user clicks on ‘View full business details’ on affected one list companies (this also appears in Sentry). An example of this can be found here - an error message should appear in the 'data-hub-sentry' channel after the link is clicked.

The bug is caused if someone using Django Admin changes a company to the one list tier but does not set the one list account owner. In the code, the translator always expects one_list_global_account_manager to be defined and throws an unexpected error when encountering 'null'

This PR allows the transformer to check the value of one_list_global_account_manager before the transformation process starts, and returns a value of ‘not set’ if the variable hasn’t been set beforehand.

Test instructions

Go the the company identified above in the Heroku app and the business details page should appear normally

Screenshots

Before

Screenshot 2020-03-05 at 09 49 28

After

A normal business details page should appear

Checklist

  • Has the branch been rebased to master?
  • Automated tests (Any of the following when applicable: Unit, Functional or Acceptance)
  • Manual compatibility testing (Browsers: Chrome, Firefox, IE11, Safari)

@cgsunkel cgsunkel added the bug label Mar 5, 2020
@cgsunkel cgsunkel requested a review from a team March 5, 2020 10:21
@data-hub-bot data-hub-bot temporarily deployed to data-hub-fro-fix-global-kvm3kc March 5, 2020 10:22 Inactive
@cgsunkel cgsunkel force-pushed the fix/global-account-manager-is-null branch from 6739d98 to f23b428 Compare March 5, 2020 10:30
@codecov
Copy link

codecov bot commented Mar 5, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@bf18b32). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2474   +/-   ##
=========================================
  Coverage          ?   87.41%           
=========================================
  Files             ?      336           
  Lines             ?     5922           
  Branches          ?        0           
=========================================
  Hits              ?     5177           
  Misses            ?      745           
  Partials          ?        0
Impacted Files Coverage Δ
...ps/companies/apps/business-details/transformers.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf18b32...8c647b5. Read the comment docs.

@data-hub-bot data-hub-bot temporarily deployed to data-hub-fro-fix-global-kvm3kc March 5, 2020 10:30 Inactive
@data-hub-bot data-hub-bot temporarily deployed to data-hub-fro-fix-global-kvm3kc March 5, 2020 11:16 Inactive
@cgsunkel cgsunkel requested a review from a team March 5, 2020 11:48
@data-hub-bot data-hub-bot temporarily deployed to data-hub-fro-fix-global-kvm3kc March 5, 2020 12:39 Inactive
@cgsunkel cgsunkel requested a review from web-bert March 5, 2020 12:46
@data-hub-bot data-hub-bot temporarily deployed to data-hub-fro-fix-global-kvm3kc March 5, 2020 13:06 Inactive
@cgsunkel cgsunkel force-pushed the fix/global-account-manager-is-null branch from 5c5c319 to 8c647b5 Compare March 5, 2020 13:14
@data-hub-bot data-hub-bot temporarily deployed to data-hub-fro-fix-global-kvm3kc March 5, 2020 13:14 Inactive
@cgsunkel cgsunkel merged commit 704a9d0 into master Mar 5, 2020
@cgsunkel cgsunkel deleted the fix/global-account-manager-is-null branch March 5, 2020 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants