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

fix(tv show tabs): add sorting to vuex store to fix season ordering #480

Merged
merged 2 commits into from
Feb 10, 2021

Conversation

camc314
Copy link
Contributor

@camc314 camc314 commented Jan 2, 2021

Potentially fixes #479

After adding the array of episodes of each season. Sort by parentIndexNumber

@sonarcloud
Copy link

sonarcloud bot commented Jan 2, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codecov-io
Copy link

codecov-io commented Jan 2, 2021

Codecov Report

Merging #480 (4946183) into master (b3cb5d8) will increase coverage by 0.49%.
The diff coverage is 37.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #480      +/-   ##
==========================================
+ Coverage   11.76%   12.25%   +0.49%     
==========================================
  Files         133      133              
  Lines        3451     3451              
  Branches      525      525              
==========================================
+ Hits          406      423      +17     
+ Misses       3024     3005      -19     
- Partials       21       23       +2     
Impacted Files Coverage Δ
components/Item/SeasonTabs.vue 0.00% <0.00%> (ø)
store/tvShows.ts 37.77% <60.00%> (+37.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3cb5d8...4946183. Read the comment docs.

@heyhippari
Copy link
Contributor

heyhippari commented Jan 2, 2021

Analyzing this a bit, this seems more like a logic issue in how the data is structured and assigned than a sorting issue.

We're assigning episodes to an array, by combining the existing contents of the array with the new one. However, nothing guarantees that we are going to receive the seasons in the proper order or that said seasons would contain anything, or even return at all.

I think the proper fix for this would be to rework the structure to be like so:

const TvShowsState = {
  '494947c860b2d2abaa645bffb9fde158': {
    'seasons': {
      0: {
        'information': BaseItemDto // The season's info
        'episodes': BaseItemDto[] // An array of all the episodes
      }
    }
  }
}

This way, we can set the info by key instead of by index, which allows us to go from 0 to 4 back to 2, as items come in. It also sets us up for adding more info to this store at a later date (Like storing the series' info directly in the store).

Edit: though another, maybe more valid way to do this would be to split episodes and seasons into two stores, then use getters to fetch what we need and collate data in the components instead.
This has the advantage of keeping the stores as flat as possible and allowing us to limit the amount of duplicate data, thereby making the subsequent updates we'll need to do through the websocket messages easier.

Copy link
Contributor

@heyhippari heyhippari left a comment

Choose a reason for hiding this comment

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

Should've probably marked my previous comment as a request for change, as the current way of doing things won't solve the core issue we're having.

@camc314
Copy link
Contributor Author

camc314 commented Feb 5, 2021

Finally updated to use an object. Should be working now

state: {
  'seriesId': {
    seasons: [] // Array of seasons (S01, S02...)
    seasonEpisodes: {
      'season-id': [S01E01, S01E02],
      'season-id': [S02E01, S02E02]
    }
  }
}

Copy link
Member

@ThibaultNocchi ThibaultNocchi left a comment

Choose a reason for hiding this comment

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

You can also simplify your Vue.set calls by just editing a sub-prop such as there:

Vue.set(state.CustomPrefs, key, defaultCustomPrefs[key]);

store/tvShows.ts Outdated Show resolved Hide resolved
store/tvShows.ts Outdated Show resolved Hide resolved
@camc314
Copy link
Contributor Author

camc314 commented Feb 9, 2021

You can also simplify your Vue.set calls by just editing a sub-prop such as there:

Vue.set(state.CustomPrefs, key, defaultCustomPrefs[key]);

The only issue is that state[itemId] may not be defined yet. By setting the whole state, we create the property on state if it is not already defined

@ThibaultNocchi
Copy link
Member

Yeah that seems fair then

@ThibaultNocchi
Copy link
Member

@camc314 Also did you see the small comments I left? :)

@camc314
Copy link
Contributor Author

camc314 commented Feb 9, 2021

@camc314 Also did you see the small comments I left? :)

Yep, I'll address them in the next few hours 👍

@sonarcloud
Copy link

sonarcloud bot commented Feb 9, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 6 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@camc314 camc314 merged commit f6ebc23 into master Feb 10, 2021
@camc314 camc314 deleted the fix/tvTabs branch February 10, 2021 12:56
@camc314 camc314 mentioned this pull request Feb 10, 2021
16 tasks
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.

Episodes are in the wrong season
5 participants