requests: split mine and shared with me #426
Conversation
zzacharo
force-pushed
the
requests-shared-mine
branch
from
4a7878a to
6c66e47
Compare
* adds `shared_with_me` param to filter requests * adds dashboard dropdown to filter requests * adds topic generator on `can_read` request permission * relies on `can_read` request permission to search user requests
zzacharo
force-pushed
the
requests-shared-mine
branch
from
6c66e47 to
63d4963
Compare
zzacharo
changed the title
zzacharo
force-pushed
the
requests-shared-mine
branch
from
9f5c1cd to
503d554
Compare
| @@ -90,7 +90,7 @@ RequestStatusFilterComponent.propTypes = { | |||
| }; | |||
|
|
|||
| RequestStatusFilterComponent.defaultProps = { | |||
| keepFiltersOnUpdate: false, | |||
| keepFiltersOnUpdate: true, | |||
There was a problem hiding this comment.
We need to keep the filters so that they can be combined with the new shared_with_me one
| # My requests | ||
| search = search.filter(my_requests_q) | ||
|
|
||
| return search |
There was a problem hiding this comment.
Are ParamInterpreters easily unit testable?
The my_requests_q seems counter-intuitive to me, it reads like "the request is created by me and I'm also the receiver", is it correct?
There was a problem hiding this comment.
yes my_requests reflects all requests a user created or receives. I will check if there is any test for that but still I am missing some tests, which most probably will go to rdm-records where the record as a topic concept is defined :)
There was a problem hiding this comment.
LGTU! Peer reviewed with @sakshamarora1
Maybe some tests would be nice as Pablo suggested.
Seems that tests are failing due to isort and permission denied error.
| @@ -288,16 +288,13 @@ def search_user_requests( | |||
| identity, | |||
| params, | |||
| search_preference, | |||
| permission_action=None, | |||
| permission_action="read", | |||
There was a problem hiding this comment.
Why is this change needed?
There was a problem hiding this comment.
To be able to apply search filtering based on user's permissions.
| @@ -16,13 +16,16 @@ import { GridResponsiveSidebarColumn } from "react-invenio-forms"; | |||
| import { SearchBar } from "react-searchkit"; | |||
| import { Button, Container, Grid } from "semantic-ui-react"; | |||
|
|
|||
| import { SharedOrMineFilter } from "@js/invenio_app_rdm/components/SharedOrMineFilter"; | |||
There was a problem hiding this comment.
wrong import, you shouldn't import from app_rdm since app_rdm depends on requests not the other way around
There was a problem hiding this comment.
still wip this version :) Sorry I will move the PRs back in progress until I cleanup the new code added
zzacharo
force-pushed
the
requests-shared-mine
branch
from
fa49350 to
c89349d
Compare
zzacharo
force-pushed
the
requests-shared-mine
branch
from
c89349d to
56cc28c
Compare
| obj.includes("shared_with_me") | ||
| ); | ||
| if (sharedWithMe) { | ||
| // eslint-disable-next-line react/no-did-mount-set-state |
There was a problem hiding this comment.
sorry for duplication, the same comment applies now here
inveniosoftware/invenio-app-rdm#2982 (comment)
can we avoid setting state in the mount? It would be better to move to the constructor, no?
There was a problem hiding this comment.
fixed it! It was copied from another place and I fixed both of them
zzacharo
force-pushed
the
requests-shared-mine
branch
from
6093bf8 to
371bd96
Compare
closes inveniosoftware/invenio-rdm-records#1948