-
Notifications
You must be signed in to change notification settings - Fork 7
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
PY3 extension upgrade #350
Conversation
…th main inventory-app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, but a few more things to do.
We need to get the extension installed in cloud.gov; we can either install it in the cfstart script (my preference) or add it to the requirements install.
We'll also need to enable the extension in the production.ini file.
'revision_list': datagov_disallow_anonymous_access(), | ||
'revision_show': datagov_disallow_anonymous_access(), | ||
'package_show': datagov_disallow_anonymous_access(), | ||
'resource_show': datagov_allow_anonymous_access, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we just want to add package_show and resource_show, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put a comment on the issue ticket, but there revision history is no longer supported, so those keywords no longer exist either..
cy.visit('/dataset'); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still want this line here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was to make sure the fixture was captured in the right test. Without this line, the fixture was just run as part of the next test. I don't know if we can write the test differently to capture the fixture properly.
cy.delete_dataset('test-dataset-1') | ||
cy.delete_organization('test-organization') | ||
}) | ||
|
||
it('Cannot access the standard public pages', () => { | ||
cy.logout(); | ||
cy.request({ | ||
url: '/dataset', | ||
failOnStatusCode: false | ||
}).then((response) => { | ||
// TODO: local extension should return 403 on anonymous access |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can delete this line too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By "this", I mean this line: // TODO: local extension should return 403 on anonymous access
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -1,15 +1,12 @@ | |||
describe('Login', () => { | |||
|
|||
it('Invalid user login attempt', () => { | |||
cy.visit('/dataset') | |||
cy.get('a[href="/user/login"]').click() | |||
cy.visit('/user/login') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need this, it's handled in the cy.login
command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
# TODO: re-integrate datagov_inventory, dcat_usmetadata, usmetadata, datajson, saml2auth | ||
ckan.plugins = datastore xloader stats text_view recline_view googleanalyticsbasic s3filestore envvars | ||
# TODO: re-integrate dcat_usmetadata, usmetadata, datajson, saml2auth | ||
ckan.plugins = datagov_inventory datastore xloader stats text_view recline_view googleanalyticsbasic s3filestore envvars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all good, but it's not added to production.ini
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Also, would be nice if we could pin pygments to pygments@2.7.4 in the dev requirements, snyk has multiple vulnerabilities with that (it's pulled in via sphinx and ipdb). Or try unpinning sphinx and ipdb... This may break python2/ckan2.8, but |
PR for GSA/data.gov#3388