[closed] Object field diff misses some data types

mrfelna · September 30, 2015, 11:09am

As far as I can see, the following method needs some extension work.

farcrycore/core/blob/master/packages/lib/diff.cfc#L141-L179


	<cffunction name="getPropertyDiff" access="public" output="false" returntype="struct" hint="Performs diff on the property according to its type">
		<cfargument name="typename" required="true" />
		<cfargument name="left" required="true" />
		<cfargument name="right" required="true" />
		<cfargument name="stMetadata" required="true" />
		
		<cfset var stResult = structnew() />
		<cfset var stTemp = structnew() />
		<cfset var i = 0 />
		<cfset var j = 0 />
		<cfset var thistype = "" />
		
		<cfset stResult.left = arguments.left />
		<cfset stResult.right = arguments.right />
		<cfset stResult.leftHighlighted = arguments.left />
		<cfset stResult.rightHighlighted = arguments.right />
		<cfset stResult.aDiff = arraynew(1) />
		<cfset stResult.different = false />
		<cfset stResult.formtool = arguments.stMetadata.type />
		<cfset stResult.label = arguments.stMetadata.name />

This file has been truncated. show original

AFAIK, the function is meant to receive the same field from two objects and compare them. However, if the type/ftType is any of the following, the function skips it and always return “they are not different”, regardless of the actual values:

Boolean
Image
List
Numeric (but not an integer)

And a number of more obscure field types.This is noticeable in the webtop when, after editing a piece of versioned content, the overview incorrectly displays a “The draft is identical, discard?” message as described here: Difference between DRAFT and APPROVED.

Even with this irritation, it is an awesome feature

jeff · September 30, 2015, 7:11pm

Yeah, I get confused with the boolean one all the time. I’ll have a active (or disabled) boolean for a content type. Set it. Assuming it’s a versioned content type, the overview page will say that the draft page is identical to the approved page and it is safe to delete the draft page.

modius · October 2, 2015, 1:06am

@mrfelna are you putting your hand up for a fix?

mrfelna · October 2, 2015, 5:15am

I’m always happy to do the work. As with most of my posts, it’s
more just checking I’ve got the right idea.

Even though I’ve been using FarCry on and off for nearly three years, I
still feel like I’ve barely scratched the surface!

justincarter · October 2, 2015, 6:26am

You’re spot on, those are just plain missing It definitely needs a good once over to fill in the gaps.

mrfelna · October 3, 2015, 9:18pm

I’ve started to “fill in the gaps”

https://github.com/MrFelna/core/commit/424716c6524f5179a3c262c4850ed16abf53eec2

Just a couple of questions:

Would it be better to create a PR per field type, or add all types then submit one big PR?
I plan on adding at least image, numeric and list to the comparison types. Any requests for other field types?
Would it be good to try and add a generic catch all?

modius · October 5, 2015, 11:26pm

What ever works for you

PRs that can be merged quickly are always welcome. Release each isolated improvement if its likely you’ll need a lot of time to get the whole thing done.

Maybe there’s option for serialising stobj and comparing the entire object minus datetimelastupdated, lockedby and any other system properties that might generate false positives.

justincarter · October 5, 2015, 11:49pm

All formtools need to be covered, but there will be many formtools that are just strings (image, file, state, country, etc) and could probably be handled by a single case inside getPropertyDiff(), e.g. they could be added to the existing “string,longchar” case:

github.com

farcrycore/core/blob/master/packages/lib/diff.cfc#L179


		<cfswitch expression="#stResult.formtool#">
			<cfcase value="richtext">
				<cfset stResult.left = getHTMLAsText(stResult.left) />
				<cfset stResult.right = getHTMLAsText(stResult.right) />
				<cfset stResult.different = compare(stResult.left,stResult.right) neq 0 />
				<cfif stResult.different>
					<cfset stResult.aDiff = getDiff(old=stResult.left,new=stResult.right) />
					<cfset structappend(stResult,convertDiffToHighlights(stResult.aDiff),true) />
				</cfif>
			</cfcase>
			<cfcase value="string,longchar" delimiters=",">
				<cfset stResult.different = compare(stResult.left,stResult.right) neq 0 />
				<cfif stResult.different>
					<cfset stResult.aDiff = getDiff(old=stResult.left,new=stResult.right) />
					<cfset structappend(stResult,convertDiffToHighlights(stResult.aDiff),true) />
				</cfif>
			</cfcase>
			<cfcase value="category">
				<cfset stTemp[arguments.stMetadata.name] = arguments.left />
				<cfset stResult.left = application.formtools.category.oFactory.display(typename=arguments.typename,fieldname=arguments.stMetadata.name,stMetadata=arguments.stMetadata,stObject=stTemp) />
				<cfset stTemp[arguments.stMetadata.name] = arguments.right />

Some could be ignored such as creditcard* since it’s unlikely those are ever stored, they are usually used in components which extend farcry.core.packages.forms.forms and so are transient.

modius · October 11, 2015, 10:25pm

Many thanks @mrfelna

mrfelna · October 11, 2015, 10:30pm

No problem

Still got some types to add, but no idea when / if I’ll have the time

mrfelna · October 27, 2015, 10:00am

Oops - thanks for the fix