From 87b1561580cd1c7f54fe368913015aacb4f5a954 Mon Sep 17 00:00:00 2001 From: "Patrick W. Crawford" Date: Fri, 6 Oct 2023 21:08:35 -0700 Subject: [PATCH 1/6] Adopting py 3.7+ syntax and removing most lint warnings and errors Some can't be avoided, such as those around bpy property definitions. --- MCprep_addon/tracking.py | 229 +++++++++++++++++++++------------------ 1 file changed, 125 insertions(+), 104 deletions(-) diff --git a/MCprep_addon/tracking.py b/MCprep_addon/tracking.py index d3020d4c..9c2fd7b9 100644 --- a/MCprep_addon/tracking.py +++ b/MCprep_addon/tracking.py @@ -16,6 +16,11 @@ # # ##### END GPL LICENSE BLOCK ##### +# critical to at least load these +import os +import bpy + + # safe importing, due to rogue python libraries being missing or invalid # in bad installs cases (if the rest of the addon works, don't make it # fail to work because of this module) @@ -24,13 +29,9 @@ # Request timeout (total request time may still be longer) TIMEOUT = 60 -# critical to at least load these -import os -import bpy # remaining, wrap in safe-importing try: - from . import conf from . import util from .addon_updater import Updater as updater import re @@ -46,7 +47,7 @@ from .conf import env except Exception as err: print("[MCPREP Error] Failed tracker module load, invalid import module:") - print('\t'+str(err)) + print(f"\t{err}") VALID_IMPORT = False @@ -58,9 +59,10 @@ # max data/string lengths to match server-side validation, # if exceeded, request will return denied (no data written) -SHORT_FIELD_MAX = 64-1 -USER_COMMENT_LENGTH = 512-1 -ERROR_STRING_LENGTH = 1024-1 +SHORT_FIELD_MAX = 64 - 1 +USER_COMMENT_LENGTH = 512 - 1 +ERROR_STRING_LENGTH = 1024 - 1 + # ----------------------------------------------------------------------------- # primary class implementation @@ -87,9 +89,9 @@ def __init__(self): self._verbose = False self._version = "" self.json = {} - self._httpclient_fallback = None # set to True/False after first req - self._last_request = {} # used to debounce sequential requests - self._debounce = 5 # seconds to avoid duplicative requests + self._httpclient_fallback = None # set to True/False after first req + self._last_request = {} # used to debounce sequential requests + self._debounce = 5 # seconds to avoid duplicative requests self._feature_set = 0 # Supported addon features (e.g. experimental) # ------------------------------------------------------------------------- @@ -99,11 +101,12 @@ def __init__(self): @property def blender_version(self): return self._blender_version + @blender_version.setter def blender_version(self, value): if value is None: self._blender_version = "unknown" - elif type(value) != type((1,2,3)): + elif not isinstance(value, tuple): raise Exception("blender_version must be a tuple") else: self._blender_version = str(value) @@ -111,18 +114,20 @@ def blender_version(self, value): @property def language(self): return self._language + @language.setter def language(self, value): if value is None: self._language = "None" - elif type(value) != type("string"): - raise Exception("language must be a string") + elif not isinstance(value, str): + raise Exception(f"language must be a string: {value}, {type(value)}") else: self._language = self.string_trunc(value) @property def tracking_enabled(self): return self._tracking_enabled + @tracking_enabled.setter def tracking_enabled(self, value): self._tracking_enabled = bool(value) @@ -131,16 +136,18 @@ def tracking_enabled(self, value): @property def verbose(self): return self._verbose + @verbose.setter def verbose(self, value): try: self._verbose = bool(value) - except: - raise ValueError("Verbose must be a boolean value") + except Exception as e: + raise ValueError("Verbose must be a boolean value") from e @property def appurl(self): return self._appurl + @appurl.setter def appurl(self, value): if value[-1] == "/": @@ -150,36 +157,40 @@ def appurl(self, value): @property def failsafe(self): return self._failsafe + @failsafe.setter def failsafe(self, value): try: self._failsafe = bool(value) - except: - raise ValueError("failsafe must be a boolean value") + except Exception as e: + raise ValueError("failsafe must be a boolean value") from e @property def dev(self): return self._dev + @dev.setter def dev(self, value): try: self._dev = bool(value) - except: - raise ValueError("background must be a boolean value") + except Exception as e: + raise ValueError("background must be a boolean value") from e @property def background(self): return self._background + @background.setter def background(self, value): try: self._background = bool(value) - except: - raise ValueError("background must be a boolean value") + except Exception as e: + raise ValueError("background must be a boolean value") from e @property def version(self): return self._version + @version.setter def version(self, value): self._version = value @@ -187,6 +198,7 @@ def version(self, value): @property def addon(self): return self._addon + @addon.setter def addon(self, value): self._addon = value @@ -194,11 +206,12 @@ def addon(self, value): @property def platform(self): return self._platform + @platform.setter def platform(self, value): if value is None: self._platform = "None" - elif type(value) != type("string"): + elif not isinstance(value, str): raise Exception("platform must be a string") else: self._platform = self.string_trunc(value) @@ -207,6 +220,7 @@ def platform(self, value): def feature_set(self): values = ["", "supported", "experimental"] return values[self._feature_set] + @feature_set.setter def feature_set(self, value): if not value: @@ -240,9 +254,9 @@ def enable_tracking(self, toggle=True, enable=True): def get_platform_details(self): """Return OS related information.""" try: - res = platform.system()+":"+platform.release() + res = f"{platform.system()}:{platform.release()}" if len(res) > SHORT_FIELD_MAX: - res = res[:SHORT_FIELD_MAX-1] + "|" + res = res[:SHORT_FIELD_MAX - 1] + "|" except Exception as err: print("Error getting platform info: " + str(err)) return "unknown:unknown" @@ -260,10 +274,13 @@ def initialize(self, appurl, version, language=None, blender_version=None): self._blender_version = str(blender_version) self.language = language - self._tracker_idbackup = os.path.join(os.path.dirname(__file__), - os.pardir,self._addon+"_trackerid.json") - self._tracker_json = os.path.join(os.path.dirname(__file__), - self._addon+"_tracker.json") + self._tracker_idbackup = os.path.join( + os.path.dirname(__file__), + os.pardir, + self._addon + "_trackerid.json") + self._tracker_json = os.path.join( + os.path.dirname(__file__), + self._addon + "_tracker.json") # create the local file # push into BG push update info if true @@ -282,13 +299,12 @@ def request(self, method, path, payload, background=False, callback=None): return self.raw_request(method, path, payload, callback) else: # if self._verbose: print("Starting background thread for track call") - bg_thread = threading.Thread(target=self.raw_request, args=(method, path, payload, callback)) + bg_thread = threading.Thread( + target=self.raw_request, args=(method, path, payload, callback)) bg_thread.daemon = True - #self._bg_threads.append(bg_thread) bg_thread.start() return "Thread launched" - def raw_request(self, method, path, payload, callback=None): """Raw connection request, background or foreground. @@ -304,7 +320,7 @@ def raw_request(self, method, path, payload, callback=None): try: resp = self._raw_request_mod_requests(method, path, payload) self._httpclient_fallback = False - except: + except Exception: resp = self._raw_request_mod_http(method, path, payload) self._httpclient_fallback = True elif self._httpclient_fallback is False: @@ -314,7 +330,7 @@ def raw_request(self, method, path, payload, callback=None): if callback is not None: if self._verbose: - print(self._addon+": Running callback") + print(self._addon + ": Running callback") callback(resp) return resp @@ -360,11 +376,11 @@ def _raw_request_mod_http(self, method, path, payload): try: connection.connect() if self.verbose: - print(self._addon + ": Connection made to "+str(path)) + print(f"{self._addon}: Connection made to {path}") except Exception as err: - print(self._addon + ": Connection failed, intended report destination: "+str(path)) - print("Error: "+str(err)) - return {'status':'NO_CONNECTION'} + print(f"{self._addon}: Connection failed to: {path}") + print(f"Error: {err}") + return {'status': 'NO_CONNECTION'} if method in ("POST", "PUT"): connection.request(method, path, payload) @@ -390,22 +406,22 @@ def set_tracker_json(self): with open(self._tracker_json) as data_file: self.json = json.load(data_file) if self._verbose: - print(self._addon+": Read in json settings from tracker file") + print(f"{self._addon}: Read in json settings from tracker file") else: # set data structure self.json = { - "install_date":None, - "install_id":None, - "enable_tracking":False, - "status":None, - "metadata":{} + "install_date": None, + "install_id": None, + "enable_tracking": False, + "status": None, + "metadata": {} } if os.path.isfile(self._tracker_idbackup): with open(self._tracker_idbackup) as data_file: idbackup = json.load(data_file) if self._verbose: - print(self._addon+": Reinstall, getting IDNAME") + print(f"{self._addon}: Reinstall, getting IDNAME") if "IDNAME" in idbackup: self.json["install_id"] = idbackup["IDNAME"] self.json["status"] = "re-install" @@ -426,7 +442,7 @@ def save_tracker_json(self): outf.write(data_out) outf.close() if self._verbose: - print(self._addon+": Wrote out json settings to file") + print(f"{self._addon}: Wrote out json settings to file") def save_tracker_idbackup(self): """Save copy of the ID file to parent folder location, for detecting @@ -435,15 +451,17 @@ def save_tracker_idbackup(self): return jpath = self._tracker_idbackup - if "install_id" in self.json and self.json["install_id"] != None: + if "install_id" in self.json and self.json["install_id"] is not None: outf = open(jpath, 'w') - idbackup = {"IDNAME":self.json["install_id"], - "date":self.json["install_date"]} - data_out = json.dumps(idbackup,indent=4) + idbackup = { + "IDNAME": self.json["install_id"], + "date": self.json["install_date"]} + data_out = json.dumps(idbackup, indent=4) outf.write(data_out) outf.close() if self._verbose: - print(self._addon+": Wrote out backup settings to file, with the contents:") + print( + f"{self._addon}: Wrote out backup settings to file, with the contents:") print(idbackup) def remove_indentifiable_information(self, report): @@ -460,12 +478,12 @@ def remove_indentifiable_information(self, report): str(report)) except Exception as err: print("Error occured while removing info: {}".format(err)) - return "[pii] "+str(report) + return f"[pii] {report}" def string_trunc(self, value): """Function which caps max string length.""" ret = str(value) - if len(ret)>SHORT_FIELD_MAX: + if len(ret) > SHORT_FIELD_MAX: ret = ret[:SHORT_FIELD_MAX] return ret @@ -494,9 +512,10 @@ class TRACK_OT_toggle_enable_tracking(bpy.types.Operator): options = {'REGISTER', 'UNDO'} tracking: bpy.props.EnumProperty( - items=[('toggle', 'Toggle', 'Toggle operator use tracking'), - ('enable', 'Enable', 'Enable operator use tracking'), - ('disable', 'Disable', 'Disable operator use tracking (if already on)')], + items=[ + ('toggle', 'Toggle', 'Toggle operator use tracking'), + ('enable', 'Enable', 'Enable operator use tracking'), + ('disable', 'Disable', 'Disable operator use tracking (if already on)')], name="tracking") def execute(self, context): @@ -513,8 +532,8 @@ def execute(self, context): class TRACK_OT_popup_feedback(bpy.types.Operator): - bl_idname = IDNAME+".popup_feedback" - bl_label = "Thanks for using {}!".format(IDNAME) + bl_idname = f"{IDNAME}.popup_feedback" + bl_label = f"Thanks for using {IDNAME}!" bl_description = "Take a survey to give feedback about the addon" options = {'REGISTER', 'UNDO'} @@ -526,7 +545,7 @@ def draw(self, context): self.layout.split() col = self.layout.column() - col.alignment='CENTER' + col.alignment = 'CENTER' col.scale_y = 0.7 col.label(text="Want to help out even more?") col.label(text="Press OK below to open the MCprep survey") @@ -541,7 +560,7 @@ def execute(self, context): class TRACK_OT_popup_report_error(bpy.types.Operator): - bl_idname = IDNAME+".report_error" + bl_idname = f"{IDNAME}.report_error" bl_label = "MCprep Error, press OK below to send this report to developers" bl_description = "Report error to database, add additional comments for context" @@ -552,16 +571,17 @@ class TRACK_OT_popup_report_error(bpy.types.Operator): options={'SKIP_SAVE'}) action: bpy.props.EnumProperty( - items = [('report', 'Send', 'Send the error report to developers, fully anonymous'), - ('ignore', "Don't send", "Ignore this error report")], - ) + items=[ + ('report', 'Send', 'Send the error report to developers, fully anonymous'), + ('ignore', "Don't send", "Ignore this error report")], + ) def invoke(self, context, event): width = 500 * util.ui_scale() return context.window_manager.invoke_props_dialog(self, width=width) def draw_header(self, context): - self.layout.label(text="", icon="ERROR") # doesn't work/add to draw + self.layout.label(text="", icon="ERROR") # doesn't work/add to draw def draw(self, context): layout = self.layout @@ -570,7 +590,7 @@ def draw(self, context): box = col.box() boxcol = box.column() boxcol.scale_y = 0.7 - if self.error_report=="": + if self.error_report == "": box.label(text=" # no error code identified # ") else: width = 500 @@ -578,26 +598,27 @@ def draw(self, context): tot_ln = 0 max_ln = 10 for ln in report_lines: - sub_lns = textwrap.fill(ln, width-30) + sub_lns = textwrap.fill(ln, width - 30) spl = sub_lns.split("\n") - for i,s in enumerate(spl): + for i, s in enumerate(spl): boxcol.label(text=s) - tot_ln+=1 - if tot_ln==max_ln: break - if tot_ln==max_ln: break - boxcol.label(text="."*500) + tot_ln += 1 + if tot_ln == max_ln: + break + if tot_ln == max_ln: + break + boxcol.label(text="." * 500) # boxcol.label(text="System & addon information:") - sysinfo="Blender version: {}\nMCprep version: {}\nOS: {}\n MCprep install identifier: {}".format( - Tracker.blender_version, - Tracker.version, - Tracker.platform, - Tracker.json["install_id"], - ) + sysinfo = ( + f"Blender version: {Tracker.blender_version}\n" + f"MCprep version: {Tracker.version}\n" + f"OS: {Tracker.platform}\n" + f"MCprep install identifier: {Tracker.json['install_id']}") for ln in sysinfo.split("\n"): boxcol.label(text=ln) col.label(text="(Optional) Describe what you were trying to do when the error occured:") - col.prop(self,"comment",text="") + col.prop(self, "comment", text="") row = col.row(align=True) split = layout_split(layout, factor=0.6) @@ -605,9 +626,10 @@ def draw(self, context): spcol.label(text="Select 'Send' then press OK to share report") split_two = layout_split(split, factor=0.4) spcol_two = split_two.row(align=True) - spcol_two.prop(self,"action",text="") + spcol_two.prop(self, "action", text="") spcol_two = split_two.row(align=True) - p = spcol_two.operator("wm.url_open", text="How is this used?", icon="QUESTION") + p = spcol_two.operator( + "wm.url_open", text="How is this used?", icon="QUESTION") p.url = "https://theduckcow.com/dev/blender/mcprep/reporting-errors/" def execute(self, context): @@ -615,7 +637,6 @@ def execute(self, context): if bpy.app.background: env.log("Skip Report logging, running headless") env.log("Would have reported:") - #env.log(self.error_report) raise Exception(self.error_report) return {'CANCELLED'} @@ -623,16 +644,16 @@ def execute(self, context): self.report({"ERROR"}, "Invalid import, all reporting disabled.") return {'CANCELLED'} - if self.action=="ignore": + if self.action == "ignore": return {"FINISHED"} # also do followup callback for hey, check here for issues or support? - report = {"error":self.error_report,"user_comment":self.comment} + report = {"error": self.error_report, "user_comment": self.comment} if self.action == 'report': res = logError(report) if Tracker.verbose: print("Logged user report, with server response:") print(res) - self.report({"INFO"},"Thanks for sharing the report") + self.report({"INFO"}, "Thanks for sharing the report") return {'FINISHED'} @@ -702,7 +723,7 @@ def callback(arg): if Tracker.failsafe is True: try: runInstall(background) - except: + except Exception: pass else: runInstall(background) @@ -746,7 +767,7 @@ def runUsage(background): if Tracker.failsafe is True: try: runUsage(background) - except: + except Exception: pass else: runUsage(background) @@ -808,7 +829,7 @@ def runError(background): if Tracker.failsafe is True: try: runError(background) - except: + except Exception: pass else: runError(background) @@ -828,7 +849,7 @@ def wrapper(self, context): Tracker._handling_error = False res = function(self, context) Tracker._handling_error = False - except: + except Exception: err = traceback.format_exc() print(err) # Always print raw traceback. @@ -847,8 +868,8 @@ def wrapper(self, context): if err[ind] in ["\n", "\r"]: nth_newline += 1 if nth_newline == 3: - if len(err) > ind+1: - err = err[ind+1:] + if len(err) > ind + 1: + err = err[ind + 1:] break # Prevent recusrive popups for errors if operators call other @@ -858,7 +879,7 @@ def wrapper(self, context): bpy.ops.mcprep.report_error('INVOKE_DEFAULT', error_report=err) return {'CANCELLED'} - if res=={'CANCELLED'}: + if res == {'CANCELLED'}: return res # cancelled, so skip running usage elif hasattr(self, "skipUsage") and self.skipUsage is True: return res # skip running usage @@ -868,9 +889,9 @@ def wrapper(self, context): try: wrapper_safe_handler(self) - except: + except Exception: err = traceback.format_exc() - print("Error while reporting usage for "+str(self.track_function)) + print(f"Error while reporting usage for {self.track_function}") print(err) return res @@ -902,9 +923,9 @@ def wrapper_safe_handler(self): try: trackUsage(self.track_function, param=param, exporter=exporter) - except: + except Exception: err = traceback.format_exc() - print("Error while reporting usage for "+str(self.track_function)) + print(f"Error while reporting usage for {self.track_function}") print(err) # always update the last request gone through @@ -969,10 +990,10 @@ def register(bl_info): try: Tracker.initialize( - appurl = "https://mcprep-1aa04.firebaseio.com/", - version = str(bl_info["version"]), - language = language, - blender_version = bversion) + appurl="https://mcprep-1aa04.firebaseio.com/", + version=str(bl_info["version"]), + language=language, + blender_version=bversion) except Exception as err: err = traceback.format_exc() print(err) @@ -980,20 +1001,20 @@ def register(bl_info): # used to define which server source, not just if's below if VALID_IMPORT: - Tracker.dev = env.dev_build # True or False + Tracker.dev = env.dev_build # True or False else: Tracker.dev = False if Tracker.dev is True: Tracker.verbose = True - Tracker.background = True # test either way - Tracker.failsafe = False # test either way - Tracker.tracking_enabled = True # enabled automatically for testing + Tracker.background = True # test either way + Tracker.failsafe = False # test either way + Tracker.tracking_enabled = True # enabled automatically for testing else: Tracker.verbose = False Tracker.background = True Tracker.failsafe = True - Tracker.tracking_enabled = True # User accepted on download + Tracker.tracking_enabled = True # User accepted on download for cls in classes: bpy.utils.register_class(cls) From c3f090b7e874fd63bf50b832fd90453784439a29 Mon Sep 17 00:00:00 2001 From: "Patrick W. Crawford" Date: Fri, 6 Oct 2023 22:47:25 -0700 Subject: [PATCH 2/6] Proactively identify bad analytics ID and replace with new one Addresses an issue introduced in MCprep v3.5.0 --- MCprep_addon/tracking.py | 73 +++++++++++++++++++++++++++++----------- 1 file changed, 53 insertions(+), 20 deletions(-) diff --git a/MCprep_addon/tracking.py b/MCprep_addon/tracking.py index 9c2fd7b9..6242a710 100644 --- a/MCprep_addon/tracking.py +++ b/MCprep_addon/tracking.py @@ -29,6 +29,11 @@ # Request timeout (total request time may still be longer) TIMEOUT = 60 +# Ids with known analytical problems, such as clobbered-together installs. +# If we see any of these IDs in local json files, still treat as a re-install +# but replace the ID with the new one received. +SKIP_IDS = ["-Nb8TgbvAoxHrnEe1WFy"] + # remaining, wrap in safe-importing try: @@ -402,35 +407,63 @@ def set_tracker_json(self): if self._tracker_json is None: raise ValueError("tracker_json is not defined") + # Placeholder struc before saving to class + _json = { + "install_date": None, + "install_id": None, + "enable_tracking": False, + "status": None, + "metadata": {} + } + + valid_tracker = False if os.path.isfile(self._tracker_json) is True: with open(self._tracker_json) as data_file: - self.json = json.load(data_file) + jdata = json.load(data_file) + + _json = jdata + if self._verbose: + print(f"{self._addon}: Read in json settings from tracker file") + if jdata.get("install_id") in SKIP_IDS: + valid_tracker = False + _json["install_id"] = None + _json["status"] = "invalid_id" if self._verbose: - print(f"{self._addon}: Read in json settings from tracker file") - else: - # set data structure - self.json = { - "install_date": None, - "install_id": None, - "enable_tracking": False, - "status": None, - "metadata": {} - } + print(f"{self._addon}: Skip ID detected, treat as new") + else: + valid_tracker = True + + if not valid_tracker: if os.path.isfile(self._tracker_idbackup): with open(self._tracker_idbackup) as data_file: idbackup = json.load(data_file) + + bu_id = idbackup.get("IDNAME") + if bu_id in SKIP_IDS: + if self._verbose: + print(f"{self._addon}: Skipping blocked ID list") + + # Still count as a reinstall + _json["status"] = "re-install" + _json["install_date"] = idbackup["date"] + + elif bu_id is not None: + print("Detected this scenario???") if self._verbose: print(f"{self._addon}: Reinstall, getting IDNAME") - if "IDNAME" in idbackup: - self.json["install_id"] = idbackup["IDNAME"] - self.json["status"] = "re-install" - self.json["install_date"] = idbackup["date"] + _json["install_id"] = idbackup["IDNAME"] + _json["status"] = "re-install" + _json["install_date"] = idbackup["date"] + self.json = _json self.save_tracker_json() + else: + # Tracker was valid, so just load it. + self.json = _json - # update any other properties if necessary from json - self._tracking_enabled = self.json["enable_tracking"] + # Update any other properties if necessary from json + self._tracking_enabled = self.json.get("enable_tracking", False) def save_tracker_json(self): """Save out current state of the tracker json to file.""" @@ -669,7 +702,7 @@ def trackInstalled(background=None): return # if already installed, skip - if Tracker.json["status"] is None and Tracker.json["install_id"] is not None: + if not Tracker.json.get("status") and Tracker.json.get("install_id"): return if Tracker.verbose: @@ -687,7 +720,7 @@ def runInstall(background): location = "/1/track/install.json" # capture re-installs/other status events - if Tracker.json["status"] is None: + if Tracker.json.get("status") is None: status = "New install" else: status = Tracker.json["status"] @@ -703,7 +736,7 @@ def runInstall(background): "status": Tracker.string_trunc(status), "platform": Tracker.platform, "language": Tracker.language, - "ID": str(Tracker.json["install_id"]) + "ID": str(Tracker.json.get("install_id", "")) }) _ = Tracker.request('POST', location, payload, background, callback) From 9aa378a33284110e8b70004c13d24d2b8dc6ce8d Mon Sep 17 00:00:00 2001 From: "Patrick W. Crawford" Date: Fri, 6 Oct 2023 23:56:39 -0700 Subject: [PATCH 3/6] Raise more specific error with runtime error wrapper in background. --- MCprep_addon/tracking.py | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/MCprep_addon/tracking.py b/MCprep_addon/tracking.py index 6242a710..613f81e9 100644 --- a/MCprep_addon/tracking.py +++ b/MCprep_addon/tracking.py @@ -670,7 +670,7 @@ def execute(self, context): if bpy.app.background: env.log("Skip Report logging, running headless") env.log("Would have reported:") - raise Exception(self.error_report) + raise RuntimeError(self.error_report) return {'CANCELLED'} if not VALID_IMPORT: @@ -806,8 +806,11 @@ def runUsage(background): runUsage(background) -def logError(report, background=None): - """Send error report to database.""" +def logError(report: dict, background=None): + """Send error report to database. + + report: structure like {"error": str, "user_comment": str} + """ if not VALID_IMPORT: return @@ -825,15 +828,10 @@ def runError(background): else: location = "/1/log/user_report.json" - # extract details - if "user_comment" in report: - user_comment = report["user_comment"] - else: - user_comment = "" - if "error" in report: - error = report["error"] - else: - error = "" + # Extract details + user_comment = report.get("user_comment", "") + error = report.get("error", None) + if error is None: print("No error passed through") return @@ -977,8 +975,6 @@ def wrapper_safe_handler(self): def layout_split(layout, factor=0.0, align=False): """Intermediate method for pre and post blender 2.8 split UI function""" - if not hasattr(bpy.app, "version") or bpy.app.version < (2, 80): - return layout.split(percentage=factor, align=align) return layout.split(factor=factor, align=align) From 2747df62ab8364de8705dd90eca760a9edf9390a Mon Sep 17 00:00:00 2001 From: "Patrick W. Crawford" Date: Fri, 6 Oct 2023 23:57:00 -0700 Subject: [PATCH 4/6] Establish baseline tracker.py unittest --- test_files/tracker_test.py | 94 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100644 test_files/tracker_test.py diff --git a/test_files/tracker_test.py b/test_files/tracker_test.py new file mode 100644 index 00000000..7106214c --- /dev/null +++ b/test_files/tracker_test.py @@ -0,0 +1,94 @@ +# ##### BEGIN GPL LICENSE BLOCK ##### +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software Foundation, +# Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. +# +# ##### END GPL LICENSE BLOCK ##### + +import os +import unittest + +import bpy + +from MCprep_addon import tracking + + +class TrackingTest(unittest.TestCase): + """Tracking-related tests.""" + + @classmethod + def setUpClass(cls): + bpy.ops.preferences.addon_enable(module="MCprep_addon") + + def setUp(self): + """Clears scene and data between each test""" + self.tst_tracker = tracking.Singleton_tracking() + self.tst_tracker.initialize( + appurl="https://mcprep-1aa04.firebaseio.com/", + version=str((0, 0, 1)), + language="en_US", + blender_version=(2, 80, 0)) + + # Setup vars for testing purposes + self.tst_tracker.dev = True + self.tst_tracker.tracking_enabled = True + self.tst_tracker.background = False # Syncrhonous + self.tst_tracker.failsafe = False # test either way + + self._backup_tracker = tracking.Tracker + tracking.Tracker = self.tst_tracker + + def tearDown(self): + tracking.Tracker = self._backup_tracker + + def test_track_install_integration(self): + """Ensure there are no exceptions during install.""" + tracking.trackInstalled(background=False) + + def test_track_usage_integration(self): + """Ensure there are no exceptions during install.""" + tracking.trackUsage( + "test_function", param=None, exporter=None, background=False) + + def test_log_error_integration(self): + """Ensure there are no exceptions log error install.""" + report = {"error": "test_error", "user_comment": "test comment"} + tracking.logError(report, background=False) + + def test_report_error_wrapper_pass(self): + + @tracking.report_error + def my_func(_self, context): + ret_val = context + return {ret_val} + + with self.subTest("finished"): + res = my_func(None, context='FINISHED') + self.assertEqual(res, {'FINISHED'}) + with self.subTest("cancelled"): + res = my_func(None, context='CANCELLED') + self.assertEqual(res, {'CANCELLED'}) + + def test_report_error_wrapper_exception(self): + + @tracking.report_error + def my_func(_self, context): + raise Exception("Test exception") + return {'FINISHED'} + + # RuntimeError comes from re-raising if bpy.app.background + # (and trying to mock.patch this object seems to break things) + with self.assertRaises(RuntimeError): + res = my_func(None, None) + self.assertEqual(res, {'CANCELLED'}) From bdda931f7882e48946505db0660d862ba89d0f57 Mon Sep 17 00:00:00 2001 From: "Patrick W. Crawford" Date: Sat, 7 Oct 2023 14:38:13 -0700 Subject: [PATCH 5/6] Ensure bad parent id is overwritten by good local id + exhaustive tests --- MCprep_addon/tracking.py | 37 ++++---- test_files/tracker_test.py | 174 +++++++++++++++++++++++++++++++++++++ 2 files changed, 195 insertions(+), 16 deletions(-) diff --git a/MCprep_addon/tracking.py b/MCprep_addon/tracking.py index 613f81e9..9f71026d 100644 --- a/MCprep_addon/tracking.py +++ b/MCprep_addon/tracking.py @@ -433,28 +433,33 @@ def set_tracker_json(self): else: valid_tracker = True - if not valid_tracker: + if os.path.isfile(self._tracker_idbackup): + with open(self._tracker_idbackup) as data_file: + idbackup = json.load(data_file) - if os.path.isfile(self._tracker_idbackup): - with open(self._tracker_idbackup) as data_file: - idbackup = json.load(data_file) - - bu_id = idbackup.get("IDNAME") - if bu_id in SKIP_IDS: - if self._verbose: - print(f"{self._addon}: Skipping blocked ID list") + bu_id = idbackup.get("IDNAME") + if bu_id in SKIP_IDS: + if self._verbose: + print(f"{self._addon}: Skipping blocked ID list") + if valid_tracker is True: + # If the backup id is bad, but the local id is good, just + # re-use the child (and immediately re-create backup). + _json["install_date"] = idbackup["date"] + os.remove(self._tracker_idbackup) # Will be re-generated. + self.json = _json + Tracker.save_tracker_idbackup() + else: # Still count as a reinstall _json["status"] = "re-install" _json["install_date"] = idbackup["date"] - elif bu_id is not None: - print("Detected this scenario???") - if self._verbose: - print(f"{self._addon}: Reinstall, getting IDNAME") - _json["install_id"] = idbackup["IDNAME"] - _json["status"] = "re-install" - _json["install_date"] = idbackup["date"] + elif not valid_tracker and bu_id is not None: + if self._verbose: + print(f"{self._addon}: Reinstall, getting IDNAME") + _json["install_id"] = idbackup["IDNAME"] + _json["status"] = "re-install" + _json["install_date"] = idbackup["date"] self.json = _json self.save_tracker_json() diff --git a/test_files/tracker_test.py b/test_files/tracker_test.py index 7106214c..28ad7ca7 100644 --- a/test_files/tracker_test.py +++ b/test_files/tracker_test.py @@ -16,8 +16,10 @@ # # ##### END GPL LICENSE BLOCK ##### +import json import os import unittest +from unittest import mock import bpy @@ -52,6 +54,19 @@ def setUp(self): def tearDown(self): tracking.Tracker = self._backup_tracker + def test_no_local_idfile(self): + """Safety check to ensure we never ship with an id file (again).""" + git_dir = os.path.dirname(os.path.dirname(__file__)) + jfile = "mcprep_addon_tracker.json" + jpath = os.path.join(git_dir, jfile) + par_jfile = "mcprep_addon_trackerid.json" + par_jpath = os.path.join(git_dir, par_jfile) + + self.assertFalse( + os.path.isfile(jpath), f"Should not have local {jfile}") + self.assertFalse( + os.path.isfile(par_jpath), f"Should not have local {par_jfile}") + def test_track_install_integration(self): """Ensure there are no exceptions during install.""" tracking.trackInstalled(background=False) @@ -92,3 +107,162 @@ def my_func(_self, context): with self.assertRaises(RuntimeError): res = my_func(None, None) self.assertEqual(res, {'CANCELLED'}) + + def test_bad_id_resolution(self): + """Validate that if we encounter a bad id, we handle as expected""" + + self.tst_tracker._httpclient_fallback = False # Force use of requests + + new_id = "-NEWID" + good_id = "-GOODID" + good_idb = "-GOODID_B" + bad_id = "-BAD" + + tracking.SKIP_IDS.append(bad_id) + + test_cases = [ + # struc of: new_install_id, local_id, parent_id + { + "scenario": "Pre-exisitng good install", + "newid": new_id, + "localid": good_idb, + "parentid": good_id, + "newlocal": good_idb, # Parent should override + "newparent": good_id, + "will_post": False # Will skip post, local install is good + }, + { + "scenario": "Fresh install with valid parent", + "newid": new_id, + "localid": None, + "parentid": good_id, + "newlocal": good_id, + "newparent": good_id, + "will_post": True + }, + { + "scenario": "First time install", + "newid": new_id, + "localid": None, + "parentid": None, + "newlocal": new_id, + "newparent": new_id, + "will_post": True + }, + { + "scenario": "Bad ID local only", + "newid": new_id, + "localid": bad_id, + "parentid": good_id, + "newlocal": good_id, + "newparent": good_id, + "will_post": True + }, + { + "scenario": "Bad ID local and par", + "newid": new_id, + "localid": bad_id, + "parentid": bad_id, + "newlocal": new_id, + "newparent": new_id, + "will_post": True + }, + { + "scenario": "Bad ID par", + "newid": new_id, + "localid": good_id, + "parentid": bad_id, + "newlocal": good_id, + "newparent": good_id, + "will_post": False # Just replace the parent if local ok + } + ] + + template_id = """ + { + "install_date": "2023-10-06 23:12:50.524861", + "install_id": "$localid", + "enable_tracking": true, + "status": null + } + """ + + template_parid = """ + { + "IDNAME": "$parentid", + "date": "2023-10-06 22:43:30.855073" + } + """ + + def rm_files(): + try: + os.remove(self.tst_tracker._tracker_json) + except FileNotFoundError: + pass + try: + os.remove(self.tst_tracker._tracker_idbackup) + except FileNotFoundError: + pass + + def do_setup(tcase): + base = os.path.dirname(__file__) + self.tst_tracker._tracker_json = os.path.join( + base, "tst_tracker.json") + self.tst_tracker._tracker_idbackup = os.path.join( + base, "tst_tracker_backup.json") + + rm_files() + + if tcase["localid"]: + with open(self.tst_tracker._tracker_json, 'w') as fd: + data = template_id.replace("$localid", tcase["localid"]) + fd.write(data) + + if tcase["parentid"]: + with open(self.tst_tracker._tracker_idbackup, 'w') as fd: + data = template_parid.replace( + "$parentid", tcase["parentid"]) + fd.write(data) + + self.assertEqual(os.path.isfile(self.tst_tracker._tracker_json), + tcase.get("localid") is not None) + self.assertEqual(os.path.isfile(self.tst_tracker._tracker_idbackup), + tcase.get("parentid") is not None) + + def simulate_startup(tcase): + # Set mock return for requests. + rawdata = {"name": new_id} + + resp = mock.MagicMock() + resp.ok = True + resp.text = json.dumps(rawdata) + + # Test the optin status. + with mock.patch("MCprep_addon.tracking.requests.post", + return_value=resp) as resp_mock: + self.tst_tracker.set_tracker_json() + tracking.trackInstalled(background=False) + if tcase["will_post"] is True: + resp_mock.assert_called_once() + else: + resp_mock.assert_not_called() + + def check_outcome(tcase): + self.assertTrue(os.path.isfile(self.tst_tracker._tracker_json)) + self.assertTrue(os.path.isfile(self.tst_tracker._tracker_idbackup)) + + with open(self.tst_tracker._tracker_json, 'r') as fd: + content = fd.read() + self.assertIn(tcase["newlocal"], content) + with open(self.tst_tracker._tracker_idbackup, 'r') as fd: + content = fd.read() + self.assertIn(tcase["newparent"], content) + + for tcase in test_cases: + with self.subTest(tcase["scenario"]): + + do_setup(tcase) + simulate_startup(tcase) + check_outcome(tcase) + rm_files() + rm_files() From 68aeb8880621ec54c0dbe6728bfaf9a75d7bdb22 Mon Sep 17 00:00:00 2001 From: "Patrick W. Crawford" Date: Mon, 16 Oct 2023 22:50:42 -0700 Subject: [PATCH 6/6] Renaming SKIP_IDS to be INVALID_IDS. --- MCprep_addon/tracking.py | 11 ++++++----- test_files/tracker_test.py | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/MCprep_addon/tracking.py b/MCprep_addon/tracking.py index 9f71026d..08e4bb31 100644 --- a/MCprep_addon/tracking.py +++ b/MCprep_addon/tracking.py @@ -32,7 +32,8 @@ # Ids with known analytical problems, such as clobbered-together installs. # If we see any of these IDs in local json files, still treat as a re-install # but replace the ID with the new one received. -SKIP_IDS = ["-Nb8TgbvAoxHrnEe1WFy"] +# See example: https://github.com/Moo-Ack-Productions/MCprep/issues/491 +INVALID_IDS = ["-Nb8TgbvAoxHrnEe1WFy"] # remaining, wrap in safe-importing @@ -424,12 +425,12 @@ def set_tracker_json(self): _json = jdata if self._verbose: print(f"{self._addon}: Read in json settings from tracker file") - if jdata.get("install_id") in SKIP_IDS: + if jdata.get("install_id") in INVALID_IDS: valid_tracker = False _json["install_id"] = None _json["status"] = "invalid_id" if self._verbose: - print(f"{self._addon}: Skip ID detected, treat as new") + print(f"{self._addon}: Invalid ID detected, treat as new") else: valid_tracker = True @@ -438,9 +439,9 @@ def set_tracker_json(self): idbackup = json.load(data_file) bu_id = idbackup.get("IDNAME") - if bu_id in SKIP_IDS: + if bu_id in INVALID_IDS: if self._verbose: - print(f"{self._addon}: Skipping blocked ID list") + print(f"{self._addon}: Skipping Invalid ID") if valid_tracker is True: # If the backup id is bad, but the local id is good, just diff --git a/test_files/tracker_test.py b/test_files/tracker_test.py index 28ad7ca7..8657ea80 100644 --- a/test_files/tracker_test.py +++ b/test_files/tracker_test.py @@ -118,7 +118,7 @@ def test_bad_id_resolution(self): good_idb = "-GOODID_B" bad_id = "-BAD" - tracking.SKIP_IDS.append(bad_id) + tracking.INVALID_IDS.append(bad_id) test_cases = [ # struc of: new_install_id, local_id, parent_id