Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added ability to notify and update url service about changes in related resources #10336

Merged
merged 18 commits into from
Jan 8, 2019
Merged

Added ability to notify and update url service about changes in related resources #10336

merged 18 commits into from
Jan 8, 2019

Conversation

naz
Copy link
Contributor

@naz naz commented Jan 4, 2019 •
edited
Loading

refs #10124

This PR solves one of the existing bugs in referenced issue described here.

sitemaps problem has it's root in url service not being able to distinguish that related resources were changed. We need a way to pass information to Resources.js to event's like _onResourceUpdated telling about relational data being changed e.g.: tag was removed

Still in progress:

  • cleaner handling of tag updates from post model
  • similar to tags handling of related post authors
  • handle related resource updates when post is removed completely
  • handle related resource updates when post is set to published

@naz naz changed the title [WIP] 🚧 Added ability to notify and update url service about changes in related resources [WIP] Added ability to notify and update url service about changes in related resources Jan 4, 2019
kirrg001
kirrg001 reviewed Jan 4, 2019
View reviewed changes
core/server/models/post.js Outdated Show resolved Hide resolved
@kirrg001
Copy link
Contributor

kirrg001 commented Jan 4, 2019

I think this will work 🤩GREAT

@naz naz force-pushed the url-service-related-resource-updates branch from 26ddf4d to 8e5f6bb Compare January 4, 2019 21:40
@naz naz force-pushed the url-service-related-resource-updates branch from e473730 to 9196b74 Compare January 5, 2019 00:23
kirrg001
kirrg001 reviewed Jan 5, 2019
View reviewed changes
core/server/models/post.js Outdated Show resolved Hide resolved
@naz naz force-pushed the url-service-related-resource-updates branch from 5921a0a to a1ede86 Compare January 5, 2019 12:37
@naz
Copy link
Contributor Author

naz commented Jan 5, 2019

One test needs to be fixed. Otherwise I think this PR is ready to go 🚀

@kirrg001
Copy link
Contributor

kirrg001 commented Jan 5, 2019

whoop whooooop

naz
naz commented Jan 7, 2019
View reviewed changes
core/test/unit/services/url/Resources_spec.js
@@ -108,13 +108,13 @@ describe('Unit: services/url/Resources', function () {

should.exist(resources.getByIdAndType(options.eventData.type, options.eventData.id));
obj.tags.length.should.eql(1);
Object.keys(obj.tags[0]).sort().should.eql(['id', 'slug'].sort());
Object.keys(obj.tags[0]).sort().should.eql(['id', 'post_id', 'slug', 'visibility'].sort());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

withRelatedFields doesn't contain these fields, will look into the code itself to confirm why they are being returned here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a little investigation, looks like there is a bug in the current implementation of raw_knex.fetchAll in the base model where these fields are not properly filtered on related tables. @kirrg001 I propose fixing this in a separate issue to be able to merge these changes sooner?

@naz naz force-pushed the url-service-related-resource-updates branch from c26da2c to 662f84d Compare January 7, 2019 12:56
@naz naz changed the title [WIP] Added ability to notify and update url service about changes in related resources Added ability to notify and update url service about changes in related resources Jan 7, 2019
@naz naz force-pushed the url-service-related-resource-updates branch 2 times, most recently from d8f42f3 to b791390 Compare January 7, 2019 14:24
@naz naz force-pushed the url-service-related-resource-updates branch from b791390 to 5861c28 Compare January 7, 2019 14:25
@naz naz force-pushed the url-service-related-resource-updates branch from bb709c3 to 5bcfdf5 Compare January 7, 2019 20:08
@naz
Removed part of URL service integration suite due to flakyness of tests
d0af733 
- Resource service is now making asynchronous call to the db which causes lots of trouble synchronizing these tests properly
@naz naz force-pushed the url-service-related-resource-updates branch from 5bcfdf5 to d0af733 Compare January 7, 2019 20:18
@naz
Copy link
Contributor Author

naz commented Jan 7, 2019

@kirrg001 some of the tests were removed/skipped due to timeouts for test cases with a lot of state setup. This is due to the introduction of a db call in Resources service. There is this disconnection between the model layer and resources/urls/sitemaps services because they communicate through event emission. This doesn't allow checking if all operations that given event triggered have finished.

This PR is needed for tomorrows release, so if you are back and well tomorrow morning, please give it another look 👍 Will do a bit more manual testing and self merge it if there are no objections.

@naz
Copy link
Contributor Author

naz commented Jan 8, 2019

Did some more manual testing this morning. Nothing obvious came up, so merging it in :)

@naz naz merged commit df1ba8a into TryGhost:master Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kirrg001 kirrg001 kirrg001 left review comments

Assignees

@kirrg001 kirrg001

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@naz @kirrg001