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

Consider job permissions in UI #346

Merged
merged 5 commits into from
Feb 18, 2024
Merged

Consider job permissions in UI #346

merged 5 commits into from
Feb 18, 2024

Conversation

annavik
Copy link
Member

@annavik annavik commented Feb 10, 2024

Fixes #328

Also added some info in UI about what is going on:

Screenshot 2024-02-10 at 09 53 12

Copy link

netlify bot commented Feb 10, 2024

Deploy Preview for ami-web ready!

Name Link
🔨 Latest commit c0eacca
🔍 Latest deploy log https://app.netlify.com/sites/ami-web/deploys/65d23d3ee108b8000814bdad
😎 Deploy Preview https://deploy-preview-346--ami-web.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 72
Accessibility: 95
Best Practices: 92
SEO: 92
PWA: 70
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Feb 10, 2024

Deploy Preview for ami-storybook ready!

Name Link
🔨 Latest commit c0eacca
🔍 Latest deploy log https://app.netlify.com/sites/ami-storybook/deploys/65d23d3e1be6e200081a61f6
😎 Deploy Preview https://deploy-preview-346--ami-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -14,6 +15,10 @@ export class CaptureDetails extends Capture {
}
}

get canUpdate(): boolean {
Copy link
Member Author

Choose a reason for hiding this comment

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

@mihow I felt a bit weird checking for update permission on capture, we should perhaps check for create permission on jobs? Although I rather avoid fetch a list of jobs, just to get this info.

Let me know if this solutions is fine or if you have thoughts :)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is for "Process now" in session detail view

@annavik annavik requested a review from mihow February 10, 2024 10:07
@mihow
Copy link
Collaborator

mihow commented Feb 10, 2024 via email

@annavik
Copy link
Member Author

annavik commented Feb 11, 2024

Ah yes create job makes more sense! We are not modifying the capture. Thank you!

On Sat, Feb 10, 2024 at 2:07 AM Anna Viklund @.> wrote: @annavik https://github.com/annavik requested your review on: #346 <#346> Consider job permissions in UI. — Reply to this email directly, view it on GitHub <#346 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABGTX25FS3KF6PI4GJMWVDYS5BGNAVCNFSM6AAAAABDCSK7G6VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJRG43DINJTHE3TOMQ . You are receiving this because your review was requested.Message ID: @.>

I agree, but thing is I get the "create job" permission when fetching a list of jobs and it doesn't really make sense to do so in this this context... Can we make this permissions available in a different way? We have the same situation for "starred", I would prefer to check update permission for this collection, but I need to fetch extra stuff that is not really needed in order to do so.

@mihow
Copy link
Collaborator

mihow commented Feb 11, 2024 via email

@annavik
Copy link
Member Author

annavik commented Feb 18, 2024

Ah okay I will look into making those available. Is the update permission for the active project currently available? We can use that permission we can use for multiple features. Also I might have to prefix all URLs with the current project ID. To help with query speed and permission checking for private projects. So just a heads up about that. On Sun, Feb 11, 2024 at 4:29 AM Anna Viklund @.> wrote:

Ah yes create job makes more sense! We are not modifying the capture. Thank you! … <#m_-6256968134272742576_> On Sat, Feb 10, 2024 at 2:07 AM Anna Viklund @.
> wrote: @annavik https://github.com/annavik https://github.com/annavik https://github.com/annavik requested your review on: #346 <#346> <#346 <#346>> Consider job permissions in UI. — Reply to this email directly, view it on GitHub <#346 (comment) <#346 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABGTX25FS3KF6PI4GJMWVDYS5BGNAVCNFSM6AAAAABDCSK7G6VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJRG43DINJTHE3TOMQ https://github.com/notifications/unsubscribe-auth/AABGTX25FS3KF6PI4GJMWVDYS5BGNAVCNFSM6AAAAABDCSK7G6VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJRG43DINJTHE3TOMQ . You are receiving this because your review was requested.Message ID: @.
> I agree, but thing is I get the "create job" permission when fetching a list of jobs and it doesn't really make sense to do so in this this context... Can we make this permissions available in a different way? We have the same situation for "starred", I would prefer to check update permission for this collection, but I need to fetch extra stuff that is not really needed in order to do so. — Reply to this email directly, view it on GitHub <#346 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABGTX5RECE7Q2W3BA5JX6DYTC2TLAVCNFSM6AAAAABDCSK7G6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZXGY4DKOBRHE . You are receiving this because you were mentioned.Message ID: @.
**>

Yes, that we can absolutely do! Project details we fetch from a wrapper component and all the child components can access this data without us having to do a new fetch. PR is updated to check project permissions for "star" and "process now" :)

In future, for more detailed permissions, I wonder if it would make sense to put them on the user object?

@mihow
Copy link
Collaborator

mihow commented Feb 18, 2024

@annavik thank you so much! I added a ticket about moving some permissions to the user object: #354

@mihow mihow merged commit 0bfb5cb into main Feb 18, 2024
6 checks passed
@mihow mihow deleted the fix/consider-job-permissions branch February 18, 2024 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add permissions boundary for jobs
2 participants