SEED-platform / seed

Standard Energy Efficiency Data (SEED) Platform™ is a web-based application that helps organizations easily manage data on the energy performance of large groups of buildings.
Other
107 stars 55 forks source link

Unmerge properties with meters assigns meters incorrectly #2157

Open macintoshpie opened 4 years ago

macintoshpie commented 4 years ago

Expected Behavior

After merging properties with their own meters, unmerging them should result in them having their original meters.

Actual Behavior

After unmerging, it appears that only one of the properties meters is used and assigned to both properties. Ie, one gets it's original meters, but the other gets the other property's meters as well.

Steps to Reproduce

Using files here: https://drive.google.com/drive/folders/1w9d1mX6MxMaZi0fsTP3HX_WItP2G6WCk

  1. Upload the Monthly Metrics sample data
  2. Upload the example pm monthly meter data
  3. Upload the buildingsync data
  4. Merge the first PM property with the buildingsync property - see that their meters have been merged
  5. Go to the property detail pages and unmerge the properties
  6. See that the meters for one of the properties is incorrect

Note that there may be other cases where this issue occurs:

Instance Information

Discovered on develop SHA: e08aa15f9ec6397c9ae19197ee1ab915d6e00e6c

Related Issues

Possibly #1984

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity within 60 days. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity within 60 days. It will be closed if no further activity occurs. Thank you for your contributions.

isalanglois commented 1 year ago

@nllong or @RDmitchell Can we verify if this is still relevant after recent release?

RDmitchell commented 1 year ago

@isalanglois / @nllong -- can we either add a Test status to the 23 Q4 project, or move it to the Q1 or Q2 projects, set the status to Test and assign my name to it, so that I will see it in the Test column for me to test? thx.

RDmitchell commented 1 year ago

@isalanglois -- I guess it's fine to leave it in the To Do column as long as I am assigned to it. I will try to remember to look at that project when looking for things to test.

isalanglois commented 1 year ago

@RDmitchell I added a test column for this project and move it there

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity within 60 days. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity within 60 days. It will be closed if no further activity occurs. Thank you for your contributions.

RDmitchell commented 1 year ago

Instance: dev1 SHA: 1f888a125 Org: LBNL Test 200

This is still an issue. The meters merged properly but the unmerge did not. The records remained merged on both the records that were unmerged.

Here is what I did:

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity within 60 days. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity within 60 days. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] commented 10 months ago

This issue has been automatically marked as stale because it has not had recent activity within 60 days. It will be closed if no further activity occurs. Thank you for your contributions.

RDmitchell commented 10 months ago

@kflemin -- This is probably a big deal to fix, but since Clearly Energy would like the unmerge properties issues fixed (#4408) it might be worth looking into this ??

So I added P-1 but you all should evaluate it also based on level of effort.

haneslinger commented 9 months ago

Okay, so this is quite a big lift, let me outline why:

As we all know:

  1. Properties related to PropertyViews related to PropertyStates.
  2. Properties do not relate directly to states.
  3. Of the three, only states have any sense of history (ie, I was a product of a merge between these two states).
  4. Meters related to Properties.

Now, when we do a merge:

  1. a new Property, PropertyView, and PropertyState is created.
  2. The existing meters are added to the new Property.
  3. The old PropertyViews are deleted

From the above, we know unmerging the meters would be tricky because neither the Property nor the meters have any memory of which Property the meters were inherited from, so we dont know where to give them back to.

Now, if I HAD to take a crack at fixing this, here is what I think I would do: Somehow, meters must know from whence they came. Currently, PropertyStates are the only objects with a sense of history. I don't want to create a new history log, so I guess I would link meters to property states? This would once again make us confront https://github.com/SEED-platform/seed/issues/3904, but I think the bigger problem with this solution is it's just messy. Without loads of context, it's not obvious why meters woud be related to both Property and PropertyStates, not to mention its a new way the db could hold corrupt info (ie, a meter has Property a and PropertyState b, but PropertyState b is not a part of Property a).

I suggest before pulling the trigger on my tape and bubble gum solution, we all sit down and chat about it. Maybe the heart of the user pain point isn't what I think it is and we can address that different issue, or another dev has a better solution.

github-actions[bot] commented 6 months ago

This issue has been automatically marked as stale because it has not had recent activity within 60 days. It will be closed if no further activity occurs. Thank you for your contributions.