-
Notifications
You must be signed in to change notification settings - Fork 2
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
resources actions #19
Conversation
|
Screen.Recording.2023-02-23.at.14.47.52.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok So far so good, minor updates request
public/locales/cs/translation.json
Outdated
}, | ||
"ResourcesDeletedDetailContainer": { | ||
"Resource": "Zdroj", | ||
"Hard-delete": "Navždy odstranit", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok I would choose something like Trvale odstranit
, it sounds better to me (just a subjective note)
public/locales/cs/translation.json
Outdated
"Failed to hard-delete this resource": "Nepodařilo se navždy odstranit tento zdroj", | ||
"Failed to retrieve resource": "Obnova zdroje se nezdařila", | ||
"Resource retrieved successfuly": "Obnova zdroje proběhla úspěšně", | ||
"Resource was successfully hard-deleted": "Zdroj byl úspěšně navždy odstranit", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok use trvale
instead of navzdy
public/locales/cs/translation.json
Outdated
"Hard-delete": "Navždy odstranit", | ||
"Retrieve": "Obnovit zdroj", | ||
"Can't fetch resource details": "Nepodařilo se načíst zdroj", | ||
"Failed to hard-delete this resource": "Nepodařilo se navždy odstranit tento zdroj", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok use trvale
instead of navzdy
public/locales/cs/translation.json
Outdated
"Failed to retrieve resource": "Obnova zdroje se nezdařila", | ||
"Resource retrieved successfuly": "Obnova zdroje proběhla úspěšně", | ||
"Resource was successfully hard-deleted": "Zdroj byl úspěšně navždy odstranit", | ||
"Do you really want to hard-delete this resource": "Opravdu si přejete navždy odstranit zdroj?", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok use trvale
instead of navzdy
src/modules/auth/index.js
Outdated
path: '/auth/resources-deleted', | ||
exact: true, | ||
name: 'Deleted resources', | ||
component: ResourcesDeletedListContainer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok You should update your branch from main
and then add a correct resource here
src/modules/auth/index.js
Outdated
path: '/auth/resources-deleted/:resource_id', | ||
exact: true, | ||
name: 'Deleted resource detail', | ||
component: DeletedResourceDetailContainer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok As per my comment above, add a correct resource
const [editMode, setEditMode] = useState(false); | ||
const { resource_id } = props.match.params; | ||
|
||
const editResource = "authz:superuser"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok check the resources by the document on sharepoint and ask Robin, what resources should be implemented for any action we are providing to user regarding resources removal
{editMode ? | ||
<> | ||
<ButtonGroup> | ||
<Button color="primary" type="submit" >{t("Save")}</Button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok I bet this button should be ButtonWithAuthz
with some particular resource
<i className="cil-code pr-2"></i> | ||
JSON | ||
<div className='actions-right'> | ||
<Button color="danger" type="button" onClick={() => terminateResourceForm(resource._id)}>{t("ResourcesDetailContainer|Delete resource")}</Button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok Again, this should be ButtonWithAuthz
I suppose
@@ -3,7 +3,7 @@ import { useTranslation } from 'react-i18next'; | |||
import { useSelector } from 'react-redux'; | |||
|
|||
import { DataTable, ButtonWithAuthz } from 'asab-webui'; | |||
import { Container } from 'reactstrap'; | |||
import { Container, Button } from 'reactstrap'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok You dont use Button
in the ResourcesListContainer, so it does not have to be loaded
const [count, setCount] = useState(0); | ||
const [page, setPage] = useState(1); | ||
const [loading, setLoading] = useState(true); | ||
const [show, setShow] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok I think this is not needed. I think its ok if the content loader appears only for a second. You are adding jsut too much logic to this considering we dont use it elsewhere. But if you want to do it this way, then we should update it in other parts of app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my subjective point of view, I think its enough to handle it just with loading
state
|
||
useEffect(()=>{ | ||
setShow(false); | ||
if (resources.length === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok as my comment above
@@ -19,6 +19,7 @@ function ResourcesListContainer(props) { | |||
const ref = useRef(null); | |||
const { t } = useTranslation(); | |||
|
|||
const deleteResource = "seacat:resource:edit"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok do you really need it? take a look a line below, just use the resource on line 23, its the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok Just small renaming comment, otherwise it looks good from my point of view
src/modules/auth/index.js
Outdated
exact: true, | ||
name: 'Deleted resources', | ||
component: ResourcesDeletedListContainer, | ||
resource: 'seacat:resource:access' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok Clarify with @byewokko if this screen shouldnt be accessed only by superuser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok Then also other actions regarding hard deleting resource(s) will be only for superusers I guess (and not seacat:resource:edit
)
src/modules/auth/index.js
Outdated
exact: true, | ||
name: 'Deleted resource detail', | ||
component: DeletedResourceDetailContainer, | ||
resource: 'seacat:resource:access' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok As per my comment above
const ref = useRef(null); | ||
const { t } = useTranslation(); | ||
|
||
const retrieveButtonResource = "seacat:resource:edit"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok Rename to retrieveResource
- I think here its just fine
src/modules/auth/index.js
Outdated
app.Router.addRoute({ | ||
path: '/auth/resources/:resource_id', | ||
exact: true, | ||
name: 'Resource detail', | ||
component: ResourcesDetailContainer, | ||
resource: "seacat:resource:access" | ||
resource: "authz:superuser" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok Oh, this resource should have stayed as it was, please put it back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pe5h4 oh, right, sorry, my mistake
src/modules/auth/index.js
Outdated
exact: true, | ||
name: 'Deleted resources', | ||
component: ResourcesDeletedListContainer, | ||
resource: 'seacat:resource:access' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrKavulok This resource should be superuser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
related to