-
Notifications
You must be signed in to change notification settings - Fork 6
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
Conversation
- 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
…into issue-1604
Shelfmark should also be marked as required in the |
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? |
Yep. |
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 @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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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
There was a problem hiding this 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 :)
Looks good, but not actually ready to merge.
This was causing a TypeError on the source create and edit forms. Also adds the fix to the admin source create form. Fixes #1604