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

Sle 15 ga registration popup #402

Merged
merged 6 commits into from Oct 12, 2018
Merged

Conversation

schubi2
Copy link
Member

@schubi2 schubi2 commented Oct 12, 2018

No description provided.

@coveralls
Copy link

coveralls commented Oct 12, 2018

Coverage Status

Coverage increased (+0.03%) to 71.532% when pulling b71f57c on SLE-15-GA-registration-popup into 3beb5c9 on SLE-15-GA.

"Without these media only a minimum system is available\n" \
"in this installation.") %
{ media_name: "SLE-15-Packages", download_url: "https://download.suse.com" }
if media_name && !media_name.empty? &&
Copy link
Member

Choose a reason for hiding this comment

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

The nil check is actually not needed, ProductFeatures.GetStringFeature returns empty string if it is not found, see here. Just remove the media_name && download_url checks and add a comment describing that so it is clear.

_("A full system can be installed using the\n" \
"%{media_name} media from %{download_url}.") %
{ media_name: media_name, download_url: download_url }
elsif media_name && !media_name.empty?
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking if handling of the other cases is needed for the future. It feels a bit like over engineering...

I think the additional medium will always have some name (even if a stupid one like DVD2), you need to be able to somehow tell the customers to use an additional medium and that's hard without a name. 😁

Maybe we could simply require defining both and drop these extra conditions. On the other hand this extra code is pretty small and straightforward so I'll leave the decision up to you. 😃

Copy link
Member Author

Choose a reason for hiding this comment

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

I even do not have a testcase for that corner cases :-) --> removed.

Copy link
Member

@lslezak lslezak left a comment

Choose a reason for hiding this comment

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

LGTM

@schubi2 schubi2 merged commit fbba394 into SLE-15-GA Oct 12, 2018
@schubi2 schubi2 deleted the SLE-15-GA-registration-popup branch October 12, 2018 12:24
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.

None yet

3 participants