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

Make shelfmark a required field on source create and edit forms #1606

Merged
merged 9 commits into from
Aug 30, 2024

Conversation

lucasmarchd01
Copy link
Contributor

This was causing a TypeError on the source create and edit forms. Also adds the fix to the admin source create form. Fixes #1604

- make shelfmark a required field on source create and edit pages
- make shelfmark a required field on source create and edit pages
- make shelfmark a required field on admin source add form
@ahankinson
Copy link
Member

Shelfmark should also be marked as required in the Source model. Also, holding_institution needs to be marked as required in both the editing form and the model.

@dchiller
Copy link
Contributor

Shelfmark should also be marked as required in the Source model. Also, holding_institution needs to be marked as required in both the editing form and the model.

Was the fact that these weren't previously required because null needed to be allowed while we transitioned the records? But the intention was that they really shouldn't allow null after that?

@ahankinson
Copy link
Member

Was the fact that these weren't previously required because null needed to be allowed while we transitioned the records?

Yep.

@dchiller
Copy link
Contributor

Great... I think it makes sense to make those changes, and then when we update production we just won't apply this migration, then run migrate_records, and then apply this last migration.

@lucasmarchd01 Do you want to add those changes here or we can create another issue?

@lucasmarchd01
Copy link
Contributor Author

@lucasmarchd01 Do you want to add those changes here or we can create another issue?

Sure, we can add those changes here.

@@ -57,7 +57,7 @@ <h3>Create Source</h3>
<div class="form-row">
<div class="form-group m-1 col-lg-3">
<label for="{{ form.shelfmark.id_for_label }}">
<small>Shelfmark:</small>
<small>Shelfmark:</small><span class="text-danger" title="This field is required">*</span></small>
Copy link
Member

Choose a reason for hiding this comment

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

See: #1550

Better would be to add the .small class to the <label> element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I've fixes the ones for these fields by using class='form-control-sm on the <label> element. But there's many more of these cases we'll want to fix in the future.

- in source model
- on the source create and edit forms
- holding institution and shelfmark fields are now required
- fixes incorrect use of <small> tag on these fields
@lucasmarchd01
Copy link
Contributor Author

There's a migration script that needs to be added, but I can generate that after #1615 is merged.

…eViewTest

- add holding institution required field
dchiller
dchiller previously approved these changes Aug 26, 2024
Copy link
Contributor

@dchiller dchiller left a comment

Choose a reason for hiding this comment

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

Cool. Good to merge imo with the migration added :)

@dchiller dchiller dismissed their stale review August 26, 2024 15:02

Looks good, but not actually ready to merge.

@lucasmarchd01 lucasmarchd01 merged commit 28657a2 into DDMAL:develop Aug 30, 2024
1 check passed
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.

TypeError on Source Create page
3 participants