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

Jump to bottom

Bugfix 2395 citations display #2510

Merged
merged 4 commits into from
Sep 5, 2024
Merged

Bugfix 2395 citations display #2510

merged 4 commits into from
Sep 5, 2024

Conversation

rushirajnenuji
Copy link
Member

@rushirajnenuji rushirajnenuji commented Sep 5, 2024 •
edited
Loading

Fixes issues with Citation display on dataset and repository landing pages.
#2395

@rushirajnenuji
Only use inline import, remove the top level model import to avoid ci…
28f6859 
…rcular dependency

Only use inline import, remove the top level model import to avoid circular dependency

Reference: #2395
@rushirajnenuji
Restore dual require for Citations collection; add dual require for C…
e2e9b7c 
…itation Model

Restore dual require for Citations collection; add dual require for Citation Model

#2395
@rushirajnenuji
Merge branch 'develop' into bugfix-2395-citations-display
e7e32ff
@rushirajnenuji
Fix ES Lint issues with Citation collection and model
82cb423 
Fix ES Lint issues with Citation collection and model
github-actions[bot]
github-actions bot reviewed Sep 5, 2024
View reviewed changes
src/js/collections/Citations.js
@@ -1,36 +1,37 @@
"use strict";

define(["jquery", "underscore", "backbone", "models/CitationModel"], function (
define(["jquery", "underscore", "backbone", "models/CitationModel"], (
$,
_,
Backbone,
CitationModel,
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [eslint] <no-unused-vars> reported by reviewdog 🐶
'CitationModel' is defined but never used. Allowed unused args must match /^_/u.

src/js/collections/Citations.js
/** @lends Citations.prototype */ {
// eslint-disable-next-line object-shorthand
model: function (attrs, options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ [eslint] <func-names> reported by reviewdog 🐶
Unexpected unnamed method 'model'.

src/js/collections/Citations.js
model: function (attrs, options) {
// We use the inline require here in addition to the define above to
// avoid an issue caused by the circular dependency between
// CitationModel and Citations
var CitationModel = require("models/CitationModel");
const CitationModel = require("models/CitationModel");
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [eslint] <no-shadow> reported by reviewdog 🐶
'CitationModel' is already declared in the upper scope on line 7 column 3.

src/js/models/CitationModel.js
@@ -1,11 +1,11 @@
"use strict";

define(["jquery", "underscore", "backbone", "collections/Citations"], function (
define(["jquery", "underscore", "backbone", "collections/Citations"], (
$,
_,
Backbone,
Citations,
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [eslint] <no-unused-vars> reported by reviewdog 🐶
'Citations' is defined but never used. Allowed unused args must match /^_/u.

src/js/models/CitationModel.js
* @param {Object} options - Options to pass to the parse() method.
* @returns {Object} The parsed response
* @param {object} response - The response from the metrics-service API
* @param {object} options - Options to pass to the parse() method.
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ [eslint] <jsdoc/check-param-names> reported by reviewdog 🐶
@param "options" does not match an existing function parameter.

src/js/models/CitationModel.js
@@ -972,10 +976,11 @@ define(["jquery", "underscore", "backbone", "collections/Citations"], function (
/**
* Check if a string is a valid DOI.
* @param {string} doi - The string to check.
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ [eslint] <jsdoc/check-param-names> reported by reviewdog 🐶
Expected @param names to be "str". Got "doi, str".

src/js/models/CitationModel.js
@@ -972,10 +976,11 @@ define(["jquery", "underscore", "backbone", "collections/Citations"], function (
/**
* Check if a string is a valid DOI.
* @param {string} doi - The string to check.
* @param str
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ [eslint] <jsdoc/require-param-description> reported by reviewdog 🐶
Missing JSDoc @param "str" description.

src/js/models/CitationModel.js
@@ -972,10 +976,11 @@ define(["jquery", "underscore", "backbone", "collections/Citations"], function (
/**
* Check if a string is a valid DOI.
* @param {string} doi - The string to check.
* @param str
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ [eslint] <jsdoc/require-param-type> reported by reviewdog 🐶
Missing JSDoc @param "str" type.

src/js/models/CitationModel.js
@@ -1058,7 +1063,7 @@ define(["jquery", "underscore", "backbone", "collections/Citations"], function (
* @returns {string} Returns the URL for the citation or an empty string.
* @since 2.23.0
*/
getURL: function () {
getURL() {
const urlSources = ["view_url", "source_url", "sid_url", "pid_url"];
for (let i = 0; i < urlSources.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [eslint] <no-plusplus> reported by reviewdog 🐶
Unary operator '++' used.

src/js/models/CitationModel.js
@@ -1075,7 +1080,7 @@ define(["jquery", "underscore", "backbone", "collections/Citations"], function (
* empty string.
* @since 2.23.0
*/
getID: function () {
getID() {
const idSources = ["pid", "seriesId", "source_url"];
for (let i = 0; i < idSources.length; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [eslint] <no-plusplus> reported by reviewdog 🐶
Unary operator '++' used.

github-actions[bot]
github-actions bot reviewed Sep 5, 2024
View reviewed changes
src/js/models/CitationModel.js
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ [eslint] <jsdoc/require-returns> reported by reviewdog 🐶
Missing JSDoc @returns declaration.

metacatui/src/js/models/CitationModel.js

Lines 207 to 220 in 82cb423

/**
* Override the default Backbone.Model.set() method to format the title,
* page, and volume attributes before setting them, and ensure that
* attributes that are different formats of the same value are in sync,
* including: origin and originArray; pid and pid_url; seriesId and
* seriesId_url. This method will prevent the sourceModel attribute from
* being set here.
* @param {string | object} key - The attribute name to set, or an object of
* attribute names and values to set.
* @param {string | number | object} val - The value to set the attribute to.
* @param {object} options - Options to pass to the set() method.
* @see https://backbonejs.org/#Model-set
* @since 2.23.0
*/

github-actions[bot]
github-actions bot reviewed Sep 5, 2024
View reviewed changes
src/js/models/CitationModel.js
Copy link
Contributor

Choose a reason for hiding this comment

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

🚫 [eslint] <prefer-regex-literals> reported by reviewdog 🐶
Use a regular expression literal instead of the 'RegExp' constructor.

metacatui/src/js/models/CitationModel.js

Lines 784 to 786 in 82cb423

const regex = new RegExp(
"^https?:\\/\\/orcid.org\\/(\\d{4}-){3}(\\d{3}[0-9X])$",
);

@robyngit robyngit self-assigned this Sep 5, 2024
@robyngit robyngit linked an issue Sep 5, 2024 that may be closed by this pull request
Issues with Citation Display #2395
Closed
robyngit
robyngit approved these changes Sep 5, 2024
View reviewed changes
Copy link
Member

@robyngit robyngit left a comment

Choose a reason for hiding this comment

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

🪄 you fixed what I broke! Sorry about that unnecessary circular dependency and thanks for figuring this out!!!

@robyngit robyngit merged commit 1094b26 into develop Sep 5, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@robyngit robyngit robyngit approved these changes

Assignees

@robyngit robyngit

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Issues with Citation Display
2 participants
@rushirajnenuji @robyngit