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

Clean up and rename computed variables #10307

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Clean up and rename computed variables #10307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7af98a2
Clean up computed variables and move popular identifier logic into Id…
schu96 Jan 9, 2025
38de33a
Restore previous popular id behavior, unify identifier config inputs
schu96 Jan 11, 2025
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
78 changes: 37 additions & 41 deletions openlibrary/components/IdentifiersInput.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,15 @@
<th>
<select class="form-control" v-model="selectedIdentifier" name="name">
<option disabled value="">Select one</option>
<template v-if="isEdition">
<option v-for="entry in popularEditionConfigs" :key="entry.name" :value="entry.name">
<template v-if="hasPopularIds">
<option v-for="entry in popularIdDict" :key="entry.name" :value="entry.name">
{{ entry.label }}
</option>
<option disabled value="">---</option>
<option v-for="entry in secondaryEditionConfigs" :key="entry.name" :value="entry.name">
{{ entry.label }}
</option>
</template>
<template v-else>
<option v-for="idConfig in identifierConfigsByKey" :key="idConfig.name" :value="idConfig.name">
{{idConfig.label}}
</option>
</template>
<option v-for="idConfig in identifierConfigsByKey" :key="idConfig.name" :value="idConfig.name">
{{idConfig.label}}
</option>
</select>
</th>
<th>
Expand Down Expand Up @@ -60,6 +55,7 @@ const identifierPatterns = {
amazon: /^B[0-9A-Za-z]{9}$/,
youtube: /^@[A-Za-z0-9_\-.]{3,30}/,
}

export default {
// Props are for external options; if a subelement of this is modified,
// the view automatically re-renders
Expand All @@ -74,13 +70,15 @@ export default {
* {"identifiers": [{"label": "ISNI", "name": "isni", "notes": "", "url": "http://www.isni.org/@@@", "website": "http://www.isni.org/"}, ... ]}
*/
id_config_string: {
type: String
type: String,
required: true
},
/** see createHiddenInputs function for usage
* #hiddenEditionIdentifiers, #hiddenWorkIdentifiers
*/
output_selector: {
type: String
type: String,
required: true
},
input_prefix: {
/*
Expand All @@ -89,19 +87,20 @@ export default {
*/
type: String
},
/* Props specifically for Editions */
/* Editions and works can have multiple identifiers in the form of a list */
multiple: {
/* Editions can have multiple identifiers in the form of a list */
type: String,
default: 'false',
},
admin: {
type: String,
default: 'false',
},
/* Maintains identifier order in the dropdown list */
edition_popular: {},
secondary_identifiers: {}
/** Maintains identifier order in the dropdown list
* Expects a list containing the name field from the
* respective identifiers.yaml file
*/
popular_ids: {},
},

// Data is for internal stuff. This needs to be a function so that we get
Expand All @@ -115,40 +114,37 @@ export default {
},

computed: {
popularEditionConfigs: function() {
if (this.edition_popular) {
const popularConfigs = JSON.parse(decodeURIComponent(this.edition_popular));
return Object.fromEntries(popularConfigs.map(e => [e.name, e]));
}
return {};
parsedConfig: function() {
Copy link
Collaborator

@cdrini cdrini Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So this can then be renamed to idConfig, and we don't specify parsed

return JSON.parse(decodeURIComponent(this.id_config_string))
},
secondaryEditionConfigs: function() {
if (this.secondary_identifiers) {
const secondConfigs = JSON.parse(decodeURIComponent(this.secondary_identifiers));
return Object.fromEntries(secondConfigs.map(e => [e.name, e]));
popularIdDict: function() {
Copy link
Collaborator

@cdrini cdrini Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, looking great! This can now just be a list, since we never have to do a look-up by identifier name, which makes this code much tighter!

        popularIds: function() {
            const popularIdNames = JSON.parse(decodeURIComponent(this.popular_ids));
            return this.parsedConfig
                .filter((config) => popularConfig.includes(config.name));
        },

Copy link
Collaborator

Choose a reason for hiding this comment

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

And to keep the naming consistent, let's renaming the property to popular_ids_string ; that way any time we have a JSON/string input, it ends with foo_string, and then the parsed non-string version is just foo.

if (this.popular_ids) {
let popularConfig = JSON.parse(decodeURIComponent(this.popular_ids))
popularConfig = this.parsedConfig.filter((config) => {
if (popularConfig.includes(config.name)) {
return true;
}
return false;
})
return Object.fromEntries(popularConfig.map(e => [e.name, e]))
}
return {};
},
identifierConfigsByKey: function(){
const parsedConfigs = JSON.parse(decodeURIComponent(this.id_config_string));
if (this.isEdition) {
const betterConfigs = JSON.parse(decodeURIComponent(this.id_config_string));
return Object.fromEntries(betterConfigs.map(e => [e.name, e]));
}
return Object.fromEntries(parsedConfigs['identifiers'].map(e => [e.name, e]));
identifierConfigsByKey: function() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This now also looks un-needed and can be removed ; we can replace where this is used with the parsedConfig computed.

return Object.fromEntries(this.parsedConfig.map(e => [e.name, e]));
},
isAdmin() {
isAdmin: function() {
return this.admin.toLowerCase() === 'true';
},
isEdition() {
return this.multiple.toLowerCase() === 'true' && this.edition_popular;
},
saveIdentifiersAsList() {
saveIdentifiersAsList: function() {
return this.multiple.toLowerCase() === 'true';
},
setButtonEnabled: function(){
return this.selectedIdentifier !== '' && this.inputValue !== '';
}
},
hasPopularIds: function() {
return Object.keys(this.popularIdDict).length !== 0;
},
},

methods: {
Expand Down Expand Up @@ -218,7 +214,7 @@ export default {
},
selectIdentifierByInputValue: function() {
// Ignore for edition identifiers
if (this.isEdition) {
if (this.saveIdentifiersAsList) {
return;
}
// Selects the dropdown identifier based on the input value when possible
Expand Down
2 changes: 1 addition & 1 deletion openlibrary/templates/books/edit.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ <h3 class="editFormBookAuthors">
<div id="hiddenWorkIdentifiers"></div>
<div id="identifiers-display-works">
$ admin = str(ctx.user.is_admin() or ctx.user.is_super_librarian())
$:render_component('IdentifiersInput', attrs=dict(assigned_ids_string=work.get_identifiers().values(), output_selector='#hiddenWorkIdentifiers', id_config_string=work_config, input_prefix='work--identifiers', multiple='true', admin=admin))
$:render_component('IdentifiersInput', attrs=dict(assigned_ids_string=work.get_identifiers().values(), output_selector='#hiddenWorkIdentifiers', id_config_string=work_config.identifiers, input_prefix='work--identifiers', multiple='true', admin=admin))
</div>
</fieldset>
</div>
Expand Down
2 changes: 1 addition & 1 deletion openlibrary/templates/books/edit/edition.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@
$ set1 = [id_dict[name] for name in popular if name in id_dict and name not in exclude]
$ set2 = sorted([id for id in edition_config.identifiers if id.name not in popular and id.name not in exclude], key=lambda id:id.label)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh and set1 and set2 might now no longer be used!


$:render_component('IdentifiersInput', attrs=dict(id_config_string=edition_config.identifiers, assigned_ids_string=book.get_identifiers().values(), input_prefix='edition--identifiers', output_selector='#hiddenEditionIdentifiers', multiple='true', admin=str(is_privileged_user), edition_popular=set1,secondary_identifiers=set2))
$:render_component('IdentifiersInput', attrs=dict(id_config_string=edition_config.identifiers, assigned_ids_string=book.get_identifiers().values(), input_prefix='edition--identifiers', output_selector='#hiddenEditionIdentifiers', multiple='true', popular_ids=popular,admin=str(is_privileged_user)))
</div>
</div>
</div>
Expand Down
4 changes: 2 additions & 2 deletions openlibrary/templates/type/author/edit.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
$def with (page)

$ author_config = get_identifier_config('author')
$var title: $page.name

$putctx("robots", "noindex,nofollow")
Expand Down Expand Up @@ -107,7 +107,7 @@ <h1>$_("Edit Author")</h1>
<div id="hiddenAuthorIdentifiers"></div>
<div id="identifiers-display">
$ admin = ctx.user.is_admin() or ctx.user.is_super_librarian()
$:render_component('IdentifiersInput', attrs=dict(assigned_ids_string=dict(page.remote_ids),output_selector='#hiddenAuthorIdentifiers', admin=str(admin), input_prefix='author--remote_ids', id_config_string=dict(get_identifier_config('author'))))
$:render_component('IdentifiersInput', attrs=dict(assigned_ids_string=dict(page.remote_ids),output_selector='#hiddenAuthorIdentifiers', admin=str(admin), input_prefix='author--remote_ids', id_config_string=author_config.identifiers))
</div>
<br>
<fieldset class="major">
Expand Down
Loading