Create check-in form from template #10974

jimchamp · 2025-07-02T15:40:28Z

Closes #10582

Makes the following changes:

Decouples yearly reading goal and check-in code
Creates new check-in forms from a single template
Consolidates client-side check-in code in the my-books module
Check-in form now displayed in a colorbox
Replace check-in $.ajax calls with fetch
Updates check-in unit tests

Technical

Testing

Screenshot

Stakeholders

accesslint

There are accessibility issues in these changes.

tests/unit/js/sample-html/checkIns-test-data.js

openlibrary/templates/my_books/check_ins/check_in_form.html

tests/unit/js/sample-html/checkIns-test-data.js

openlibrary/templates/my_books/check_ins/check_in_form.html

jimchamp · 2025-07-02T15:58:54Z

openlibrary/plugins/openlibrary/js/my-books/MyBooksDropper/CheckInComponents.js

+                    this.checkInDisplay.hide()
+                    this.checkInPrompt.show()
+                })
+                .catch(err => {


Suggested change

.catch(err => {

.catch(() => {

err is unused, and this is causing our CI/CD checks to fail.

jimchamp · 2025-07-02T15:59:41Z

openlibrary/plugins/upstream/checkins.py

+        data = json.loads(web.data())
+        if not self.validate_data(data):
+            raise web.HTTPError(
+                "404 Bad Request", headers={"Content-Type": "application/json"}


Suggested change

"404 Bad Request", headers={"Content-Type": "application/json"}

"400 Bad Request", headers={"Content-Type": "application/json"}

Fix copy-paste oversight.

This might be best as a 400 Bad Request than a 404

jimchamp · 2025-07-03T17:09:14Z

openlibrary/plugins/upstream/readinggoals.py

This should probably be renamed to yearly_reading_goals.py, to match the name of the DB API for this feature.

cdrini

Woohoo great refactor! Separating out the yearly reading goal logic makes a ton of sense. And this fixes the bug about not being able to update the date! 🥳

No blockers, a few suggestions on code/naming/arch.

I tested:

Moving a book to "Already read" shows the prompt
I can click "2025" and it saves
I can then later edit it to add month/year
Today link works
Edition persisted when on books page
- Existing bug: not persisted when from search page.

Merge at your discretion!

cdrini · 2025-07-07T13:19:15Z

openlibrary/plugins/openlibrary/js/my-books/MyBooksDropper.js

+        const splitKey = this.workKey ? this.workKey.split('/') : ['']
+        const workOlid = splitKey[splitKey.length - 1]


Suggested change

const splitKey = this.workKey ? this.workKey.split('/') : ['']

const workOlid = splitKey[splitKey.length - 1]

const workOlid = this.workKey ? this.workKey.split('/')[2] : '';

cdrini · 2025-07-07T13:26:04Z

openlibrary/plugins/openlibrary/js/my-books/MyBooksDropper.js

+
+        const splitKey = this.workKey ? this.workKey.split('/') : ['']
+        const workOlid = splitKey[splitKey.length - 1]
+        this.checkInComponents = new CheckInComponents(document.querySelector(`#check-in-container-${workOlid}`))


Note this could cause some subtle bugs if a book appears twice on a page; can we avoid having it being a global id? Something like this.dropper.querySelector('.checkin-form') ? Although it seems like this exists outside the my-books div. I think that makes the dividing line between our components a bit messy; it might make more sense to either have checkin-form be its own independent component outside the book dropper.

But those are kind of bigger changes; maybe quicker might be this.dropper.parentElement.querySelector('.last-read-date') with a comment about the assumptions.

cdrini · 2025-07-07T13:26:46Z

openlibrary/plugins/openlibrary/js/my-books/MyBooksDropper.js

+
+        const splitKey = this.workKey ? this.workKey.split('/') : ['']
+        const workOlid = splitKey[splitKey.length - 1]
+        this.checkInComponents = new CheckInComponents(document.querySelector(`#check-in-container-${workOlid}`))


Regardless, a bit odd this is plural, confused me for a bit whether this was initializing all the check in components on the page.

Suggested change

this.checkInComponents = new CheckInComponents(document.querySelector(`#check-in-container-${workOlid}`))

this.checkInComponent = new CheckInComponents(document.querySelector(`#check-in-container-${workOlid}`))

A CheckInComponents object is comprised of three individual components:

The check-in form, which is displayed in a modal.

The check-in prompt, which appears below the dropper and prompts the patron for a last-read date.

A display that shows the last check-in date, and provided an affordance to edit the date.

cdrini · 2025-07-07T13:29:15Z

openlibrary/plugins/openlibrary/js/my-books/MyBooksDropper/CheckInComponents.js

+ *
+ * @class
+ */
+export class CheckInComponents {


Ditto here, the plural is a little misleading.

Suggested change

export class CheckInComponents {

export class CheckInComponent {

openlibrary/plugins/openlibrary/js/my-books/MyBooksDropper/CheckInComponents.js

cdrini · 2025-07-07T13:41:23Z

openlibrary/plugins/openlibrary/js/my-books/MyBooksDropper/CheckInComponents.js

+        let hiddenModalContentContainer = document.querySelector('#hidden-modal-content-container')
+        if (!hiddenModalContentContainer) {
+            hiddenModalContentContainer = document.createElement('div')
+            hiddenModalContentContainer.classList.add('hidden')
+            hiddenModalContentContainer.id = 'hidden-modal-content-container'
+            document.body.appendChild(hiddenModalContentContainer)
+        }
+
+        const modalContent = this.createModalContentFromTemplate()
+        hiddenModalContentContainer.appendChild(modalContent)
+
+        this.modalContent = hiddenModalContentContainer.querySelector(`#modal-content-${this.config.workOlid}`)


If I understand correctly, this is creating one shared #hidden-modal-content-container used by all the CheckIn components, and putting inside it CheckInForms from all check-ins on the page? And then only getting things with the given work olid to apply to this.

Could we make the form generate dynamically only when needed? The current approach will create a lot of DOM, the majority of which is very unlikely to be used, which has a memory impact.

Will create follow-up issue to track this.

openlibrary/plugins/openlibrary/js/my-books/MyBooksDropper/CheckInComponents.js

cdrini · 2025-07-07T14:29:35Z

openlibrary/plugins/upstream/checkins.py

+        data = json.loads(web.data())
+        if not self.validate_data(data):
+            raise web.HTTPError(
+                "404 Bad Request", headers={"Content-Type": "application/json"}


This might be best as a 400 Bad Request than a 404

cdrini · 2025-07-07T14:53:06Z

openlibrary/templates/my_books/check_ins/check_in_form.html

+      <span>
+        <label class="check-in__year-label">$_('Year:')</label>
+        <select class="check-in__select" name="year">
+          <option value="">$_('Year')</option>
+          <option class="hidden show-if-local-year" value="$(year + 1)">$(year + 1)</option>
+          $for i in range(120):
+            <option value="$(year - i)">$(year - i)</option>
+        </select>
+      </span>


Oh and another thing: we can avoid having to align these via JS by using this HTML trick. This automatically associated the label without having to specify for.

Suggested change

<span>

<label class="check-in__year-label">$_('Year:')</label>

<select class="check-in__select" name="year">

<option value="">$_('Year')</option>

<option class="hidden show-if-local-year" value="$(year + 1)">$(year + 1)</option>

$for i in range(120):

<option value="$(year - i)">$(year - i)</option>

</select>

</span>

<label>

$_('Year:')

<select class="check-in__select" name="year">

<option value="">$_('Year')</option>

<option class="hidden show-if-local-year" value="$(year + 1)">$(year + 1)</option>

$for i in range(120):

<option value="$(year - i)">$(year - i)</option>

</select>

</label>

jimchamp · 2025-07-15T23:46:52Z

Blocked on necessary rebase.

- Handler will fail fast if called while unauthenticated - Errors are returned as `application/json`

- Client-side check-in code has been consolidated in `ReadDateComponents.js` - Check-in templates have been moved to `templates/mybooks/` - Check-in form is now displayed in a `colorbox` component - Check-in forms are now created client-side, from a template rendered in `check_in_prompt.html`

The `ajax` `error` callbacks were not being executed when responses were not `OK`. These calls have been replaced with `fetch`. Any errors are handled by the caller.

Adds `dialog--open` class and `aria-controls` to the dialog opener affordances (the "Edit" and "Other" links). These changes cause the dialog openers to be initialized in the main `index.js` flow, in the typical manner. `open-modal` custom events and the `openModal` method were removed from `CheckInComponents.js, as the dialog openers are now initialized in `index.js``. Adds `initDialogClosers` function to `dialog.js` for adding listeners to elements that were created client-side.

codecov-commenter · 2025-07-30T15:48:21Z

Codecov Report

❌ Patch coverage is 20.67183% with 307 lines in your changes missing coverage. Please review.
✅ Project coverage is 17.85%. Comparing base (e027195) to head (d1ffc6a).
⚠️ Report is 700 commits behind head on master.

Files with missing lines	Patch %	Lines
...ry/js/my-books/MyBooksDropper/CheckInComponents.js	29.85%	174 Missing and 14 partials ⚠️
...rary/plugins/openlibrary/js/reading-goals/index.js	0.00%	80 Missing and 13 partials ⚠️
...rary/js/my-books/MyBooksDropper/ReadingLogForms.js	0.00%	7 Missing and 4 partials ⚠️
.../plugins/openlibrary/js/my-books/MyBooksDropper.js	0.00%	7 Missing and 3 partials ⚠️
openlibrary/plugins/openlibrary/js/dialog.js	0.00%	3 Missing ⚠️
openlibrary/plugins/openlibrary/js/index.js	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10974      +/-   ##
==========================================
+ Coverage   17.14%   17.85%   +0.71%     
==========================================
  Files          91       92       +1     
  Lines        4981     5096     +115     
  Branches      867      887      +20     
==========================================
+ Hits          854      910      +56     
- Misses       3588     3639      +51     
- Partials      539      547       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

for more information, see https://pre-commit.ci

accesslint bot reviewed Jul 2, 2025

View reviewed changes

jimchamp commented Jul 2, 2025

View reviewed changes

jimchamp marked this pull request as ready for review July 2, 2025 16:07

jimchamp force-pushed the read-dates-for-legacy-browsers branch 2 times, most recently from 648fbfa to 8d74f4f Compare July 3, 2025 16:56

jimchamp commented Jul 3, 2025

View reviewed changes

mekarpeles assigned cdrini Jul 3, 2025

cdrini added the Priority: 1 Do this week, receiving emails, time sensitive, . [managed] label Jul 3, 2025

cdrini approved these changes Jul 7, 2025

View reviewed changes

cdrini reviewed Jul 7, 2025

View reviewed changes

jimchamp added the State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] label Jul 15, 2025

cdrini assigned jimchamp and unassigned cdrini Jul 17, 2025

jimchamp added 14 commits July 28, 2025 17:07

Extract yearly reading goal code

207bd80

Update check-in POST handler

a782e02

- Handler will fail fast if called while unauthenticated - Errors are returned as `application/json`

Extract check-in component classes

788f734

Rename file

b3028de

Replace $.ajax calls with fetch

cbde5ae

The `ajax` `error` callbacks were not being executed when responses were not `OK`. These calls have been replaced with `fetch`. Any errors are handled by the caller.

Set edition key input in form

5abea6e

Update unit tests for check-in form

648e381

Set label elements' for attribute

282978e

Update for clarity

73d990c

Add unit test for form label accessibility

a45bb08

Fix broken import

5288870

Rename readinggoals.py

9b3301b

jimchamp force-pushed the read-dates-for-legacy-browsers branch from 37e3a6c to dc9211e Compare July 30, 2025 15:46

jimchamp removed the State: Blocked Work has stopped, waiting for something (Info, Dependent fix, etc. See comments). [managed] label Jul 30, 2025

Apply some notes from review

3e15cb3

jimchamp force-pushed the read-dates-for-legacy-browsers branch from eb0911d to 3e15cb3 Compare July 30, 2025 15:53

[pre-commit.ci] auto fixes from pre-commit.com hooks

d1ffc6a

for more information, see https://pre-commit.ci

jimchamp merged commit baf7303 into internetarchive:master Jul 30, 2025
5 checks passed

jimchamp deleted the read-dates-for-legacy-browsers branch July 30, 2025 17:20

hornc mentioned this pull request Jul 30, 2025

move check-in form inside conditionally hidden div #10581

Merged

	"404 Bad Request", headers={"Content-Type": "application/json"}
	"400 Bad Request", headers={"Content-Type": "application/json"}

		const splitKey = this.workKey ? this.workKey.split('/') : ['']
		const workOlid = splitKey[splitKey.length - 1]

	const splitKey = this.workKey ? this.workKey.split('/') : ['']
	const workOlid = splitKey[splitKey.length - 1]
	const workOlid = this.workKey ? this.workKey.split('/')[2] : '';

	this.checkInComponents = new CheckInComponents(document.querySelector(`#check-in-container-${workOlid}`))
	this.checkInComponent = new CheckInComponents(document.querySelector(`#check-in-container-${workOlid}`))

	export class CheckInComponents {
	export class CheckInComponent {

Uh oh!

Create check-in form from template #10974

Create check-in form from template #10974

Conversation

jimchamp commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Technical

Testing

Screenshot

Stakeholders

Uh oh!

accesslint bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jimchamp Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimchamp Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdrini left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimchamp commented Jul 15, 2025

Uh oh!

codecov-commenter commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jimchamp commented Jul 2, 2025 •

edited

Loading

jimchamp Jul 2, 2025 •

edited

Loading

jimchamp Jul 2, 2025 •

edited

Loading

cdrini left a comment •

edited

Loading

codecov-commenter commented Jul 30, 2025 •

edited

Loading