-
Notifications
You must be signed in to change notification settings - Fork 303
Fixes #38945 - make RH flatpak helper org scoped and minor fixes #11594
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
Fixes #38945 - make RH flatpak helper org scoped and minor fixes #11594
Conversation
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.
Hey there - I've reviewed your changes - here's some feedback:
- In the
has_redhat_flatpak_remotequery you droppedunscoped, which changes behavior when an organization is not present or when a default scope exists; consider preservingunscopedwhile still scoping byorganization_idwhen@organizationis set. - The hardcoded
setName('Red Hat remote')string in the helper banner isn't localized; consider wrapping it in__()(or reusing an existing translated label) so it follows the rest of the UI's i18n pattern. - The Red Hat flatpak URL (
https://flatpaks.redhat.io/rhel) is now duplicated between frontend behavior and backend detection logic; pulling this into a shared constant or at least a single definition would reduce the risk of future drift.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `has_redhat_flatpak_remote` query you dropped `unscoped`, which changes behavior when an organization is not present or when a default scope exists; consider preserving `unscoped` while still scoping by `organization_id` when `@organization` is set.
- The hardcoded `setName('Red Hat remote')` string in the helper banner isn't localized; consider wrapping it in `__()` (or reusing an existing translated label) so it follows the rest of the UI's i18n pattern.
- The Red Hat flatpak URL (`https://flatpaks.redhat.io/rhel`) is now duplicated between frontend behavior and backend detection logic; pulling this into a shared constant or at least a single definition would reduce the risk of future drift.
## Individual Comments
### Comment 1
<location> `webpack/scenes/FlatpakRemotes/CreateEdit/FlatpakRemoteform.js:130-132` </location>
<code_context>
actionLinks={
<React.Fragment>
- <AlertActionLink onClick={() => setUrl('https://flatpaks.redhat.io/rhel')}>
+ <AlertActionLink onClick={() => {
+ setUrl('https://flatpaks.redhat.io/rhel');
+ setName('Red Hat remote');
+ }}
+ >
</code_context>
<issue_to_address>
**issue:** New label string for the Red Hat remote name should likely go through the i18n helper.
This introduces a new user-visible string that skips the translation helper used elsewhere in this file (e.g. `__('Add Red Hat flatpak remote')`). Please wrap it with the i18n helper for consistency and localization support, e.g. `setName(__('Red Hat remote'))`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <Modal | ||
| ouiaId="create-flatpak-modal" | ||
| title={__('Create Flatpak Remote')} | ||
| title={__('Create flatpak remote')} |
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.
I know sentence case is usually the goal here, but I wonder if this one should be different because Flatpak is a proper name.
| title={__('Create flatpak remote')} | |
| title={__('Create Flatpak remote')} |
@MariSvirik thoughts?
| <Modal | ||
| ouiaId="edit-flatpak-modal" | ||
| title={__('Edit Flatpak Remote')} | ||
| title={__('Edit flatpak remote')} |
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.
same comment as above
0f496bb to
6ea3e23
Compare
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.
Functionality-wise, this is working nicely..Was able to test the changed org and remote name behavior..Ack from me.. Merge after @MariSvirik acks 👍🏼
|
If "Flatpak" is functionally equivalent to a proper noun like "Ansible" or "Kubernetes," the correct choice is to capitalize it. Create Flatpak remote |
6ea3e23 to
581f19c
Compare
|
Upper-cased both, thank you @MariSvirik and @jeremylenz ! |
sjha4
left a comment
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.
APJ 👍🏼 Thanks @vsedmik !
sambible
left a comment
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.
Nice fix! Need to find myself some sort of issue like this for my first upstream contribution :)
What are the changes introduced in this pull request?
This PR just slightly modifies the "Create flatpak remote" modal:
flatpaks.redhat.ioin URL) is missing in the current organization (used to be global).I did not touch the Create/Update buttons, they seem fine to me.
This PR has been AI-assisted.
Considerations taken when implementing this change?
None.
What are the testing steps for this pull request?
Summary by Sourcery
Scope Red Hat flatpak helper behavior to the current organization and align the flatpak remote creation UI copy.
New Features:
Bug Fixes:
Enhancements: