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

feat: 🚀 generic task view #3005

Closed
wants to merge 11 commits into from
Closed

Conversation

zcrt
Copy link
Contributor

@zcrt zcrt commented May 29, 2024

Changes

This PR introduces a task view for all organizations simultaneously. To this end:

  • Add new taskviews
  • Enhances scheduler endpoints
  • Updates templates to use organisation information from tasks
  • Extends the main menu on the generic page

What needs to be done is to make sure the new views are only accessible for staff users. I tried this with the commented out django code, but the user object seems to be missing. I left some loggingdebugstatements that need to be removed after that is fixed.

Issue link

Closes #2645

Demo

image

image

image


Code Checklist

  • All the commits in this PR are properly PGP-signed and verified.
  • This PR only contains functionality relevant to the issue; tickets have been created for newly discovered issues.
  • I have written unit tests for the changes or fixes I made.
  • For any non-trivial functionality, I have added integration and/or end-to-end tests.
  • I have performed a self-review of my code and refactored it to the best of my abilities.

Communication

  • I have informed others of any required .env changes files if required and changed the .env-dist accordingly.
  • I have made corresponding changes to the documentation, if necessary.
  • I have included comments in the code to elaborate on what is not self-evident from the code itself, including references to issues and discussions online, or implicit behavior of an interface.

Checklist for code reviewers:

Copy-paste the checklist from the docs/source/templates folder into your comment.


Checklist for QA:

Copy-paste the checklist from the docs/source/templates folder into your comment.

@zcrt zcrt requested a review from a team as a code owner May 29, 2024 12:45
@underdarknl
Copy link
Contributor

nice work!
This needs checks for user-rights, so the user can only see 'all' tasks if he has those rights, Or we should filter the task-lists based on the list of organisations the user has access to if we want to offer this to everyone.

@zcrt
Copy link
Contributor Author

zcrt commented May 30, 2024

Indeed, how would I go about checking for user-rights in OpenKAT?

Copy link
Contributor

@ammar92 ammar92 left a comment

Choose a reason for hiding this comment

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

Nice feature and work overall. I do have a few remarks to improve it even more.

Regarding user rights, you might be able to combine the OrganizationMember and the current user to create a list of accessible schedulers

rocky/rocky/scheduler.py Outdated Show resolved Hide resolved
rocky/rocky/scheduler.py Outdated Show resolved Hide resolved
rocky/rocky/scheduler.py Outdated Show resolved Hide resolved
rocky/rocky/views/tasks.py Outdated Show resolved Hide resolved
rocky/rocky/views/tasks.py Show resolved Hide resolved
rocky/rocky/views/tasks.py Show resolved Hide resolved
@underdarknl
Copy link
Contributor

nice work!
This needs checks for user-rights, so the user can only see 'all' tasks if he has those rights, Or we should filter the task-lists based on the list of organisations the user has access to if we want to offer this to everyone.

Nice feature and work overall. I do have a few remarks to improve it even more.

Regarding user rights, you might be able to combine the OrganizationMember and the current user to create a list of accessible schedulers

That could work on the rocky end, but the scheduler api does not allow for filtering on many organizations I think?

@ammar92
Copy link
Contributor

ammar92 commented Jun 10, 2024

That could work on the rocky end, but the scheduler api does not allow for filtering on many organizations I think?

If the scheduler API doesn't support this yet, you'd still have to retrieve the user's organizations to know which task lists the user can see or retrieve

@jpbruinsslot
Copy link
Contributor

From the scheduler side I'd recommend to retrieve the task for all organizations from the tasks endpoint, this makes sure that filtering is done in db-time instead of python-time. Filtering on multiple scheduler ids and retrieving tasks is possible from this endpoint.

@zcrt
Copy link
Contributor Author

zcrt commented Jun 25, 2024

I updated the PR to retrieve the user accessible schedulers and show all related tasks and stats

Copy link
Contributor

@ammar92 ammar92 left a comment

Choose a reason for hiding this comment

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

Thanks for the rework and updates. Just a few tiny remarks on the scheduler client. Should be ready to be functionally tested (can you take a look at this, @stephanie0x00?)

rocky/rocky/scheduler.py Outdated Show resolved Hide resolved
rocky/rocky/scheduler.py Outdated Show resolved Hide resolved
rocky/rocky/scheduler.py Outdated Show resolved Hide resolved
mula/scheduler/server/server.py Outdated Show resolved Hide resolved
mula/scheduler/server/server.py Outdated Show resolved Hide resolved
rocky/rocky/scheduler.py Outdated Show resolved Hide resolved
def handle_page_action(self, action: str) -> None:
if action == PageActions.RESCHEDULE_TASK.value:
task_id = self.request.POST.get("task_id")
organization_code = self.request.POST.get("organization_code")
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to validate that the user can access this organization. There is some code further down the line that checks if the given organization ID matches the Job's organization ID, but checking for access rights needs to be done here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another route could be to just link to the organization's taskview page action, which because it loads with the organization in the url, will aready do this check for us. That would mean we just need to point the rerun form to that url in the same way the organiaton specific view does it. In that case this whole post handler and handle_page_action ca be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean something like

organization_code in [o.code for o in self.request.user.organizations]

?

@underdarknl
Copy link
Contributor

merged as another PR after fixing the conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Task list view for super users for all organisations
4 participants