Skip to content

Conversation

@tr1ten
Copy link

@tr1ten tr1ten commented Jan 30, 2022

Allow user to import book directly from amazon.

@tr1ten tr1ten marked this pull request as ready for review February 8, 2022 15:08
Copy link

@kellnerd kellnerd left a comment

Choose a reason for hiding this comment

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

Great job, I've been anticipating BB importer scripts ever since I first discovered the project, so thanks a lot for your work on the server's POST routes and the userscript 👍

I've just tested it on https://test.bookbrainz.org and noticed a few things which were not working or which could be improved:

  1. When you change the script's file extension to .user.js, a userscript manager automatically offers to install the script in GitHub's raw file mode.
  2. There should be a check in the code which aborts the import if the userscript is running on an unsupported Amazon page (not all products are books).
  3. Following that approach, the script should simply skip propertis for which it fails during extraction (missing DOM element) or parsing (unexpected format of the text).
  4. I am not sure whether the order of the dimensions (height, width and depth) is correct, probably there is not even a consistent order (see my example link below).
  5. The whole script fails as soon as the page's language is not English as the properties are named differently in other languages (e.g. I had to set the language to English on German Amazon to make it work).
  6. On MB it's best practice that importer scripts also seed the edit note with the URL of the data source and the name and version of the userscript (draws attention of other users and helps to understand potential systematic issues with a certain version of a script).

amazon-import.js Outdated
`,
"bookbrainz"
);
GM_addStyle(
Copy link

Choose a reason for hiding this comment

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

You can combine all these stylesheet injections into a single call which injects all rules at once.

amazon-import.js Outdated
"bookbrainz"
);
const convertToSI = {
cm: 1,
Copy link

Choose a reason for hiding this comment

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

Suggested change
cm: 1,
cm: 10, // BB expects mm

amazon-import.js Outdated
}
let [height, width, depth] = res.dimensions?.split("x");
let lenghtToSIkey = depth.match(/[A-Za-z]+/gi)[0];
let wtToSIkey = res.weight.match(/[A-Za-z]+/gi)[0];
Copy link

Choose a reason for hiding this comment

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

Suggested change
let wtToSIkey = res.weight.match(/[A-Za-z]+/gi)[0];
let wtToSIkey = res.weight?.match(/[A-Za-z]+/gi)[0];

Prevents the script from failing if the weight is not available, but this should be fixed properly to skip all further processing of the missing field.

amazon-import.js Outdated
Comment on lines 175 to 176
date = new Date(date);
date = date?.toISOString()?.split("T")[0];
Copy link

Choose a reason for hiding this comment

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

Suggested change
date = new Date(date);
date = date?.toISOString()?.split("T")[0];
date = new Date(date.replace('.', '')); // temporary fix for unsupported dates like `20 Oct. 2021`
date = [date.getFullYear(), date.getMonth() + 1, date.getDate()].join('-');

The second line is necessary to avoid dates which are off by one day for certain time zones.
But this is just a quick fix, the better solution would be to use a date parsing library as Date has a lot of caveats [1].

amazon-import.js Outdated
}
try {
// Setting up UI
const submitUrl = "https://bookbrainz.org/edition/create";
Copy link

Choose a reason for hiding this comment

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

Suggested change
const submitUrl = "https://bookbrainz.org/edition/create";
const submitUrl = "https://test.bookbrainz.org/edition/create";

It would be helpful to have the link temporarily point to the test server until the changes have been released.

@tr1ten
Copy link
Author

tr1ten commented Feb 9, 2022

@kellnerd Thanks for reviewing the PR and giving the suggestions!

  1. There is already a check if author exist for a product only then run the import script.
  2. As for non English domains of amazon, i suppose we have to add different css selectors for each domain.
  3. I also think submission note would be helpful when seeding, but currently BB PR doesn't support submission note seeding, i will update this PR as soon it does.

Copy link

@kellnerd kellnerd left a comment

Choose a reason for hiding this comment

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

There is already a check if author exist for a product only then run the import script.

Maybe I'm blind, but I don't see anything in the code which extracts or parses the author of the book at all... Are these local chnages which you haven't pushed to GitHub yet?

I also think submission note would be helpful when seeding, but currently BB PR doesn't support submission note seeding, i will update this PR as soon it does.

Great, I've already seen your commit for this feature in the BB PR, so let's wait for @MonkeyDo to review it.

<label class="bb-flabel" for="bb-date">Start date:</label>
<input class="bb-finput" name="editionSection.releaseDate" type="date" value=${
itemDetails.date
} id="bb-date" name="" value="2018-07-22">
Copy link

Choose a reason for hiding this comment

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

Suggested change
} id="bb-date" name="" value="2018-07-22">
} id="bb-date">

These duplicate attributes are probably left over from testing.

Comment on lines 156 to 160
date = [
date.getFullYear(),
((date.getMonth() < 10) ? "0" : "") + `${date.getMonth() + 1}`,
date.getDate(),
].join("-");
Copy link

Choose a reason for hiding this comment

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

Sorry, I forgot to pad the date (was not necessary for my example). The day also has to be padded, so I suggest to use the following (year is already long enough, so it will be left as is by the mapper function):

Suggested change
date = [
date.getFullYear(),
((date.getMonth() < 10) ? "0" : "") + `${date.getMonth() + 1}`,
date.getDate(),
].join("-");
date = [date.getFullYear(), date.getMonth() + 1, date.getDate()]
.map((component) => String(component).padStart(2, '0'))
.join('-');

g: 1,
pounds: 453.6,
ounces: 28.3,
inches: 2.5,
Copy link

Choose a reason for hiding this comment

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

Suggested change
inches: 2.5,
inches: 25.4, // mm

Comment on lines 273 to 274
<textarea class="bb-finput" name="submissionSection" id="bb-format">${submissionNote}</textarea>
<button class="bb-btn" type="submit">Submit</button>
Copy link

Choose a reason for hiding this comment

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

Suggested change
<textarea class="bb-finput" name="submissionSection" id="bb-format">${submissionNote}</textarea>
<button class="bb-btn" type="submit">Submit</button>

Has to be removed or commented out until the server-side has been fixed.

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.

2 participants