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

Fixes #35730 - Show include artifact checkboxes for rpm and module stream #10356

Merged
merged 1 commit into from
Nov 21, 2022

Conversation

sjha4
Copy link
Member

@sjha4 sjha4 commented Nov 16, 2022

Move Include/Exclude all RPMs with no errata and Include/Exclude all module streams with no errata checkboxes on RPM and Module stream filter details view

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

  1. Create a RPM filter and a module stream filter.
  2. The Include/Exclude all RPMs with no errata and Include/Exclude all module streams with no errata should be visible and editable even without any filter rules added.

@theforeman-bot
Copy link

Issues: #35730

@sjha4
Copy link
Member Author

sjha4 commented Nov 17, 2022

@MariSvirik : So this what we have with this PR. Looking for feedback.

Screenshot from 2022-11-17 10-45-39

@jeremylenz
Copy link
Member

What's been bothering me about "Include all RPMs with no errata" is that it's unclear in a couple ways:

  1. Does it mean "Include all RPMs and don't include any errata?" Or "Include all RPMs which do not contain any errata?" I think it's the latter. Is there any better wording we could use?
  2. In the context of the explicit list below: It actually means "In addition to the list explicitly defined below, include [all RPMs with no errata]." So in effect, it affects what RPMs are on the Include list, even though those are not explicitly displayed.

How about "In addition to the list below, include all RPMs which do not contain any errata"?

@sjha4
Copy link
Member Author

sjha4 commented Nov 17, 2022

  1. Include all RPMs which do not contain any errata?

That's the right one. The usecase for this is mostly together with errata filters. Errata filters add errata + packages for that errata to the new version. To include/exclude all packages that would be filtered out because they don't have any errata, we have this checkbox on package filters.

We could add a help tooltip with a detailed description for what that field does..

@jeremylenz
Copy link
Member

We could add a help tooltip with a detailed description for what that field does..

+1 to that!

@ianballou
Copy link
Member

How about "In addition to the list below, include all RPMs which do not contain any errata"?

Other way around, "...include all RPMs that 'are not listed in' / 'do not belong to' any errata."

@sjha4
Copy link
Member Author

sjha4 commented Nov 17, 2022

Any thoughts on whether we should rename the field?

@jeremylenz
Copy link
Member

Other way around, "...include all RPMs that 'are not listed in' / 'do not belong to' any errata."

That makes so much more sense!!

@ianballou
Copy link
Member

Any thoughts on whether we should rename the field?

Which field do you mean @sjha4 ?

@ianballou
Copy link
Member

This PR is making me realize we have no way to delete filters from their details pages...

@ianballou
Copy link
Member

ianballou commented Nov 17, 2022

I'm seeing an issue where sometimes the filter's "exclude all" or "include all" switch isn't in the correct position. A couple times I've flipped it on or off, published a version, looked back at it, and it's showing the opposite.

If I click to another tab and then navigate back to the filter, it corrects itself. So it seems to be just a UI thing.

@MariSvirik
Copy link

@sjha4 empty state looks good. Just change an icon to "plus".
https://www.patternfly.org/v4/components/empty-state/design-guidelines

Screenshot 2022-11-18 at 15 13 40

For a better microcopy, I'll ping Akshay.

@MariSvirik
Copy link

MariSvirik commented Nov 18, 2022

@sjha4

  1. remove the period at the end of the label - it's viewed as a filter setting, not a sentence.
  2. switcher and label text should be center aligned https://www.patternfly.org/v4/components/switch

Screenshot 2022-11-18 at 16 00 10

@sjha4
Copy link
Member Author

sjha4 commented Nov 18, 2022

Any thoughts on whether we should rename the field?

Which field do you mean @sjha4 ?

I meant the Include all RPMs without errata label for the switch. It still makes sense to me but I am open to changing it if that's the consensus.

@sjha4
Copy link
Member Author

sjha4 commented Nov 18, 2022

Screenshot from 2022-11-18 10-23-40

@sjha4
Copy link
Member Author

sjha4 commented Nov 18, 2022

@sjha4 empty state looks good. Just change an icon to "plus". https://www.patternfly.org/v4/components/empty-state/design-guidelines

Screenshot 2022-11-18 at 15 13 40

Updated.
@jeremylenz : Change for this makes sense to me but changes pages outside CV UI. Let me know if it makes sense.

@ianballou
Copy link
Member

I meant the Include all RPMs without errata label for the switch. It still makes sense to me but I am open to changing it if that's the consensus.

What do we think about a slight rewording to indicate that errata own RPMs and not the other way around?

Include/Exclude all RPMs not associated to errata

@MariSvirik
Copy link

@ianballou would this be for the label?
"Switch on (switcher) Include/Exclude all RPMs not associated to errata"

@ianballou
Copy link
Member

@ianballou would this be for the label? "Switch on (switcher) Include/Exclude all RPMs not associated to errata"

@MariSvirik yeah, the text that goes next to the switch. And for include filters it would start with "Include", and for exclude filters it would start with "Exclude" (just to be clear that I wasn't asking to actually put "Include/Exclude" with the slash).

@sjha4
Copy link
Member Author

sjha4 commented Nov 18, 2022

Added kebab next to dropdown with delete action.
Screenshot from 2022-11-18 12-08-19

@AkshayGadhaveRH
Copy link

@MariSvirik slightly rewording @ianballou's suggestion: Include all RPMs not associated with any errata; Does this sound better?

@sjha4
Copy link
Member Author

sjha4 commented Nov 21, 2022

@MariSvirik slightly rewording @ianballou's suggestion: Include all RPMs not associated with any errata; Does this sound better?

@AkshayGadhaveRH .. That sounds good. I will update the text.

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

This is working well for me!

@sjha4
Copy link
Member Author

sjha4 commented Nov 21, 2022

Merging this..Thanks all for the feedback and reviews!

@sjha4 sjha4 merged commit e601dae into Katello:master Nov 21, 2022
chris1984 pushed a commit to chris1984/katello that referenced this pull request Nov 28, 2022
chris1984 pushed a commit that referenced this pull request Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants