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

🐛 FIX: Initialising a Node with a User #4977

Merged
merged 4 commits into from
Jun 16, 2021
Merged

🐛 FIX: Initialising a Node with a User #4977

merged 4 commits into from
Jun 16, 2021

Conversation

chrisjsewell
Copy link
Member

@chrisjsewell chrisjsewell commented Jun 7, 2021

So it appears that no one has ever tried to specifically set a user for a node (e.g. Data(user=another_user)) because, as you see from the diff, it would then try to call .backend_entity on the backend entity and fail.
Disappointing that mypy didn't pick this up in the type checking (perhaps there is a stricter setting that I haven't added)

cc @ltalirz as this came up with respect to the REST API (i.e. if we want to create a node for a specific user)

So it appears that no one has ever tried to specifically set a user for a node because, as you see from the diff, it would then try to call `.backend_entity` on the backend entity and fail.
Disappointing that mypy didn't pick this up in the type checking (perhaps there is a stricter setting that I haven't added)
@chrisjsewell chrisjsewell requested review from sphuber and ltalirz June 7, 2021 13:07
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 @chrisjsewell . Straight forward enough, but since this wasn't caught by tests and is so basic, would be good to add a simple test for this.

@chrisjsewell
Copy link
Member Author

but since this wasn't caught by tests and is so basic, would be good to add a simple test for this.

yeh yeh I knew you were going to say this 😛 Just wanted to quickly add the code change via github first, so I didn't forget

@chrisjsewell chrisjsewell requested a review from sphuber June 16, 2021 06:26
@chrisjsewell
Copy link
Member Author

@sphuber I converted all of tests/orm/node/test_node.py to pytest and added some extra tests

@codecov
Copy link

codecov bot commented Jun 16, 2021

Codecov Report

Merging #4977 (e1f8e7a) into develop (1bc9dbe) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4977      +/-   ##
===========================================
+ Coverage    80.11%   80.12%   +0.01%     
===========================================
  Files          515      515              
  Lines        36673    36676       +3     
===========================================
+ Hits         29378    29382       +4     
+ Misses        7295     7294       -1     
Flag Coverage Δ
django 74.59% <100.00%> (+0.01%) ⬆️
sqlalchemy 73.51% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiida/manage/tests/pytest_fixtures.py 93.34% <100.00%> (+0.28%) ⬆️
aiida/orm/nodes/node.py 96.29% <100.00%> (ø)
aiida/transports/plugins/local.py 81.66% <0.00%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bc9dbe...e1f8e7a. Read the comment docs.

@chrisjsewell chrisjsewell changed the title 🐛 FIX: Specifying the user for a node 🐛 FIX: Initialising a Node with a User Jun 16, 2021
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 @chrisjsewell

@sphuber sphuber merged commit f4f543c into develop Jun 16, 2021
@sphuber sphuber deleted the fix/node-user branch June 16, 2021 19:14
sphuber pushed a commit that referenced this pull request Aug 8, 2021
The `Node` constructor allows for a specific user to be set other
than the current default, except this wasn't actually working. The bug
has been fixed and a test added.

Cherry-pick: f4f543c
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.

2 participants