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

feat: Update IMDb.js to add URL to item #3385

Closed
wants to merge 6 commits into from
Closed

Conversation

johev09
Copy link

@johev09 johev09 commented Nov 8, 2024

Changes

IMDb.js:

  • Adding URL to item in scrape(...) method

Notes

  • This changes populates the URL field when saving a IMDb page to Zotero using Zotero Connector.
  • Populating the URL makes the Zotero item clickable
  • Opens IMDb page on double-clicking the Zotoer item.

Testing

  • Tested on my local machine.
  • Verified URL is getting populating on saving an IMDb link.

Pull Request Test failed wtih:
```
  TranslatorTester: Running IMDb Test 2
    TranslatorTester: Translating https://www.imdb.com/find?q=shakespeare&s=tt
    TranslatorTester: IMDb Test 2: failed (Detection failed)
```

The existing `multiple` check checked for `/find?` in url but IMDb is updating the URL to `/find/?` which is failing the check.

Example URL: https://www.imdb.com/find/?q=shakespeare&s=tt
@AbeJellinek
Copy link
Member

I don't think this is really correct. We could add a Snapshot attachment, but the URL field is for URLs where full-text (full-film?) content can be found. IMDb is a catalog site akin to a library catalog; we don't add URLs for those.

(I'm also confused about the seemingly unrelated changes to the URL check and XPaths.)

@johev09
Copy link
Author

johev09 commented Nov 25, 2024

Hi, Thanks for checking my PR.

Oh, I didn't know that about the URL field. I just checked the documentation and it does mention the same. Thanks for clariyfing that.

Regarding the xpath changes, the search page xpaths have changed and was making the test cases fail so tried to update them but couldn't make it work.

I will go ahead and close this PR as the primary change is not needed.

@johev09 johev09 closed this Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants