Use internal addon representation and themeData #3320
Conversation
Codecov Report
@@ Coverage Diff @@
## master mozilla/addons-frontend#3320 +/- ##
==========================================
- Coverage 95.57% 95.55% -0.03%
==========================================
Files 166 166
Lines 3163 3169 +6
Branches 632 634 +2
==========================================
+ Hits 3023 3028 +5
- Misses 120 121 +1
Partials 20 20
Continue to review full report at Codecov.
|
| type LoadOtherAddonsByAuthorsParams = {| | ||
| slug: string, | ||
| addons: Array<AddonType>, | ||
| addons: Array<ExternalAddonType>, |
src/core/reducers/addons.js
Outdated
| version: apiAddon.theme_data.version, | ||
| }), | ||
| // We should be using this property. | ||
| themeData: removeUndefinedProps({ ...apiAddon.theme_data }), |
There was a problem hiding this comment.
This can just be themeData: apiAddon.theme_data because removeUndefinedProps() was only necessary to make the spread not override existing property values with undefined.
src/core/reducers/addons.js
Outdated
| version: apiAddon.theme_data.version, | ||
| }), | ||
| // We should be using this property. | ||
| themeData: removeUndefinedProps({ ...apiAddon.theme_data }), |
There was a problem hiding this comment.
I think we need to introduce a mapper and call this like themeData: createInternalThemeData(apiAddon) because of several reasons:
- Even though it adds verbosity, I think it's helpful to see all the fields assigned one by one
- When a new field is added to the API, we won't introduce the new field by surprise
- We already have to fix the mapping for one of the fields (
description) because of a bug - We may need to re-map others later on too.
You can move all of this stuff into a new createInternalThemeData() function.
| expect(state.featured.results) | ||
| .toEqual([{ slug: 'foo' }, { slug: 'food' }]); | ||
| .toEqual([ | ||
| createInternalAddon({ slug: 'foo' }), |
There was a problem hiding this comment.
these should both be { ...fakeAddon, slug: '...' }
src/core/reducers/addons.js
Outdated
| updateURL: apiAddon.theme_data.updateURL, | ||
| version: apiAddon.theme_data.version, | ||
| }), | ||
| // We should be using this property. |
There was a problem hiding this comment.
I think it would be better to put a comment up above where theme_data gets merged into addon saying "use addon.themeData[themeProp] instead of addon[themeProp]"
| loadAddons(createFetchAddonResult(theme).entities)); | ||
|
|
||
| const expectedTheme = { | ||
| const expectedTheme = createInternalAddon({ |
There was a problem hiding this comment.
This should not use createInternalAddon(). This test is a bit weird but the intention of it was to manually recreate the theme addon to test that the mapper is doing what we expect it to. If you use createInternalAddon() then this test is pointless since it relies on that for the mapping. Instead of manually creating the entire theme addon, the test was using some spreads for the mapped properties that exactly match the original. Maybe it needs some comments to clarify this?
| const { results } = getNextState(); | ||
| expect(results).toEqual([{ slug: 'foo' }, { slug: 'food' }]); | ||
| expect(results).toEqual([ | ||
| createInternalAddon({ slug: 'foo' }), |
There was a problem hiding this comment.
I won't mark each one but a couple more of these need to change to { ...fakeAddon, slug: '...' }
| windows: undefined, | ||
| }, | ||
| isRestartRequired: false, | ||
| themeData, |
There was a problem hiding this comment.
ugh, this is an old test and it's not very useful. It may be worth just using createInternalAddon({ ...fakeDiscoAddon, ... }) for the assertions here.
There was a problem hiding this comment.
Looks like we are not using the internal addon representation here, or I don't understand how it works right now :/
|
@kumar303 updated the code, thanks for your comments! ready for a second look. |
3 participants
Fix mozilla/addons#10854
Fix mozilla/addons-frontend#3319
This PR ensures we use an internal addon representation and exposes a
themeDataproperty to this internal representation. It helped fixing an issue with collections not displaying a preview (because thecollectionsreducer uses internal addon representation).(Note: I am aware of mozilla/addons#10819 but did not want to open a big PR)
Before:
After: