-
Notifications
You must be signed in to change notification settings - Fork 1
Add Responsibilities field for Person content type #307
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
base: main
Are you sure you want to change the base?
Conversation
davisagli
left a comment
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.
This is all minor stuff, this is close to being ready.
Also needs a new release of kitconcept.solr with the schema update.
|
|
||
| # responsibilities fieldset | ||
| model.fieldset( | ||
| "responsability_categorization", |
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.
Fix the spelling everywhere it's wrong (responsability -> responsibility)
| directives.omitted("responsibilities") | ||
| directives.no_omit(IEditForm, "responsibilities") | ||
| directives.no_omit(IAddForm, "responsibilities") |
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.
You don't need this part.
| directives.omitted("responsibilities") | |
| directives.no_omit(IEditForm, "responsibilities") | |
| directives.no_omit(IAddForm, "responsibilities") |
| <adapter | ||
| factory=".responsibilities.responsibilities_indexer" | ||
| name="responsibilities" | ||
| /> |
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.
You actually don't need responsibilities_indexer. By default the index will try to read from the attribute "responsibilities", and in this case that's good enough, because it's a tuple, which the KeywordIndex knows how to handle.
| "Reindex Responsibilities index for Person content", | ||
| BatchProcess.collect_via_catalog(portal_type="Person"), | ||
| reindex_responsibilities, | ||
| ).run() |
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.
You don't need custom Python code to reindex the new index, because when the index is first created in an upgraded site, there are no content items with values for this field. So reindexing will spend a bunch of time doing nothing.
It's enough to have an upgradeDepends step with the catalog import step.
| title="Add Responsibilities index for Person content type." | ||
| handler=".add_responsibilities_index" | ||
| /> | ||
| </genericsetup:upgradeSteps> |
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.
Replace this with an upgradeDepends with import_steps="catalog"
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.
Using abbreviations in filenames just makes them harder to read. Rename to test_indexer_responsibilities.py (or just test_responsibilities.py, since the folder already makes it clear it's about an indexer)
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.
test_vocab_responsibilities.py or test_responsibilities.py
| @@ -482,14 +482,14 @@ dependencies = [ | |||
| { name = "zope-security" }, | |||
| { name = "zope-testing" }, | |||
| ] | |||
| sdist = { url = "https://files.pythonhosted.org/packages/63/12/cfaae50dbd4e570847e19d2834235bcf449ab174f2ea5616cab0c7f009bd/accesscontrol-7.2.tar.gz", hash = "sha256:5a0e84b8b775a63826082a8697e75aad8131077d821d35ee7ce63f704a06ff43", size = 114439, upload-time = "2024-11-03T11:54:12.772Z" } | |||
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.
The upload-time should not be removed here. Make sure you have an up-to-date version of uv.
|
|
||
| ## Searchable Index | ||
|
|
||
| Responsibilities are indexed in the catalog, allowing users to search for persons by their responsibilities. |
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.
Really? I don't see configuration for a querystring criterion for this field (so that it can be selected in the search/listing block).
https://gitlab.kitconcept.io/kitconcept/distribution-kitconcept-intranet/-/issues/109