Refactor: Unify XML Generation to Fix Language Scoping Bug #416
Conversation
|
This project was built from scratch this year. There is no legacy here. |
| @@ -0,0 +1 @@ | |||
| video content | |||
There was a problem hiding this comment.
Can we add small video file instead?
| @@ -0,0 +1,82 @@ | |||
| require "rails_helper" | |||
|
|
|||
| RSpec.describe XmlGenerator::AllProviders do | |||
There was a problem hiding this comment.
Let's move in to all_providers_spec.rb since this class in already tested there
There was a problem hiding this comment.
the idea is to remove those classes.
There was a problem hiding this comment.
But here in this PR you leave both classes in place
| self | ||
| end | ||
|
|
||
| def with_topic(title:, published_at:, tags: [], documents: []) |
There was a problem hiding this comment.
We can make this method accept list of topics.
Then in your tests you will be able to do with_topics instead of calling with_topic multiple times
There was a problem hiding this comment.
It can have both! It's a helper to build some specific scenario so I think that having it like this is good. And then we could extend it to create batches for other tests later.
There was a problem hiding this comment.
I see you are calling with_topic multiple times already
| # Patch the single buggy method in the legacy implementation for this test. | ||
| # This allows us to use the actual legacy classes but with the critical | ||
| # language-scoping logic fixed, ensuring a valid comparison. | ||
| # allow_any_instance_of(XmlGenerator::SingleProvider).to receive(:topic_scope) do |instance, provider| |
There was a problem hiding this comment.
Do we need this commented code here?
There was a problem hiding this comment.
If this all works we can just join all in a single spec file for the new class
There was a problem hiding this comment.
Can we remove this commented code then?
And why we can't join all right now?
| def provider_xml(provider, topics) | ||
| Ox::Element.new("content_provider").tap do |provider_element| | ||
| provider_element[:name] = provider.name | ||
| build_year_nodes(provider_element, topics) |
There was a problem hiding this comment.
Like this decomposition into several methods!
|
@dmitrytrager I added this new class to replace the all/single XML classes and added some specs to make sure it followed the same contract. As this change is to solve a bug we can't exactly know for sure unless we deploy and someone update the Client to test with the new XML I decided on just adding this class and after we confirm everything works we could clean up. |
|
Ok, great, don't forget follow-up PR please |
Context
The legacy
XmlGenerator::AllProvidersclass incorrectly included topics from all languages in its output, regardless of the specified language context. This was caused by atopic_scopemethod that did not apply a language filter. The implementation also resulted in an N+1 query pattern during generation.Changes
XmlGenerator::AllProvidersandXmlGenerator::SingleProviderclasses have been replaced in theLanguageContentProcessorby a new, unifiedLanguageTopicsXmlGeneratorservice. The legacy classes are retained for now but are no longer in active use by the processor.language_id, which corrects the data-scoping bug and resolves the N+1 performance issue.LanguageTopicsXmlGeneratorinitializer accepts an optionalprovider:argument, allowing it to handle both all-provider and single-provider generation, unifying the previous two classes' responsibilities.LanguageContentProcessorwas updated to call the new service for all XML generation tasks.Testing
spec/support/xml_test_data_builder.rb) was added to facilitate setting up complex data scenarios.XmlGenerator::AllProviders(spec/services/xml_generator/all_providers_spec.rb) was augmented with a test case that explicitly reproduces and documents the language-scoping bug.LanguageTopicsXmlGenerator(spec/services/language_topics_xml_generator_spec.rb) was added. It verifies the new service's behavior and includes a direct comparison against a patched version of the legacy generator to ensure output compatibility.FileUploadJobspec was refactored to be less repetitive and to correctly test the integration with the new service.LanguageContentProcessorspec was updated to reflect the correct number of jobs being enqueued.