Merge and Link Matches -- didn't seem to merge records that should have merged

[RDmitchell]

Expected Behavior

I expected to have the program automatically merge together two records that I had unmerged by hand.

Actual Behavior

Program didn't merge them back together again

Steps to Reproduce

Org: LBNL Test 200 Cycle: Test ReMerging

Imported two different files

• 1 - example-data-ESPM-Alerts-CustomID.xlsx
• 2 - example-data-ESPM-Alerts-UBID.xlsx (has a slightly different Energy Star Score than the first file)

Matching fields were Custom ID 1 and PM Property ID

In the Inventory List view, they are all shown as merged

Looking at the Inventory Detail View for one of the records, ie, Custom ID 1 == 102, the two records are merged -- the first file (far right column) shows an Energy Star Score of 63. The second file shows an Energy Star Score of 67.

I use the "Unmerge Last" button to unmerge the two records.

They are now 2 separate records, ie, 2 records with Custom ID 1 == 102.

Going back to the Detail view for the first record, click the Merge and Link Matches option from the Action pulldown

Nothing happens -- the two records are still unmerged

Instance Information

Instance; seeddemostaging SHA: 4afb845b3 (2.7.2.1)

[RDmitchell]

Files can be found in the issue folder https://drive.google.com/drive/folders/1fnLzjZPHNWILWw0Y4cZh3P-M2GxhP40-?usp=sharing

[adrian-lara]

Ah, ok - I see what's happening.

As hinted in the developer console, we botched the permissions on this request. We inadvertently only allowed admins to be able to run this command. It should be any organization member that can modify data.

Given this is a new feature, I'd lean towards patching it just so users don't get a bad first impression of this. We were going to officially release a patch anyway - this can be thrown into it as well.

Alternatively, I could also see the argument that this is such an edge case that we maybe shouldn't worry about patching necessarily.

Let me know if you feel strongly either way.

Note, the permissions were fixed in latest development.

[RDmitchell]

@adrian-lara -- I think it is actually OK to not fix this in a 2.7.2 patch.

It is an edge case, and it is also a bit "dangerous", and I haven't been able to test it at all.

So I would recommend NOT including the fix in the 2.7.2 patch, and I will just tell users that it is not working at this time. I suspect no one will need it.

Others may have a different opinion, of course.

[Ryoken]

I can confirm this is working as expected on the current build. As an org member I was able to import the two link sheets, see the merged records, un-merge and then re-merge successfully. No code changes were needed.

[RDmitchell]

@Ryoken / @nllong -- I would like to test this, so can we reopen it and put it in test once the fix is deployed to dev1?

[Ryoken]

I've just verified this works as expected on v2.9.0

[RDmitchell]

Instance: dev1 SHA: 9475793de Org: LBNL Test 200 Cycle: 2018 Compliance Cycle

This works in terms of merging the record.

However, the records are not merged in a way that maintains the "date" history order. In this particular case, the earlier values, from the record created on 10-23, are the latest (left-most) in the history, and the later record, created 12-15, is on the right side. This means that the data, such as ENERYG STAR Score from the earlier record, 67, is merged into the Master record, rather than the later value of 63.

When this type of merge is done "by hand" in the List View, the user is given the option to set the order of the data -- it might be good to implement that same functionality here.

• The user selects the records to be merged

• The program allows the user to put the records in the correct order

• The result is that the correct record controls the Master record

[RDmitchell]

@Ryoken / @adrian-lara -- what do you think about adding the ability to control the order of the merged records? Seems kind of important.

[adrian-lara]

We have this ability already. In your second-to-last screenshot, when you moved "63" to be the first record, you moved the entire record as opposed to just the value of "63".

[RDmitchell]

@adrian-lara -- right, but that is only available when doing merges from the List view.

I want that same functionality in the Detail view when records are merged and matched, ie, to control the order of the records

[adrian-lara]

@RDmitchell to capture our conversation, we brought up the idea of removing the ability to trigger an automatic merge (and linking) of matching records given that the order isn't preserved as expected during merge. Practically speaking, this would just be a simple removal of the option in the detail page dropdown.

Before implementing that, I think there may be something more going on here. I think the expected order of precedence is actually to give highest priority to the "current" record (the one whose detail page you're on), and then go in chronological order from there - higher priority to more recent records. @Ryoken can you verify this?

@RDmitchell the related question I'd have for you is what would the ideal order be?

[RDmitchell]

@adrian-lara -- I think the order should be in chronological order, but I'm not sure that starting with the "current" record and making it the highest priority may not be the right approach.

I think no matter what record you are starting on, the records should just be put into chronological order, even if it means that the current record you are on is not the most current.

Presumably SEED knows how to correctly get the records in chronological order (?)

[adrian-lara]

@Ryoken Could you confirm this piece?

I think the expected order of precedence is actually to give highest priority to the "current" record (the one whose detail page you're on), and then go in chronological order from there - higher priority to more recent records.

[adrian-lara]

Thanks @RDmitchell

I think either functionality would be reasonable as long as we make it clear what's happening. If we do want to make a change, we should definitely open a new ticket for it.

[adrian-lara]

To capture our conversation, @Ryoken we'll disable this feature for now (gray out the option), and @RDmitchell will discuss with users, then open a new ticket with how this should eventually work.

Given this ticket was just to fix the permissions, and it seems that has been achieved, let's just disable this feature and make that the criteria for closing this ticket.

[Ryoken]

Sounds good, I'll push up a branch that disables the option.

[Ryoken]

https://github.com/SEED-platform/seed/pull/2619 is the PR to disable the menu option. I looked into how the properties are merged and at the moment it isn't doing anything intelligent to the id list regarding a sort. The form submits a list of ids, then just before the merge service is hit the list is reversed, so the order is mostly determined by how the form is submitted.

[adrian-lara]

Sorry - we ended up leaving you with a lot to read through. Could you also look into the order specified by the button you disabled? Or is that really it? If it is, where's the list of ids coming from?

[RDmitchell]

Instance: dev1 SHA: b12fbd00f

Currently disabled

@adrian-lara -- not sure if this should be closed or moved to the next release cycle to resolve the issues brought up here?

[adrian-lara]

We can close this since we've disabled it for now and have a new issue open for future changes