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

Added separate user projection in query help for rest api #2793

Merged
merged 1 commit into from
May 3, 2019

Conversation

waychal
Copy link
Contributor

@waychal waychal commented Apr 24, 2019

Resolves #2764

@waychal waychal requested a review from szoupanos April 24, 2019 09:26
@coveralls
Copy link

coveralls commented Apr 24, 2019

Coverage Status

Coverage increased (+0.003%) to 72.329% when pulling 95e4008 on waychal:restapi_user into 948fa0c on aiidateam:develop.

szoupanos
szoupanos previously approved these changes Apr 25, 2019
@@ -268,7 +269,7 @@ def _get_content(self):
return {}

# otherwise ...
node = self.qbobj.first()[0]
node = self.qbobj.all()[0][1]
Copy link
Contributor

Choose a reason for hiding this comment

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

We should see with Leonid why this is not possible anymore.
If we don't find a solution, a ticket should be opened.

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue describing this #2808 I have fixed in PR #2824 so this change can now be reverted

@szoupanos
Copy link
Contributor

This should be merged after it is tested with the django_jsonb branch where the hybrid properties are removed.

I will need help from @sphuber to rebase the django_jsonb branch since after his last PRs I have conflicts that are not obvious to resolve.

Then we can test this PR with the rebased django_jsonb and merge it to develop.

@sphuber
Copy link
Contributor

sphuber commented May 3, 2019

I have tested this with the django_jsonb branch and confirmed that it fixes the tests. I will merge this in develop and then rebase django_jsonb which will allow the REST API tests to be re-enabled.

Automatically projection user email for nodes relied on hybrid
properties in SqlAlchemy, which have been removed. The alternative is to
explicitly join on the User entity and project separately.
@sphuber sphuber self-requested a review May 3, 2019 08:16
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

thanks @waychal

@sphuber sphuber merged commit dadc5d1 into aiidateam:develop May 3, 2019
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.

REST API should not be based on Hybrid properties
4 participants