Ignore images unless changed, default segment count when nil #1170
Conversation
| validates :width, comparison: {equal_to: :height}, if: :should_validate? | ||
|
|
||
| def should_validate? | ||
| status_complete? && (height_changed? || width_changed?) |
There was a problem hiding this comment.
Any case for moving this to the shared ImageFile? ITunesImage has similar validations, and there's a :format validation as well.
I see 1829 invalid episode images in prod ... as recently created as Feb 2023. Bit surprised by that.
There was a problem hiding this comment.
image format on this one wasn't an issue, just the dimensions; probably we enforced a more lax value in the past.
There was a problem hiding this comment.
Just checked prod, and I see 1829 invalid EpisodeImages. But 0 ITunesImages. Since this isn't a problem there, probably don't need the validation in both models.
| if new_record? && strict_validations | ||
| self.segment_count ||= 1 | ||
| elsif segment_count.blank? && contents.any? | ||
| self.segment_count = contents.map(&:position).max || contents.size |
There was a problem hiding this comment.
This makes me a bit more nervous. There's already logic in the episode media to set this field, and to handle nil segment_counts (I've definitely seen those in the wild).
Was this also an issue with the ep referenced in the ticket?
There was a problem hiding this comment.
yes, as I recall it had a 0 segment count, even though it had audio
There was a problem hiding this comment.
Remembering now - nil segment counts either come in from RSS imports (items with no media), or API users.
We were keeping these nil for API users. So when they PUT an update, they don't need to send both media: [...three urls...] and a matching segment_count: 3.
We could change media= to also set the segment count? That setter is used in 3 places:
- The API (we'd just have to decide
segment_countrelated validations can't be used by API users, which is maybe fine) - The
EpisodeRssImport, but it's immediately setting the segment count after that. So changing would be fine. Uncut.slice_contents, which i can't imagine thesegmentationnot matching the segment count anyways right? Or we could build contents in a different way there, which would maybe be better.
Or we don't force these to be set here. (The UI handles nils already).
|
I'm also wondering if this relates to #1040. Where an import creates an invalid image, but the UI wasn't telling me it was invalid. |
|
Retesting some things - I'm not sure this would ever be an issue with recent data? (Since the introduction of `image.status == "invalid"). These days, the porter callbacks would set the image.status to invalid, so the validation wouldn't run in the first place. |
| if new_record? && strict_validations | ||
| self.segment_count ||= 1 | ||
| elsif segment_count.blank? && contents.any? | ||
| self.segment_count = contents.map(&:position).max || contents.size |
There was a problem hiding this comment.
Remembering now - nil segment counts either come in from RSS imports (items with no media), or API users.
We were keeping these nil for API users. So when they PUT an update, they don't need to send both media: [...three urls...] and a matching segment_count: 3.
We could change media= to also set the segment count? That setter is used in 3 places:
- The API (we'd just have to decide
segment_countrelated validations can't be used by API users, which is maybe fine) - The
EpisodeRssImport, but it's immediately setting the segment count after that. So changing would be fine. Uncut.slice_contents, which i can't imagine thesegmentationnot matching the segment count anyways right? Or we could build contents in a different way there, which would maybe be better.
Or we don't force these to be set here. (The UI handles nils already).
| validates :width, comparison: {equal_to: :height}, if: :should_validate? | ||
|
|
||
| def should_validate? | ||
| status_complete? && (height_changed? || width_changed?) |
There was a problem hiding this comment.
Just checked prod, and I see 1829 invalid EpisodeImages. But 0 ITunesImages. Since this isn't a problem there, probably don't need the validation in both models.
|
looks like this might cause problems with API calls (spoolr) that are not setting segments - do not merge quite yet! |
fix issues with update errors for older Mortified issues -
https://prx.zendesk.com/agent/tickets/204696