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.

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

Fix follow IDs #455

Merged
merged 4 commits into from
Mar 4, 2019
Merged

Fix follow IDs #455

merged 4 commits into from
Mar 4, 2019

Conversation

elegaanz
Copy link
Member

I changed the syntax of the insert! macro, allowing to modify an object just after it was saved to the database. You can now optional pass it a "closure" (actually it just looks like a closure) that will be run just after the insertion.

I updated all the models, including Follow, to use it to generate AP ID and URL. The advantages are that the ID/URL are now correct (fixes #449) and that we don't have methods like update_boxes or update_ap_url to call manually and that were easy to forget.

@elegaanz elegaanz added C: Bug Something isn't working C: Enhancement New feature or request A: Federation Stuff related to Federation S: Ready for review This PR is ready to be reviewed A: Backend Code running on the server labels Feb 27, 2019
@@ -227,7 +227,6 @@ pub fn update(blog: String, slug: String, user: User, cl: ContentLen, form: Leni
post.license = form.license.clone();
post.cover_id = form.cover;
post.update(&*conn, &searcher).expect("post::update: update error");;
let post = post.update_ap_url(&*conn).expect("post::update: update ap url error");
Copy link
Member Author

Choose a reason for hiding this comment

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

Just a note: I removed that without replacing it with something else, since it is incorrect (the AP IDs should change), and was not working anyway (since we were checking if ap_url was empty before updating it, which was only the case on newly created articles).

@codecov
Copy link

codecov bot commented Mar 4, 2019

Codecov Report

Merging #455 into master will increase coverage by 1.41%.
The diff coverage is 79.31%.

@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
+ Coverage   25.06%   26.48%   +1.41%     
==========================================
  Files          64       64              
  Lines        6316     7111     +795     
==========================================
+ Hits         1583     1883     +300     
- Misses       4733     5228     +495

Copy link
Contributor

@trinity-1686a trinity-1686a left a comment

Choose a reason for hiding this comment

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

LGTM

plume-models/src/blogs.rs Show resolved Hide resolved
insert!(follows, NewFollow);
insert!(follows, NewFollow, |inserted, conn| {
if inserted.ap_url.is_empty() {
inserted.ap_url = ap_url(&format!("{}/follows/{}", *BASE_URL, inserted.id));
Copy link
Contributor

Choose a reason for hiding this comment

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

this route should really get implemented one day

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think we already have an issue for it…

@elegaanz elegaanz merged commit 2a188ab into master Mar 4, 2019
@elegaanz elegaanz deleted the fix-follow-id branch March 4, 2019 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: Backend Code running on the server A: Federation Stuff related to Federation C: Bug Something isn't working C: Enhancement New feature or request S: Ready for review This PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange urls in follows.ap_url column
3 participants