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

Remove returning stored site user #311

Closed
wants to merge 2 commits into from

Conversation

Zharktas
Copy link
Member

@Zharktas Zharktas commented Feb 5, 2018

Site user is lost when validation fail during package_create.

This might fix #151.

@metaodi
Copy link
Member

metaodi commented Feb 5, 2018

To fix the build add the following lines to .travis.yml:

# the new trusty images of Travis cause build errors with psycopg2, see https://github.com/travis-ci/travis-ci/issues/8897
dist: trusty
group: deprecated-2017Q4

@metaodi metaodi self-requested a review February 5, 2018 12:39
@metaodi
Copy link
Member

metaodi commented Feb 5, 2018

@Zharktas what do you mean with "site user is lost"? Lost how? The method only returns the username and not a user object, and the username won't change. Or am I missing something?

@Zharktas
Copy link
Member Author

Zharktas commented Feb 5, 2018

@metaodi
Copy link
Member

metaodi commented Feb 7, 2018

Okay just to get it right, if the user has been previously fetched (i.e. self._user_name is set) then the following line is not executed:

self._site_user = p.toolkit.get_action('get_site_user')(context, {})

So far, everything is clear.

What I don't understand is: why is it a problem if that line is not executed? Do we need self._site_user somewhere? In most cases _get_user_name() will just return harvest (btw: the above line is also not executed if there is a harvest user on the system).
My guess by looking at the source of get_site_user() is, that the model.Session.flush() plays a role, but I'm not sure.

I don't think, that it's a good idea to call get_site_user() for every dataset in the harvester, so to save the username in a variable seems like a good idea.

@Zharktas
Copy link
Member Author

Zharktas commented Feb 7, 2018

I'll test tomorrow if reorganizing the code will help the issue. Either validate the dataset before any SQL magic happens or catch validation errors properly.

@Zharktas
Copy link
Member Author

Zharktas commented Feb 8, 2018

Closed in favor of #312

@Zharktas Zharktas closed this Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DetachedInstanceError when harvesting CKAN
2 participants