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

Update Buggy Code in Advance of Python 3 Update #3457

Merged
merged 25 commits into from Oct 10, 2019

Conversation

ilan-gold
Copy link
Member

@ilan-gold ilan-gold commented Oct 3, 2019

There are a few things/bugs here related to #2619 that should be fixed before updating to Python3.

  1. We don't save() the status during the test_tasks.py test which means we are comparing None and int when _check_galaxy_history_state runs, which is problematic in Python3 and not exactly what we want these tests to do anyway in Python2.
  2. If a use logs out in another webpage and then tries to access a dataset in another page, I noticed we were returning a 500 error, so I wrapped that in a try-catch.
  3. We don't pass context into ToolSerializer, which we should since it raises an exception that is caught (Pass in Context for ToolSerializer #3455).

@ilan-gold ilan-gold changed the title Update Code in Advance of Python 3 Update Update Buggy Code in Advance of Python 3 Update Oct 3, 2019
@ilan-gold ilan-gold requested a review from jkmarx October 3, 2019 13:41
if not request.user.has_perm('core.read_meta_dataset', data_set):
try:
perms = request.user.has_perm('core.read_meta_dataset', data_set)
except User.DoesNotExist:
Copy link
Member

Choose a reason for hiding this comment

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

If the user is not logged in, they are the anon user. What's throwing the exception exactly?

Copy link
Member Author

@ilan-gold ilan-gold Oct 9, 2019

Choose a reason for hiding this comment

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

I believe this has to do with the way in which the logged-in user is held in browser vs. what is happening on the backend - the browser still has everything it needs to have the permission/User but the User has been logged out already in the backend so when the cached user in the browser comes through, Django uses the load of it from the cache but can then not find it once inside the application logic.

Copy link
Member Author

@ilan-gold ilan-gold Oct 9, 2019

Choose a reason for hiding this comment

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

so this is indeed what appears to be happening in the Authentication middleware (i imagine little changed from 1.8 to 1.9 regarding this - check out the get_user function: https://github.com/django/django/blob/1.9/django/contrib/auth/middleware.py)

@codecov
Copy link

codecov bot commented Oct 10, 2019

Codecov Report

Merging #3457 into develop will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3457      +/-   ##
===========================================
+ Coverage    87.42%   87.45%   +0.02%     
===========================================
  Files           96       96              
  Lines        15159    14799     -360     
===========================================
- Hits         13253    12942     -311     
+ Misses        1906     1857      -49
Impacted Files Coverage Δ
refinery/analysis_manager/test_tasks.py 100% <100%> (ø) ⬆️
refinery/tool_manager/views.py 92.3% <100%> (ø) ⬆️
refinery/core/models.py 82.47% <0%> (-0.99%) ⬇️

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 9419113...c8ee5bc. Read the comment docs.

@ilan-gold ilan-gold merged commit bf46b82 into develop Oct 10, 2019
@ilan-gold ilan-gold deleted the ilan-gold/fix_problematic_python2_code branch October 10, 2019 13:54
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.

None yet

2 participants