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.

Sign up for GitHub

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

cleanup app-link for tests #4526

Merged
merged 9 commits into from
Jul 5, 2018
Merged

cleanup app-link for tests #4526

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
21caa76
cleanup app-link for tests
OmgImAlexis Jul 2, 2018
2886218
remove isAnonymised
OmgImAlexis Jul 3, 2018
ab8e534
moved template property to top
OmgImAlexis Jul 3, 2018
ac325f1
remove redundant component name
OmgImAlexis Jul 3, 2018
d806089
remove MEDUSA from component
OmgImAlexis Jul 3, 2018
e313e43
add review code
OmgImAlexis Jul 4, 2018
76d140a
Fix component registration
sharkykh Jul 4, 2018
2de7d08
`indexerName` computed prop was missing some code
sharkykh Jul 4, 2018
e13d1af
Fix null computedHref when href is loaded after creation
sharkykh Jul 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 17 additions & 16 deletions themes-default/slim/views/vue-components/app-link.mako
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@
</component>
</script>
<script>
Vue.component('app-link', {
Vue.component({
name: 'app-link',
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is redundant, please remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for future usage.

Copy link
Contributor

@sharkykh sharkykh Jul 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Okay, then remove the one above it, which overrides the first argument to Vue.component.

template: '#app-link-template',
store,
props: {
to: [String, Object],
Expand All @@ -30,6 +31,17 @@ Vue.component('app-link', {
config() {
return this.$store.state.config;
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that for future usage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is for future usage.

indexerName() {
const { indexerId, config } = this;
if (!indexerId) {
return '';
}
return Object.keys(config.indexers).filter(indexer => {
if (config.indexers[indexer].id === parseInt(indexerId, 10)) {
return config.indexers[indexer].name;
}
})[0];
Copy link
Contributor

@sharkykh sharkykh Jul 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

  1. The keys of config.indexers don't contain the indexer names,
    it's config.indexers.config.indexers. see below [1].
    It's because of the MEDUSA.config structure. When we stop using it we should be able to fix that...
  2. This can use find instead of filter, and be much simpler.
    This array iteration is so confusing. We don't return the name, we return the key (which is the identifier).
    name=TVDBv2 | identifier=tvdb
const { indexers } = config.indexers.config; // This is weird, I know...
// Returns `undefined` if not found.
return Object.keys(indexers).find(indexer => indexers[indexer].id === parseInt(indexerId, 10));
[1] 
{ store.state
  "config": {
    "indexers": {
      "config": {
        "indexers": {
          "tvdb": {
            "xemOrigin": "tvdb",
            "apiParams": {
              "useZip": true,
              "language": "en"
            },
            "showUrl": "https://www.thetvdb.com/dereferrer/series/",
            "id": 1,
            "icon": "thetvdb16.png",
            "name": "TVDBv2",
            "scene_loc": "https://cdn.pymedusa.com/scene_exceptions/scene_exceptions_tvdb.json",
            "enabled": true,
            "baseUrl": "https://api.thetvdb.com/",
            "mappedTo": "tvdb_id",
            "identifier": "tvdb"
          },
          "tmdb": {
            "xemOrigin": null,
            "apiParams": {
              "useZip": true,
              "language": "en"
            },
            "showUrl": "https://www.themoviedb.org/tv/",
            "id": 4,
            "icon": "tmdb16.png",
            "name": "TMDB",
            "scene_loc": "https://cdn.pymedusa.com/scene_exceptions/scene_exceptions_tmdb.json",
            "enabled": true,
            "baseUrl": "https://www.themoviedb.org/",
            "mappedTo": "tmdb_id",
            "identifier": "tmdb"
          },
          "tvmaze": {
            "xemOrigin": null,
            "apiParams": {
              "useZip": true,
              "language": "en"
            },
            "showUrl": "http://www.tvmaze.com/shows/",
            "id": 3,
            "icon": "tvmaze16.png",
            "name": "TVmaze",
            "scene_loc": "https://cdn.pymedusa.com/scene_exceptions/scene_exceptions_tvmaze.json",
            "enabled": true,
            "baseUrl": "http://api.tvmaze.com/",
            "mappedTo": "tvmaze_id",
            "identifier": "tvmaze"
          }
        },
        "main": {
          "externalMappings": {<truncated>},
          "statusMap": {<truncated>},
          "traktIndexers": {<truncated>},
          "validLanguages": [<truncated>],
          "langabbvToId": {<truncated>}
        }
      }
    }
  }
}

},
computedBase() {
// Return prop before HTML element to allow tests to mock
if (this.base) {
Expand All @@ -39,11 +51,11 @@ Vue.component('app-link', {
return document.getElementsByTagName('base')[0].getAttribute('href');
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any problem with doing this:

const { base } = this;
// Return prop before HTML element to allow tests to mock
return this.base || document.getElementsByTagName('base')[0].getAttribute('href');

It's just a personal preference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need to make sure document isn't ever called since it'll throw an error in the tests. I know how bad that sounds but I'll fix it once I move this to a .vue file.

Copy link
Contributor

@sharkykh sharkykh Jul 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

AFAIK, JavaScript logical statements are evaluated from left to right and not all at once.
Meaning if Boolean(this.base) is true, it will return that value. As long as you pass the base in the tests, should be good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not that. It's the testing framework seeing it in the AST that's the issue.

},
computedHref() {
const { indexerId, placeholder } = this;
const { href, indexerId, placeholder, indexerName } = this;
if (indexerId && placeholder) {
return this.href.replace(placeholder, MEDUSA.config.indexers.indexerIdToName(indexerId));
return href.replace(placeholder, indexerName);
}
return this.href;
return href;
},
isIRC() {
return this.computedHref.startsWith('irc://');
Expand All @@ -60,11 +72,6 @@ Vue.component('app-link', {
isHashPath() {
return this.computedHref.startsWith('#');
},
isAnonymised() {
const { anonRedirect } = this.config;
const href = this.computedHref;
return anonRedirect && href.startsWith(anonRedirect);
},
anonymisedHref() {
const { anonRedirect } = this.config;
const href = this.computedHref;
Expand Down Expand Up @@ -92,11 +99,6 @@ Vue.component('app-link', {
};
}

// @TODO: Remove this once we move to vue only
if (isAnonymised) {
throw new Error('Still using anon_url in Python!');
}

return {
is: 'a',
target: isAbsolute && isExternal ? '_blank' : '_self',
Expand Down Expand Up @@ -124,8 +126,7 @@ Vue.component('app-link', {
rel: isAbsolute && isExternal ? 'noreferrer' : undefined
};
}
},
template: '#app-link-template'
}
});
</script>
<style>
Expand Down
33 changes: 17 additions & 16 deletions themes/dark/templates/vue-components/app-link.mako
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@
</component>
</script>
<script>
Vue.component('app-link', {
Vue.component({
name: 'app-link',
template: '#app-link-template',
store,
props: {
to: [String, Object],
Expand All @@ -30,6 +31,17 @@ Vue.component('app-link', {
config() {
return this.$store.state.config;
},
indexerName() {
const { indexerId, config } = this;
if (!indexerId) {
return '';
}
return Object.keys(config.indexers).filter(indexer => {
if (config.indexers[indexer].id === parseInt(indexerId, 10)) {
return config.indexers[indexer].name;
}
})[0];
},
computedBase() {
// Return prop before HTML element to allow tests to mock
if (this.base) {
Expand All @@ -39,11 +51,11 @@ Vue.component('app-link', {
return document.getElementsByTagName('base')[0].getAttribute('href');
},
computedHref() {
const { indexerId, placeholder } = this;
const { href, indexerId, placeholder, indexerName } = this;
if (indexerId && placeholder) {
return this.href.replace(placeholder, MEDUSA.config.indexers.indexerIdToName(indexerId));
return href.replace(placeholder, indexerName);
}
return this.href;
return href;
},
isIRC() {
return this.computedHref.startsWith('irc://');
Expand All @@ -60,11 +72,6 @@ Vue.component('app-link', {
isHashPath() {
return this.computedHref.startsWith('#');
},
isAnonymised() {
const { anonRedirect } = this.config;
const href = this.computedHref;
return anonRedirect && href.startsWith(anonRedirect);
},
anonymisedHref() {
const { anonRedirect } = this.config;
const href = this.computedHref;
Expand Down Expand Up @@ -92,11 +99,6 @@ Vue.component('app-link', {
};
}

// @TODO: Remove this once we move to vue only
if (isAnonymised) {
throw new Error('Still using anon_url in Python!');
}

return {
is: 'a',
target: isAbsolute && isExternal ? '_blank' : '_self',
Expand Down Expand Up @@ -124,8 +126,7 @@ Vue.component('app-link', {
rel: isAbsolute && isExternal ? 'noreferrer' : undefined
};
}
},
template: '#app-link-template'
}
});
</script>
<style>
Expand Down
33 changes: 17 additions & 16 deletions themes/light/templates/vue-components/app-link.mako
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@
</component>
</script>
<script>
Vue.component('app-link', {
Vue.component({
name: 'app-link',
template: '#app-link-template',
store,
props: {
to: [String, Object],
Expand All @@ -30,6 +31,17 @@ Vue.component('app-link', {
config() {
return this.$store.state.config;
},
indexerName() {
const { indexerId, config } = this;
if (!indexerId) {
return '';
}
return Object.keys(config.indexers).filter(indexer => {
if (config.indexers[indexer].id === parseInt(indexerId, 10)) {
return config.indexers[indexer].name;
}
})[0];
},
computedBase() {
// Return prop before HTML element to allow tests to mock
if (this.base) {
Expand All @@ -39,11 +51,11 @@ Vue.component('app-link', {
return document.getElementsByTagName('base')[0].getAttribute('href');
},
computedHref() {
const { indexerId, placeholder } = this;
const { href, indexerId, placeholder, indexerName } = this;
if (indexerId && placeholder) {
return this.href.replace(placeholder, MEDUSA.config.indexers.indexerIdToName(indexerId));
return href.replace(placeholder, indexerName);
}
return this.href;
return href;
},
isIRC() {
return this.computedHref.startsWith('irc://');
Expand All @@ -60,11 +72,6 @@ Vue.component('app-link', {
isHashPath() {
return this.computedHref.startsWith('#');
},
isAnonymised() {
const { anonRedirect } = this.config;
const href = this.computedHref;
return anonRedirect && href.startsWith(anonRedirect);
},
anonymisedHref() {
const { anonRedirect } = this.config;
const href = this.computedHref;
Expand Down Expand Up @@ -92,11 +99,6 @@ Vue.component('app-link', {
};
}

// @TODO: Remove this once we move to vue only
if (isAnonymised) {
throw new Error('Still using anon_url in Python!');
}

return {
is: 'a',
target: isAbsolute && isExternal ? '_blank' : '_self',
Expand Down Expand Up @@ -124,8 +126,7 @@ Vue.component('app-link', {
rel: isAbsolute && isExternal ? 'noreferrer' : undefined
};
}
},
template: '#app-link-template'
}
});
</script>
<style>
Expand Down