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

only show MT-button if user has publish rights for a page #3367

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jonbulz
Copy link
Contributor

@jonbulz jonbulz commented Jan 27, 2025

Short description

Show MT-button for pages only to users with publishing rights

Proposed changes

  • add additional check user_has_publish_rights in PageTranslationForm and return an empty queryset if they don't

Side effects

  • requires changes in some tests, because the AUTHOR role can now only use machine translations for specific pages

Resolved issues

Fixes: #2961


Pull Request Review Guidelines

@jonbulz
Copy link
Contributor Author

jonbulz commented Jan 27, 2025

Hi @MizukiTemma , I'm a bit lost as to why the tests are failing. I suspect it's because the message I am checking in the logs is the wrong message, but I'm not sure what to expect instead. Can you help me with that?

@MizukiTemma
Copy link
Member

MizukiTemma commented Jan 27, 2025

@jonbulz
It's probably because of no permission/role check on python side. The buttons for translation is hidden, but the related functions are not checking the permission before translating by MT provider. In GUI author users do not see the MT option and therefore cannot use it. But in the test we call the functions directly, which means author users are not blocked by "hidden" buttons.

@jonbulz jonbulz force-pushed the fix/machine_translation_user_rights branch from 776711e to 4aad1f7 Compare January 28, 2025 11:49
@jonbulz
Copy link
Contributor Author

jonbulz commented Jan 28, 2025

@MizukiTemma thanks, I see now. Since I don't change any actual permissions, the behavior in tests basically stays the same. I'll have to think about this, I don't think it's very clean. At the very least, I would like to have a test that investigates the expected user experience for authors

@jonbulz jonbulz force-pushed the fix/machine_translation_user_rights branch from 4aad1f7 to fb3cb66 Compare January 28, 2025 13:13
@jonbulz
Copy link
Contributor Author

jonbulz commented Feb 3, 2025

@MizukiTemma I tried implementing a test to check if the button is visible for observers without publish rights. However, it seems that my approach is not working. Are you doing something similar somewhere else in the code?

@MizukiTemma
Copy link
Member

@jonbulz
Hmm, I don't have examples with buttons, but here is an example of test checking whether a certain thins is shown.

@jonbulz
Copy link
Contributor Author

jonbulz commented Feb 3, 2025

@MizukiTemma thanks! This is similar to what I have tried, though. I'm afraid that the part of the page I want to check is rendered later, because it never showed up in the response content. Maybe I should bring it up in the next weekly?

@JoeyStk JoeyStk self-requested a review February 3, 2025 13:23
@JoeyStk JoeyStk self-assigned this Feb 3, 2025
@JoeyStk
Copy link
Contributor

JoeyStk commented Feb 3, 2025

Thank you a lot for tackling this issue 🚀 For the page view it works as expected :)
I have one question: Do we also want to hide the translation bulk action in page tree? (maybe this is a question for @osmers?)

@MizukiTemma
Copy link
Member

@MizukiTemma thanks! This is similar to what I have tried, though. I'm afraid that the part of the page I want to check is rendered later, because it never showed up in the response content. Maybe I should bring it up in the next weekly?

Sure. Alternatively you can pop this discussion in one of our channels. It helps us to share the context before the weekly and someone may answer quicker.

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.

Allow MT only when user has publish_page right
3 participants