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 #25562 - Product rendering issue without recurring logic #7854

Merged
merged 1 commit into from Nov 28, 2018

Conversation

sjha4
Copy link
Member

@sjha4 sjha4 commented Nov 27, 2018

This happens when there is a sync plan without a recurring logic. This should not happen in general but we need to handle the rendering issue regardless.

To reproduce:

  1. Create a sync plan.
  2. Log into Katello db and delete the recurring logic of the sync plan from the db.
  3. Go to Products or sync plan index page.

@theforeman-bot
Copy link

Issues: #25562

@theforeman-bot
Copy link

There were the following issues with the commit message:

  • length of the first commit message line for a45fda9 exceeds 65 characters
  • commit message for a45fda9 is not wrapped at 72nd column

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

@beav
Copy link
Contributor

beav commented Nov 27, 2018

@sjha4 thanks for the quick PR for this one

@@ -118,7 +118,7 @@ def sync_time
end

def next_sync_date
return nil unless (self.enabled || !self.foreman_tasks_recurring_logic.tasks.nil?)
return nil unless (self.enabled || !self.foreman_tasks_recurring_logic.try(:tasks).nil?)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you flip this to self.foreman_tasks_recurring_logic.try(:tasks).present?? this would avoid a double negative

@sjha4 sjha4 force-pushed the 25562 branch 2 times, most recently from df3dce6 to bfcadcb Compare November 27, 2018 21:21
@sjha4
Copy link
Member Author

sjha4 commented Nov 27, 2018

@beav: Rubocop wouldn't allow me to catch all exceptions so using safe navigation to avoid nil values instead.

@beav
Copy link
Contributor

beav commented Nov 27, 2018

@sjha4 looks good, thanks! I don't think this code is overly defensive, given that its trying to display something

edit: the change looks good but if the missing field is from some other bug/issue, I would say we can address that and then close this pr without merging

@sjha4
Copy link
Member Author

sjha4 commented Nov 28, 2018

@beav: The upgrade task for 3.9 failed for the user which resulted in the inconsistent db state. The upgrade failure is fixed by #7849. This should be good to merge.

@beav
Copy link
Contributor

beav commented Nov 28, 2018

@sjha4 great thanks!

@beav beav merged commit d1005da into Katello:master Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants