Mutual Checker: Sideblog Update #1603

AprilSylph · 2018-07-23T13:55:48Z

Uses messaging information by default to check if you're mutuals, which accurately reports single-member sideblogs as mutuals if the main blog follows you/your chosen sideblog (without actually exposing the URL of the main blog). Keeps the original method as a fallback for when the messaging data cannot be obtained, which happens if your chosen sideblog is a group blog.

cyle

looks good overall, left some random comments if you want em because i'm not super familiar with the overall style guide for the repo

cyle · 2018-10-09T04:03:05Z

Extensions/mutualchecker.js

-						XKit.extensions.mutualchecker.add_label($name_div, url);
-					}
-				});
+				var fake_obj = {


what's fake_obj ? could you use a more descriptive name than this or leave an inline comment?

cyle · 2018-10-09T04:04:17Z

Extensions/mutualchecker.js

-						XKit.extensions.mutualchecker.mutuals[json_obj.name] = false;
+				XKit.tools.Nx_XHR({
+					method: "GET",
+					url: "https://www.tumblr.com/svc/conversations/participant_info?q=" + json_obj.name + "&participant=" + this.preferences.main_blog.value,


do you write https://www.tumblr.com/ for every one of these URLs, or is that something you could bake into your XKit.tools.Nx_XHR() helper to reduce the redundancy?

cyle · 2018-10-09T04:04:45Z

Extensions/mutualchecker.js

+					method: "GET",
+					url: "https://www.tumblr.com/svc/conversations/participant_info?q=" + json_obj.name + "&participant=" + this.preferences.main_blog.value,
+					headers: {
+						"X-Requested-With": "XMLHttpRequest",


same Q here and for X-Tumblr-Form-Key below -- is this used every time? if so, can you bake it into XKit.tools.Nx_XHR() to reduce redundancy?

this is in fact on my to-do list, the entire function was kinda written in a huge hurry

cyle · 2018-10-09T04:06:39Z

Extensions/mutualchecker.js

@@ -100,6 +121,17 @@ XKit.extensions.mutualchecker = new Object({
 		}
 	},

+	plain_check: function(json_obj, $link) {


what does plain_check mean? could you add some inline documentation here to make this clearer? JSDocs can be super helpful to people trying to contribute to your code 😄

i would prefer a descriptive function name to inline documentation. group_blog_fallback is probably the best bet.

prefer a descriptive function name to inline documentation

100% that this is generally the best first step, but overall i think it's a great one-two punch for any shared codebase to complement descriptive feature names with detailed inline documentation. there are many times when you just can't make a function or variable name descriptive enough, like the always-around options object variable, which is kind of like the json_obj here.

sure, but in this case that's because the function is taking a opaque and poorly specified object when it's only using one of its variables—this should just be group_blog_fallback(username, $link). json_obj should probably be named something like popover_metadata, and we should explicitly destructure the variables we expect from it when we parse it from the DOM. (const {name, following, customizable} = JSON.parse(popover_metadata)). Inline documentation in this case would obscure the more fundamental encapsulation problem.

inline documentation is bad because either 1) you're making up for illegible code by half-heartedly explaining it, and any change is going to put your comment out of date, or 2) you're justifying design decisions that should be written up in a more long-form way and live in the pull request description or commit message—because they're also a record of a particular place and time and should be tied to their specific context, not living in the codebase forever.

nightpool

Overall i think 80% of the code needs to be moved to xkit_patches.

nightpool · 2018-10-09T12:49:36Z

Extensions/mutualchecker.js

@@ -100,6 +121,17 @@ XKit.extensions.mutualchecker = new Object({
 		}
 	},

+	plain_check: function(json_obj, $link) {


i would prefer a descriptive function name to inline documentation. group_blog_fallback is probably the best bet.

nightpool · 2018-10-09T12:55:11Z

Extensions/mutualchecker.js

-						XKit.extensions.mutualchecker.mutuals[json_obj.name] = false;
+				XKit.tools.Nx_XHR({
+					method: "GET",
+					url: "https://www.tumblr.com/svc/conversations/participant_info?q=" + json_obj.name + "&participant=" + this.preferences.main_blog.value,


this should be built using $.params instead of string-interpolated

nightpool · 2018-10-09T13:05:18Z

Extensions/mutualchecker.js

+						"X-Requested-With": "XMLHttpRequest",
+						"X-Tumblr-Form-Key": XKit.interface.form_key()
+					},
+					onerror: function(response) {


use promises, not callbacks.

nightpool · 2018-10-09T13:05:57Z

Extensions/mutualchecker.js

-						XKit.extensions.mutualchecker.mutuals[json_obj.name] = true;
-					} else {
-						XKit.extensions.mutualchecker.mutuals[json_obj.name] = false;
+				XKit.tools.Nx_XHR({


We should extract this whole request into is_following so that we can provide sideblog-correct results everywhere, it shouldn't just live in mutualchecker

nightpool · 2018-10-09T13:15:26Z

Extensions/mutualchecker.js

+								XKit.extensions.mutualchecker.plain_check(json_obj, $link);
+							} else {
+								var data = raw_data.response;
+								if (data.is_following_blog && data.is_blog_following_you) {


I get that having the additional information available is good, but now we have two distinct cases for when we'll add the mutual icon. Either we use this, which checks for mutual follow/following, or we use the fallback, in which case we're only checking for if they're following us, and we still have to be careful about where to call this function.

This entangling makes it hard to reason about this code path. We should either 1) drop the is_blog_following_you check here, 2) drop the fallback or 3) add an is_following check to the fallback that's going the other way, if that's possible.

mutual checker already only activates on username links with data-json that says you're following the blog, so the end result is the same. i just thought that since tumblr provides a double confirmation of it here, it might as well be used

nightpool · 2018-10-09T20:36:10Z

the logic should only live in one place. if we put the double check here, then it's very likely someone (probably future April) will come in somewhere down the line and go "oh, let's just remove this redundant data-json check" and break the fallback.

…

On Tue, Oct 9, 2018, 3:51 PM April Sylph ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Extensions/mutualchecker.js <#1603 (comment)>: > + url: "https://www.tumblr.com/svc/conversations/participant_info?q=" + json_obj.name + "&participant=" + this.preferences.main_blog.value, + headers: { + "X-Requested-With": "XMLHttpRequest", + "X-Tumblr-Form-Key": XKit.interface.form_key() + }, + onerror: function(response) { + XKit.extensions.mutualchecker.plain_check(json_obj, $link); + }, + onload: function(response) { + try { + var raw_data = JSON.parse(response.responseText); + if (raw_data.errors) { + XKit.extensions.mutualchecker.plain_check(json_obj, $link); + } else { + var data = raw_data.response; + if (data.is_following_blog && data.is_blog_following_you) { mutual checker already only activates on username links with data-json that says you're following the blog, so the end result is the same. i just thought that since tumblr provides a double confirmation of it here, it might as well be used — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1603 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAORV-CUjJG-B6vXkYXztMHiqJg-sC3Iks5ujP4pgaJpZM4VbClB> .

AprilSylph · 2018-10-10T05:04:13Z

the data-json check is never redundant since it's also used to never send unnecessary XHRs, and I would try to justify the double-check within the onload logic but if I'm going to be moving that to XKit.interface.is_following (which I am because that's a good idea) then it won't be used, and seeing as is_following is already a promise function I think that would satisfy all your concerns here

however I really really really do not want to touch XKit Patches before #1614 is dealt with properly, after which I would very much like to

maybe turn Nx_XHR into a promise function itself, and definitely implement standard headers as mentioned by cyle here
address your concerns with object iteration there, which also affects xkit main

[Mutual Checker] Check sideblogs

3f3b982

cyle reviewed Oct 9, 2018

View reviewed changes

nightpool requested changes Oct 9, 2018

View reviewed changes

AprilSylph mentioned this pull request Feb 22, 2019

XKit.svc #1663

Merged

AprilSylph closed this in #1663 Mar 13, 2019

AprilSylph deleted the feature/mutualchecker-sideblogs branch April 1, 2020 14:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mutual Checker: Sideblog Update #1603

Mutual Checker: Sideblog Update #1603

AprilSylph commented Jul 23, 2018

cyle left a comment

cyle Oct 9, 2018

cyle Oct 9, 2018

cyle Oct 9, 2018

AprilSylph Oct 9, 2018

cyle Oct 9, 2018

nightpool Oct 9, 2018

cyle Oct 12, 2018 •

edited

nightpool Oct 12, 2018

nightpool left a comment

nightpool Oct 9, 2018

nightpool Oct 9, 2018

nightpool Oct 9, 2018

nightpool Oct 9, 2018

nightpool Oct 9, 2018

AprilSylph Oct 9, 2018

nightpool commented Oct 9, 2018 via email

AprilSylph commented Oct 10, 2018

Mutual Checker: Sideblog Update #1603

Mutual Checker: Sideblog Update #1603

Conversation

AprilSylph commented Jul 23, 2018

cyle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyle Oct 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightpool left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightpool commented Oct 9, 2018 via email

AprilSylph commented Oct 10, 2018

cyle Oct 12, 2018 •

edited