-
Notifications
You must be signed in to change notification settings - Fork 1
COR-667: Media Assets Not Updating #50
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
Conversation
app/models/asset_field_type.rb
Outdated
| end | ||
| end | ||
|
|
||
| def asset_field_type_id |
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.
What's this used for?
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.
Originally before we replaced id's with parameterized Titles updated assets were storing new images on updates since the AssetFieldType id would be different each time. This caused a few issues beyond just storing a new set of images on each update.
But it's purpose is less important now that we use Asset Title instead of ID.
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'd say we probably don't need it anymore, since we're using the Media Title. You can go ahead and remove these bits. Thanks!
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.
Ok sounds good, but this branch wont work on it's own without asset_field_type_id, but it will when merged with the Media Title story
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'd suggest merging in the Media Title story, then rebasing this branch with develop. Also, would you mind updating the PR title to follow guidelines?
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.
Ok will do
6a9fad7 to
3a99531
Compare
|
@toastercup ok merged with the media title branch and removed |
|
@MKwenhua What is the status of this PR? There's been no activity for the last month |
|
@toastercup @MKwenhua |
|
@ElliottAYoung once #57 is merged and verified by a PO, we can close this and delete its branch |
Purpose: Since the media title will be replacing id for asset urls most of the "not updating" issues have been resolved. But there was still a risk of someone updating an asset with a different extension meaning posts that use the asset will not update.
I cover this in detail in a Cortex issue: cortex/issues/456
JIRA:
COR-667
Changes:
Changes to setup
Architectural changes
add a validation to check to see if the new asset matches the
content_typeof the previous assetMigrations
Library changes
Side effects
Screenshots
Before
n/a
After
n/a
QA Links:
n/a
How to Verify These Changes
Specific pages to visit
Steps to take
Responsive considerations
Relevant PRs/Dependencies:
n/a
Additional Information
cortex/issues/456