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

Added user:group to the file footer. #872

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

rickygzz
Copy link
Contributor

@rickygzz rickygzz commented Dec 9, 2024

Hello,

Here with my two cents. This is my first commit.

Adding user:group to the file footer

image

image

@jelly
Copy link
Member

jelly commented Dec 9, 2024

Thanks for the pull request, I've just re-read the issue and @garrett wrote that it should be between the modified string and permissions. This makes sense as it is the same as our list view.

isInline
component="pre"
>
{file.user}:{file.group}
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be a little more complicated and simplify showing only the user when the user and group are equal. We do this already in FileOwnership in the files-card-body.tsx

Copy link
Member

Choose a reason for hiding this comment

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

I agree with this. (Thanks for mentioning it!)

@@ -8,7 +8,8 @@ import { Popover } from "@patternfly/react-core/dist/esm/components/Popover";
import { Text } from "@patternfly/react-core/dist/esm/components/Text";
import { Tooltip } from "@patternfly/react-core/dist/esm/components/Tooltip";

import cockpit from "cockpit";
import cockpit, { file } from "cockpit";
Copy link
Member

Choose a reason for hiding this comment

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

this file import should not be needed (or even resolve)

Copy link
Member

@garrett garrett left a comment

Choose a reason for hiding this comment

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

Looking good.

However, a few issues that @jelly already pointed out (and that I agree with):

  • Placement: it should be right before permissions (and after modified time)
  • Collapsing consistency: Shrink user:group down to just user when user == group, like we do in the list.

Once those are implemented, I'm happy with this when @jelly is happy with this.

@rickygzz
Copy link
Contributor Author

Thank you for your kind review!

  1. Rearranged user:group to be in between the modified string and permissions
  2. Added shrinking logic when user==group
  3. Removed unneeded file import
  4. Rebased code

Please @jelly let me know your comments

Copy link
Member

@jelly jelly left a comment

Choose a reason for hiding this comment

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

Are you interested in adding tests, otherwise I can help or write them.

>
<DescriptionListGroup key="user">
<DescriptionListTerm>
User
Copy link
Member

@jelly jelly Dec 13, 2024

Choose a reason for hiding this comment

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

This needs to be translated

Suggested change
User
{_("User")}

</DescriptionListGroup>
<DescriptionListGroup key="group">
<DescriptionListTerm>
Group
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be translated

Suggested change
Group
{_("Group")}

@rickygzz
Copy link
Contributor Author

rickygzz commented Dec 19, 2024

@jelly

Sorry for the delay. I updated the translatable texts and rebased my commits.

I spent couple hours trying to figure out the tests in this project but I guess it will take me longer to understand them.

I guess the test code should look something like

        # Test ownership
        m.execute("chown admin:root /home/admin/newdir")
        b.wait_in_text(".files-footer-info .pf-m-link", "admin:root")
        m.execute("chown admin:admin /home/admin/newdir")
        b.wait_in_text(".files-footer-info .pf-m-link", "admin")
        # Also check popover
        b.click(".files-footer-info .pf-m-link")
        b.wait_in_text(".pf-v5-c-popover dl div:nth-child(1) > dd", "admin")
        b.wait_in_text(".pf-v5-c-popover dl div:nth-child(2) > dd", "admin")
        b.click(".pf-v5-c-popover__close > button")
        b.wait_not_present(".pf-v5-c-popover")

And I am guessing I will have to tweak the .pf-m-link "path" since now we have two: one for the ownership link and one for the permissions link. But to play around first I need to figure out how to run the tests properly, but I am experiencing issues running the make check.

$ make check
tools/node-modules make_package_lock_json
bots/image-customize --no-network --fresh \
         \
        --upload cockpit-files-node-1.tar.xz:/var/tmp/ --build cockpit-files-1.tar.xz \
        --script /home/rocky/repos/cockpit-files/test/vm.install fedora-41
Traceback (most recent call last):
  File "/home/rocky/repos/cockpit-files/bots/image-customize", line 26, in <module>
    from machine import testvm
  File "/home/rocky/repos/cockpit-files/bots/machine/testvm.py", line 28, in <module>
    from lib.directories import get_images_data_dir
  File "/home/rocky/repos/cockpit-files/bots/lib/directories.py", line 25, in <module>
    def xdg_home(subdir: str, envvar: str, *components: str, override: str | None = None) -> str:
TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'
make: *** [Makefile:187: /home/rocky/repos/cockpit-files/test/images/fedora-41] Error 1

I guess it would be faster if you write them down for me, what do you think?

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Including a link showing the owner/group of the file/directory would
make our test fail as the selector becomes ambiguous.
@jelly
Copy link
Member

jelly commented Jan 9, 2025

Taken the liberty to squash your commits, apply some eslint fixes and add tests. This should be good to go now.

martinpitt
martinpitt previously approved these changes Jan 10, 2025
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks! Some suggestions to make the tests stricter, but LGTM!

test/check-application Outdated Show resolved Hide resolved
test/check-application Outdated Show resolved Hide resolved
In the folder icon view you where not able to see the owner/group of the
selected file or folder since the removal of the sidebar.

Closes: cockpit-project#846
@martinpitt martinpitt dismissed garrett’s stale review January 10, 2025 10:23

Garrett's feedback got addressed

@martinpitt martinpitt merged commit 4fc09be into cockpit-project:main Jan 10, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants