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

Jkmarx/limit events api response #2944

Merged
merged 6 commits into from Aug 13, 2018
Merged

Conversation

jkmarx
Copy link
Member

@jkmarx jkmarx commented Aug 10, 2018

  • Limit the event api response to save time
  • Fix bug in data set serializer (null response for is_owner and public)
  • Update unit tests

Ref #2909

@jkmarx jkmarx self-assigned this Aug 10, 2018
@jkmarx jkmarx added this to the Release 1.6.6 milestone Aug 10, 2018
@@ -34,13 +34,17 @@ def get_is_owner(self, data_set):
logger.error("Request is missing a user: %s", e)
return False
return user_request == owner
else:
Copy link
Member Author

@jkmarx jkmarx Aug 10, 2018

Choose a reason for hiding this comment

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

left out else, fixes null response from API

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just return in the first try? And you could do the same thing in the except:

     def get_is_owner(self, data_set):
         try:
             return data_set.is_owner
         except:
             owner = data_set.get_owner()
             try:
                 return owner == self.context.get('request').user
             except AttributeError as e:
                logger.error("Request is missing a user: %s", e)
                return False

self.assertEqual(
json.loads(get_response.content),
get_response.data,
Copy link
Member Author

Choose a reason for hiding this comment

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

reorder response (api now handles it)

@@ -34,13 +34,17 @@ def get_is_owner(self, data_set):
logger.error("Request is missing a user: %s", e)
return False
return user_request == owner
else:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just return in the first try? And you could do the same thing in the except:

     def get_is_owner(self, data_set):
         try:
             return data_set.is_owner
         except:
             owner = data_set.get_owner()
             try:
                 return owner == self.context.get('request').user
             except AttributeError as e:
                logger.error("Request is missing a user: %s", e)
                return False


def get_public(self, data_set):
try:
data_set.public
except:
is_public = data_set.is_public()
return is_public
else:
return data_set.public
Copy link
Member

Choose a reason for hiding this comment

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

ditto: You don't want to do too much in a try, but just returning isn't going cause a different exception

).data,
'group': None,
'user': UserSerializer(self.user).data,
'type': 'UPDATE',
Copy link
Member

Choose a reason for hiding this comment

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

Originally the type was one of the CRUD verbs, so I'm surprised this isn't CREATE?

Copy link
Member

Choose a reason for hiding this comment

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

... but it looks like it's just being copied from below, so not a new issue...

@codecov
Copy link

codecov bot commented Aug 13, 2018

Codecov Report

Merging #2944 into develop will decrease coverage by 0.21%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2944      +/-   ##
===========================================
- Coverage    59.39%   59.17%   -0.22%     
===========================================
  Files          434      434              
  Lines        27345    27109     -236     
  Branches      1279     1273       -6     
===========================================
- Hits         16242    16043     -199     
+ Misses       11103    11066      -37
Impacted Files Coverage Δ
refinery/core/urls.py 100% <ø> (ø) ⬆️
refinery/core/views.py 40.61% <100%> (-6.85%) ⬇️
refinery/core/serializers.py 86.07% <100%> (+1.46%) ⬆️
refinery/core/test_views.py 100% <100%> (ø) ⬆️
...e/js/data-set-import/controllers/isa-tab-import.js 40.9% <0%> (-7.72%) ⬇️
refinery/tool_manager/tests.py 99.82% <0%> (-0.01%) ⬇️
refinery/data_set_manager/views.py 60.2% <0%> (+0.06%) ⬆️
...ce/js/dashboard/controllers/data-sets-card-ctrl.js 69.82% <0%> (+0.8%) ⬆️
... and 1 more

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 6063d16...2b65c5b. Read the comment docs.

@jkmarx jkmarx merged commit dbf1a76 into develop Aug 13, 2018
@jkmarx jkmarx deleted the jkmarx/limit-events-api-response branch August 13, 2018 15:48
@jkmarx jkmarx added this to Closed in JM Tasks 1.6.6 Aug 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants