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

589 handle subauthorities #592

Merged
merged 33 commits into from May 31, 2019
Merged

589 handle subauthorities #592

merged 33 commits into from May 31, 2019

Conversation

hudajkhan
Copy link
Contributor

@hudajkhan hudajkhan commented May 30, 2019

Fixes #589

Updated apidoc to enable calling all methods (spaces needed to be removed from tags) using current pattern for calls. If subauthority present, subauthority API call is made. Otherwise authority API is used.

@coveralls
Copy link

coveralls commented May 30, 2019

Coverage Status

Coverage decreased (-0.1%) to 82.8% when pulling ffa65ef on 589_handleSubauthorities into 4e645b3 on master.

src/components/editor/InputLookupQA.jsx Outdated Show resolved Hide resolved
src/components/editor/InputLookupQA.jsx Outdated Show resolved Hide resolved
@mjgiarlo
Copy link
Member

@hudajkhan Can you also amend the test suites for InputLookupQA and Config (once changed) to reflect the changes you've made?

@hudajkhan
Copy link
Contributor Author

Added a test for Config. Mulling over test for input qa. The changes to inputLookupQA were around identifying if a subauthority is passed from lookup config and then calling the right method from the apidoc.

@@ -7,6 +7,7 @@ import { connect } from 'react-redux'
import { getProperty } from '../../reducers/index'
import { changeSelections } from '../../actions/index'
import { booleanPropertyFromTemplate, defaultValuesFromPropertyTemplate } from '../../Utilities'
import Config from '../../../src/Config'
Copy link
Member

Choose a reason for hiding this comment

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

This component is already in the src/ directory, so I think the path should be '../../Config'

src/Config.js Outdated
@@ -63,6 +63,10 @@ class Config {
static get cognitoTestUserPass() {
return process.env.COGNITO_TEST_USER_PASS
}

static get maxRecordsForQALookups() {
return process.env.MAX_RECORDS_FOR_QA_LOOKUPS || 8
Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks!

Copy link
Member

@mjgiarlo mjgiarlo left a comment

Choose a reason for hiding this comment

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

Looking good, @hudajkhan! Happy to merge this once the build goes 💚

@hudajkhan
Copy link
Contributor Author

Having an issue with the test I intended to evaluate the function for selecting the API. I'm commenting it out for now until I figure out how to do this properly (I'm also running into interesting challenges on being able to run tests on any of my local environments but that is a different story). thanks @jgreben for helping me step through some of it!

@hudajkhan
Copy link
Contributor Author

Down to 0 errors and 12 warnings but the cutoff is 11 warnings.

@mjgiarlo
Copy link
Member

Hi, @hudajkhan! Yesterday, I believe @jermnelson said that your Sinopia time was running low. If you don't have time to knock out the last warning, I'd be happy to pick up this branch, work on the build, rebase on master, and help finish. How would you like to proceed?

@hudajkhan
Copy link
Contributor Author

Hi @mjgiarlo ! If you could help me understand how to knock out that last warning (and maybe how to properly write the test), that would be great. I can then update the code. (fyi I slacked you too)

Sinopia Work-Cycle One automation moved this from Needs Review to In Progress May 31, 2019
@mjgiarlo mjgiarlo merged commit 9e8b1e0 into master May 31, 2019
Sinopia Work-Cycle One automation moved this from In Progress to Done May 31, 2019
@mjgiarlo mjgiarlo deleted the 589_handleSubauthorities branch May 31, 2019 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Ensure QA lookups refer to subauthorities and make correct call
3 participants