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.

Sign up for GitHub

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

Jump to bottom

Permissions : Refactoring permissions usage #985

Merged
merged 1 commit into from
May 27, 2020
Merged

Permissions : Refactoring permissions usage #985

merged 1 commit into from
May 27, 2020

Conversation

zannkukai
Copy link
Contributor

@zannkukai zannkukai commented May 11, 2020 •
edited by rerowep
Loading

Remove permissions from SearchSerializer to improve API performance.

How to test?

  • Try to list some resources using the search API :
    • https://localhost:5000/api/patrons
    • https://localhost:5000/api/documents
    • https://localhost:5000/api/items
    • https://localhost:5000/api/patrons/3
    • https://localhost:5000/api/items/3
    • https://localhost:5000/api/documents/3

Code review check list

  • Commit message template compliance.
  • Commit message without typos.
  • File names.
  • Functions names.
  • Functions docstrings.
  • Unnecessary commited files?
  • Extracted translations?

@zannkukai zannkukai force-pushed the zan-#can_add_permissions branch from 8997f1c to a852fbb Compare May 11, 2020 15:56
@zannkukai zannkukai requested a review from BadrAly May 11, 2020 16:12
@zannkukai zannkukai self-assigned this May 11, 2020
@zannkukai zannkukai force-pushed the zan-#can_add_permissions branch from a852fbb to 1df6269 Compare May 12, 2020 17:56
@zannkukai zannkukai requested a review from rerowep May 12, 2020 18:47
@zannkukai zannkukai force-pushed the zan-#can_add_permissions branch 2 times, most recently from 896e93b to e1a3737 Compare May 13, 2020 07:58
rerowep
rerowep reviewed May 13, 2020
View reviewed changes
Copy link
Contributor

@rerowep rerowep left a comment

Choose a reason for hiding this comment

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

Can we get the coverage to green?

@zannkukai zannkukai force-pushed the zan-#can_add_permissions branch 3 times, most recently from bb649d0 to f862b5f Compare May 13, 2020 15:22
@zannkukai zannkukai requested a review from rerowep May 13, 2020 17:23
@zannkukai zannkukai force-pushed the zan-#can_add_permissions branch from f862b5f to ce55333 Compare May 14, 2020 07:48
rerowep
rerowep requested changes May 14, 2020
View reviewed changes
Copy link
Contributor

@rerowep rerowep left a comment •
edited
Loading

Choose a reason for hiding this comment

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

If I try to access following site https://localhost:5000/global/search/documents?q=&page=1&size=10 with deployment setup, I get following error:

127.0.0.1 - - [14/May/2020 16:21:00] "GET /api/documents/?q=&page=1&size=10&view=global HTTP/1.1" 500 -
Traceback (most recent call last):
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/flask/app.py", line 2334, in __call__
    return self.wsgi_app(environ, start_response)
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/werkzeug/middleware/dispatcher.py", line 66, in __call__
    return app(environ, start_response)
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/flask/app.py", line 2334, in __call__
    return self.wsgi_app(environ, start_response)
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/werkzeug/middleware/shared_data.py", line 220, in __call__
    return self.app(environ, start_response)
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/flask/app.py", line 2320, in wsgi_app
    response = self.handle_exception(e)
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/flask/app.py", line 1766, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/flask/_compat.py", line 36, in reraise
    raise value
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/flask/app.py", line 2317, in wsgi_app
    response = self.full_dispatch_request()
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/flask/app.py", line 1840, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/flask/app.py", line 1743, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/flask/_compat.py", line 36, in reraise
    raise value
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/flask/app.py", line 1838, in full_dispatch_request
    rv = self.dispatch_request()
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/flask_debugtoolbar/__init__.py", line 125, in dispatch_request
    return view_func(**req.view_args)
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/flask/views.py", line 88, in view
    return self.dispatch_request(*args, **kwargs)
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/invenio_rest/views.py", line 240, in dispatch_request
    *args, **kwargs
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/flask/views.py", line 158, in dispatch_request
    return meth(*args, **kwargs)
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/invenio_records_rest/views.py", line 429, in need_record_permission_decorator
    return f(self, record=record, *args, **kwargs)
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/invenio_records_rest/views.py", line 511, in inner
    return f(self, pagination=req, *args, **kwargs)
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/invenio_records_rest/views.py", line 651, in get
    item_links_factory=self.item_links_factory,
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/invenio_rest/views.py", line 221, in make_response
    return serializer(*args, **kwargs)
  File "/Users/wep/devel/virtualenvs/rero-ils-5uhYvcmh-py3.6/lib/python3.6/site-packages/invenio_records_rest/serializers/response.py", line 56, in view
    item_links_factory=item_links_factory),
  File "/Users/wep/devel/rero/rero-ils/rero_ils/modules/serializers.py", line 103, in serialize_search
    results, pid_fetcher), **self._format_args())
  File "/Users/wep/devel/rero/rero-ils/rero_ils/modules/documents/serializers.py", line 77, in post_process_serialize_search
    metadata.get('pid')).is_available(viewcode)
  File "/Users/wep/devel/rero/rero-ils/rero_ils/modules/documents/api.py", line 82, in is_available
    if holding.available:
AttributeError: 'NoneType' object has no attribute 'available'

I have restarted the setup -d and I think the error comes from an interrupted ebooks harvesting.
Shall we make our code more robust against this kind of problem?

@rerowep rerowep self-requested a review May 14, 2020 15:04
@zannkukai
Copy link
Contributor Author

If I try to access following site https://localhost:5000/global/search/documents?q=&page=1&size=10 with deployment setup, I get following error:

I have restarted the setup -d and I think the error comes from an interrupted ebooks harvesting.
Shall we make our code more robust against this kind of problem?

Do you user my local branch or the PR files to tests ?
With my local branch, I had an CTRL-X error on def available() function. This error was on my local branch not on rero-ils PR.
I ccorrected this bug, rerun setup this morning and all seems working fine.

@zannkukai zannkukai force-pushed the zan-#can_add_permissions branch 3 times, most recently from 701b63b to 28bbbee Compare May 19, 2020 07:09
iGormilhit
iGormilhit approved these changes May 19, 2020
View reviewed changes
Copy link

@iGormilhit iGormilhit left a comment

Choose a reason for hiding this comment

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

Commit message approved. Also tested with rero/rero-ils-ui#268

@zannkukai zannkukai added this to the v0.9.0 milestone May 19, 2020
@zannkukai zannkukai force-pushed the zan-#can_add_permissions branch from 28bbbee to af43f76 Compare May 19, 2020 11:56
lauren-d
lauren-d approved these changes May 26, 2020
View reviewed changes
Copy link
Contributor

@lauren-d lauren-d left a comment •
edited
Loading

Choose a reason for hiding this comment

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

tests OK. seems that works as expected

@zannkukai
permission: add 'create' to permission API
88a9b37 
* Updates the permission API adding a section about the 'create'
permission.
* Updates `SearchSerializer` to remove permissions from hit result.

Co-Authored-by: Renaud Michotte <renaud.michotte@gmail.com>
@zannkukai zannkukai force-pushed the zan-#can_add_permissions branch from af43f76 to 88a9b37 Compare May 27, 2020 06:19
@zannkukai zannkukai merged commit 89693cd into rero:dev May 27, 2020
@zannkukai zannkukai deleted the zan-#can_add_permissions branch May 27, 2020 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lauren-d lauren-d lauren-d approved these changes

@rerowep rerowep rerowep approved these changes

@BadrAly BadrAly BadrAly approved these changes

@benerken benerken benerken approved these changes

@iGormilhit iGormilhit iGormilhit approved these changes

Assignees

@zannkukai zannkukai

Labels
None yet
Projects
None yet
Milestone
v0.9.0
Development

Successfully merging this pull request may close these issues.
6 participants
@zannkukai @iGormilhit @benerken @BadrAly @rerowep @lauren-d