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 #38056 - Refresh content count action fails when count is set to {} #11244

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

sjha4
Copy link
Member

@sjha4 sjha4 commented Dec 2, 2024

What are the changes introduced in this pull request?

Add check for smart proxy content_count being {}

Considerations taken when implementing this change?

We were handling the nil case but not empty? case, i.e {}
Added empty checks for content_counts in 2 places.
One for content_counts where we reinitialize it to { content_view_versions: {} } and for repositories where we need to handle content_counts[:content_view_versions][repo.content_view_version_id.to_s].empty?

What are the testing steps for this pull request?

In the console,

sp = SmartProxy.find("id")
sp.content_counts = {}
sp.save!

Try refresh counts action against any env/cv or global and the content count task will fail.

If you have some content_counts populated, pick any cv_version_id,
You can also try to test the empty hash case for cv version id keys.
Example:
sp.content_counts["content_view_versions"]["8"] = {}

Run refresh action for that cv_version from UI and see that it passes for this PR but fails without the PR.

Copy link

@vsedmik vsedmik left a comment

Choose a reason for hiding this comment

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

Tested downstream in my snap 80 setup (curl <this> | git apply) with 2 CVs inside 2 LCEs and works well!

Tried both - reset the global and CVV-specific counts via rake console and update them back via CV-specific and LCE-specific kebab (the UI kebab).

In all cases I ended up with task successfully completed and counts correctly updated for the appropriate CV/LCE.

Copy link
Member

@chris1984 chris1984 left a comment

Choose a reason for hiding this comment

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

Code looks fine, Vlad tested it

@chris1984 chris1984 self-assigned this Dec 5, 2024
@sjha4 sjha4 merged commit fa899c0 into Katello:master Dec 5, 2024
26 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.

3 participants