Involved authorities #345
Involved authorities #345
Conversation
…et_popular_translators, Person.get_popular_xlat_authors, Person.get_popular_xlat_authors_by_genre, ApplicationController.cached_popular_authors_by_genre, ApplicationController.popups_by_genre, ApplicationController.count_authors_by_genre
Removed unused method Person.cached_translators_count
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
damisul
force-pushed
the
involved_authorities
branch
from
eed54f1
to
4859c71
Compare
damisul
force-pushed
the
involved_authorities
branch
from
4859c71
to
7033cae
Compare
| @@ -25,12 +26,8 @@ class Expression < ApplicationRecord | |||
|
|
|||
| validates :intellectual_property, presence: true | |||
|
|
|||
| def editors | |||
There was a problem hiding this comment.
This method is dangerous, as now editors can be declared both on Expression and Work level.
So we need to use Manifestation.involved_authorities_by_role instead.
There was a problem hiding this comment.
I don't understand: why can't we return the editors associated with the Expression here? How would calling it on the Manifestation disambiguate? It seems, on the contrary, that on the Manifestation we would have ambiguity about Work-level editors and Expression-level editors.
There was a problem hiding this comment.
Well... Probably I used wrong wording.
I wrote this comment while thinking about: https://github.com/abartov/bybeconv/blob/5f265048d8286dfc8fef6cf429feb7d98f6ab50a/app/controllers/manifestation_controller.rb#L845C1-L847C1
I believe in that case we should fetch illustrators and editors both from Work and Expression. So I've added Manifestation.involved_authorities_by_role method for that puropse which merges values from Work and Expression.
And there is an involved_authorities_by_role method in Work and Expression too if we need value from specific level.
| @@ -0,0 +1,17 @@ | |||
| %h3= t(:associated_persons) | |||
There was a problem hiding this comment.
I've created this partial to replaces old shared/associated_people partial to reduce code duplication
app/api/v1/entities/person.rb
Outdated
| expose :texts, | ||
| documentation: { desc: 'ID numbers of all texts this person is involved into with with role in each' }, | ||
| if: ->(_person, options) { %w(texts enriched).include?(options[:detail]) } do | ||
| InvolvedAuthority.roles.each_key do |role| |
There was a problem hiding this comment.
Exposing all available roles including newly added
| Manifestation.all_published.joins(expression: [work: :creations]).where("creations.person_id = #{self.id}") | ||
| Manifestation.all_published | ||
| .joins(expression: [work: :involved_authorities]) | ||
| .where(involved_authorities: { person_id: id }) |
There was a problem hiding this comment.
wouldn't this include translations, too? This is supposed to return only Manifestations where this person is NOT the translator.
There was a problem hiding this comment.
Well... I believe this will not include translators, because translator can only be specified on Expression level (we have validation for this). But this will include all works where Person has any role possible on Work level (e.g. illustrator).
I wonder if we should limit it to return only works where person is an author? In this case I'd propose to remove this method at all and use new Person.published_manifestations(:author) method instead. @abartov WDYT?
| end | ||
|
|
||
| def original_work_count_including_unpublished | ||
| Manifestation.joins(expression: { work: :creations }).merge(Creation.where(person_id: self.id)).count | ||
| Manifestation.joins(expression: { work: :involved_authorities }) | ||
| .merge(InvolvedAuthority.where(person_id: id)).count |
| Manifestation.all_published | ||
| .joins(expression: [work: :involved_authorities]) | ||
| .includes(:expression) | ||
| .merge(InvolvedAuthority.where(person_id: id)).each do |m| |
| latest_original_works = Manifestation.all_published | ||
| .joins(expression: [work: :involved_authorities]) | ||
| .where(involved_authorities: { person_id: id }) | ||
| .order(created_at: :desc).limit(20) |
This PR introduces new InvolvedAuthorities table to replace Creations and Realizers table.
It does not intoduces Authorities table yet, so InvolvedAuthority record can only refer to person.
Also I did not used polymorphic relation for item and added expression_id and work_id as separate columns instead.
I believe it is better from performance point of view and more SQL-friendly approach.
Also this PR contains following changes not directly related to InvolvedAuthorities: