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

Only allow staff members to use the SUMM AI bulk action #2104

Merged
merged 1 commit into from Mar 8, 2023
Merged

Only allow staff members to use the SUMM AI bulk action #2104

merged 1 commit into from Mar 8, 2023

Conversation

JoeyStk
Copy link
Contributor

@JoeyStk JoeyStk commented Mar 6, 2023

Short description

This PR restricts the usage of the SUMM AI bulk action to staff members only

Proposed changes

  • Add a check in the template if the user is staff member or not
  • Add an entry to the changelog to let others teams know that we have improved this (please correct me, if we should remove the entry in case it's not meant to be seen by everyone)

Side effects

I think none. I tested it for a few roles and didn't notice any side effects.
One thing we should keep in mind. As for now the SUMM AI function was disabled globally. So once this PR is merged, we could turn it on again

Resolved issues

Fixes: #2065

Pull Request Review Guidelines

@JoeyStk JoeyStk requested a review from a team as a code owner March 6, 2023 09:57
@codeclimate
Copy link

codeclimate bot commented Mar 6, 2023 •
edited

Code Climate has analyzed commit 60f51ee and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 75.3% (0.0% change).

View more on Code Climate.

@JoeyStk JoeyStk force-pushed the enhancement/allow_only_staff_members_to_use_summ_bulk_action branch from 578ebcd to 95e8523 Compare March 6, 2023 10:01
timobrembeck
timobrembeck reviewed Mar 6, 2023
View reviewed changes
Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

I'm not sure if this is required anymore - if we don't implement #1727, then we need a bulk action for non-staff users...

@JoeyStk
Copy link
Contributor Author

JoeyStk commented Mar 6, 2023

After a quick chat with @dkehne I was reassured that this PR - although it's not very urgent - is still wanted, because it's the cleaner solution to our current workaround. At the moment there is still an edge case in which the municipalities have a short time window in which they theoretically could use the bulk action, although they shouldn't be able to. Sure, this isn't very likely but still at least a theoretic possibility

timobrembeck
timobrembeck requested changes Mar 6, 2023
View reviewed changes
Copy link
Member

@timobrembeck timobrembeck left a comment

Choose a reason for hiding this comment

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

Ok, but in the long term region managers should be able to use the bulk action, right? We only do the initial translations ourselves, but if they want to update the existing content, they should be able to do this on their own?

Apart from that: Please also implement the check in the bulk action view itself, otherwise this just makes the option invisible, but does not prevent users to use it.

@JoeyStk
Copy link
Contributor Author

JoeyStk commented Mar 6, 2023

Yes, that's how I understood it as well

Okay. I will do so. Thank you!

@osmers
Copy link

osmers commented Mar 7, 2023

Couldn't we just use the same principle for updating translations that we use for all other DeepL languages? If SUMM is active in the region and allowed for translation via the management section, then Deutsch (leicht) is an option that can be chosen on the page form?

@timobrembeck timobrembeck mentioned this pull request Mar 7, 2023
Closed
@timobrembeck
Copy link
Member

Couldn't we just use the same principle for updating translations that we use for all other DeepL languages? If SUMM is active in the region and allowed for translation via the management section, then Deutsch (leicht) is an option that can be chosen on the page form?

@osmers Good idea, I opened #2110 for this.

@osmers
Copy link

osmers commented Mar 7, 2023 •
edited

@timoludwig perfect - I also just asked @charludo to post the latest design and ideas for language selection into the issue #1875 so that that is up to date

@JoeyStk JoeyStk force-pushed the enhancement/allow_only_staff_members_to_use_summ_bulk_action branch 3 times, most recently from 1cb6969 to 8fed7b7 Compare March 8, 2023 13:36
charludo
charludo approved these changes Mar 8, 2023
View reviewed changes
Copy link
Contributor

@charludo charludo left a comment •
edited

Choose a reason for hiding this comment

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

Very nice, works as expected! Apart from the one small comment below, looks good to me! :)

timobrembeck
timobrembeck requested changes Mar 8, 2023
View reviewed changes
integreat_cms/cms/views/bulk_action_views.py Outdated Show resolved Hide resolved
tests/summ_ai_api/summ_ai_test.py Outdated Show resolved Hide resolved
@JoeyStk JoeyStk force-pushed the enhancement/allow_only_staff_members_to_use_summ_bulk_action branch 2 times, most recently from e84d49d to 059de5e Compare March 8, 2023 14:40
@JoeyStk JoeyStk force-pushed the enhancement/allow_only_staff_members_to_use_summ_bulk_action branch from 059de5e to 60f51ee Compare March 8, 2023 14:50
@JoeyStk JoeyStk merged commit 3a27a17 into develop Mar 8, 2023
@JoeyStk JoeyStk deleted the enhancement/allow_only_staff_members_to_use_summ_bulk_action branch March 8, 2023 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charludo charludo charludo approved these changes

@timobrembeck timobrembeck timobrembeck approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Only allow staff members to use SUMM.AI bulk action
4 participants
@JoeyStk @osmers @timobrembeck @charludo