Skip to content
This repository has been archived by the owner on Jan 14, 2020. It is now read-only.

Setup Location resolvers to work with NewSpring Specific data #55

Merged
merged 5 commits into from
May 2, 2019

Conversation

IsaacHardy
Copy link
Contributor

DESCRIPTION

What does this PR do, or why is it needed?

We need to update resolvers to work with null or undefined data. This PR protects from that.

How do I test this PR?

Go through the on boarding process and make sure newspring campuses are available.

I am affirming this is my best work (Ecclesiastes 9:10)

TODO

  • PR has a relevant title that will be understandable in a public changelog (ie...non developers)
  • Closes DEV-XXX
  • No new warnings in tests, in storybook, and in-app
  • Upload GIF(s) of iOS and Android if applicable
  • Set two relevant reviewers

REVIEW

Manual QA

  • Manual QA on iOS and ensure it looks/behaves as expected
  • Manual QA on Android and ensure it looks/behaves as expected

Code Review: Questions to consider

  • Read through the "Files changed" tab very carefully
  • Edge cases: what assumptions are made about input?
  • What kind of tests could be written?
  • How might we make this easier for someone else to understand?
  • Could the code be simpler?
  • Will the code be easy to modify in the future?
  • What's one part of these changes that makes you excited to merge it?

The purpose of PR Review is to improve the quality of the software.

@IsaacHardy IsaacHardy added the ready for review This is ready to be reviewd label May 2, 2019
@codecov-io
Copy link

codecov-io commented May 2, 2019

Codecov Report

Merging #55 into master will decrease coverage by 0.19%.
The diff coverage is 12.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #55     +/-   ##
=========================================
- Coverage   56.37%   56.18%   -0.2%     
=========================================
  Files         150      150             
  Lines        1717     1723      +6     
  Branches      181      185      +4     
=========================================
  Hits          968      968             
- Misses        668      670      +2     
- Partials       81       85      +4
Impacted Files Coverage Δ
...s/apollos-church-api/src/data/campuses/resolver.js 0% <0%> (ø) ⬆️
packages/apollos-church-api/src/data/utils.js 37.5% <16.66%> (-37.5%) ⬇️

@richarddubay
Copy link
Member

This is pulling campuses that should be closed. It's also pulling 'Web' ... I'm pretty sure we shouldn't show that as a campus.

Screenshot 2019-05-02 16 01 03

@IsaacHardy
Copy link
Contributor Author

@richarddubay This PR's code is only to protect against null and undefined values. I believe the issue with bad data comes from Rock specific data. We should be able to do that from the admin panel.

@richarddubay
Copy link
Member

Okay, that's cool. I was worried that we were pulling campuses that should be closed ... and maybe that would need to be addressed here. But we can look to a later time for that.

richarddubay
richarddubay previously approved these changes May 2, 2019
@richarddubay richarddubay added merge When the tests pass, merge! and removed ready for review This is ready to be reviewd labels May 2, 2019
@IsaacHardy IsaacHardy removed the merge When the tests pass, merge! label May 2, 2019
@IsaacHardy
Copy link
Contributor Author

@richarddubay I'm updating the createAssetUrl. Had a better idea for it.

if (value.path) {
return value.path;
}
if (value.Key && value.AssetStorageProviderId) {
Copy link
Member

Choose a reason for hiding this comment

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

Just covering :allthethings:. Nice.

@richarddubay richarddubay added merge When the tests pass, merge! and removed ready for review This is ready to be reviewd labels May 2, 2019
@richarddubay richarddubay merged commit 2c8efa3 into master May 2, 2019
@richarddubay richarddubay deleted the location branch May 2, 2019 20:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
merge When the tests pass, merge!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants