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 #25543 - Breadcrumb switcher doesn't work for filters #6407

Merged
merged 1 commit into from Aug 12, 2019

Conversation

MariaAga
Copy link
Member

When editing a filter /filters/<id>/edit, the breadcrumb-switcher showed blank lines in the results box.
Also changed breadcrumbs in filters from "filters > filter" to "Roles > role Filters
Edit filter Filter" since filters are a part of roles and aren't a standalone.

@theforeman-bot
Copy link
Member

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

2 similar comments
@theforeman-bot
Copy link
Member

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

@theforeman-bot
Copy link
Member

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

@theforeman-bot
Copy link
Member

Issues: #25543

@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • 15183be must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@MariaAga MariaAga changed the title Fixes #25543 Breadcrumb switcher doesn't work for filters Fixes #25543 - Breadcrumb switcher doesn't work for filters Jan 13, 2019
@tbrisker
Copy link
Member

ok to test

Copy link
Member

@amirfefer amirfefer left a comment

Choose a reason for hiding this comment

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

Thanks @MariaAga !
I left few inline comments :)

app/models/filter.rb Outdated Show resolved Hide resolved
app/views/filters/edit.html.erb Outdated Show resolved Hide resolved
app/views/filters/edit.html.erb Outdated Show resolved Hide resolved
Copy link
Member

@amirfefer amirfefer left a comment

Choose a reason for hiding this comment

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

Thanks @MariaAga !
Test failures are related

app/models/filter.rb Outdated Show resolved Hide resolved
app/views/filters/_form.html.erb Outdated Show resolved Hide resolved
app/models/filter.rb Outdated Show resolved Hide resolved
@coveralls
Copy link

Coverage Status

Coverage remained the same at 69.565% when pulling ea656e1 on MariaAga:25543 into 98974ec on theforeman:develop.

@coveralls
Copy link

coveralls commented Apr 2, 2019

Coverage Status

Coverage remained the same at 72.666% when pulling 57d8953 on MariaAga:25543 into 0b5fef8 on theforeman:develop.

@mmoll
Copy link
Contributor

mmoll commented Apr 5, 2019

@MariaAga please fix the rubocop violation

Copy link
Member

@ekohl ekohl 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 still not sure if we should be using translated values here. In my experience having a simple model which is nil or an actual value is better. Then when you need to present it to the user, you actually ensure there's a good value. A helper for this (like display_resource_type) can make sense. Converting from a presentation value back to an actual value is a recipe for problems that are hard to trace back.

app/views/filters/edit.html.erb Outdated Show resolved Hide resolved
@MariaAga MariaAga force-pushed the 25543 branch 2 times, most recently from 2b50952 to dd6440a Compare April 14, 2019 14:19
@MariaAga
Copy link
Member Author

@ekohl can you take a look? It is now only translated when the value is presented

@sharvit
Copy link
Contributor

sharvit commented May 12, 2019

@ekohl can you have a look please?

@MariaAga
Copy link
Member Author

Test failures are unrelated.

@tbrisker I addressed all of your comments, could you please take another look?

@ekohl I fixed the translations.

Copy link
Member

@ekohl ekohl 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 still not sure about magic strings, just that my computer science professor would fail me if I tried to use them. Generally they're very fragile because you can't really trace the data flow. Adding translations into the mix just screams for bugs. I don't know the code base well enough to say anything useful about whether it would break but only translating it in the presentation (views) is usually the correct way.

app/views/filters/_form.html.erb Outdated Show resolved Hide resolved
app/models/filter.rb Outdated Show resolved Hide resolved
@MariaAga
Copy link
Member Author

@ekohl, instead of all the string handling I added a field resource_type_label to be passed to the breadcrumbs and view. Thanks for the feedback!

ekohl
ekohl previously approved these changes Jul 1, 2019
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

This looks like the correct approach Small questions but no objections to merging.

app/views/filters/edit.html.erb Show resolved Hide resolved
app/views/filters/index.html.erb Outdated Show resolved Hide resolved
@xprazak2
Copy link
Contributor

xprazak2 commented Aug 2, 2019

What is the status here?

@ekohl
Copy link
Member

ekohl commented Aug 8, 2019

@xprazak2 I think this is ready for merge, but would prefer someone else to do a final review and merge it.

@xprazak2
Copy link
Contributor

xprazak2 commented Aug 9, 2019

Works well, I just left one inline comment. After that, I think we can merge.

@xprazak2
Copy link
Contributor

@tbrisker, @ares, any additional comments?

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

Looks like my comments were addressed. Did not test again.

@xprazak2 xprazak2 merged commit b061724 into theforeman:develop Aug 12, 2019
@xprazak2
Copy link
Contributor

Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
10 participants