search

culturecreates / tech-support-tickets

Place to manage all tech support tickets with clients
0 stars 0 forks source link

Merge duplicate event Christine Morency #38

Closed saumier closed 1 year ago

saumier commented 1 year ago

I merged these 2 events in CMS. I am experimenting with the idea of an event having 2 sameAs Artsdata IDs K23-840 and K23-345.

I added the 2 extra dates in 2023 to the currently published event with 3 dates in 2024. I also deleted the unpublished event.

Image

saumier commented 1 year ago

@troughc I went ahead a did this experimental merge of 2 events. Let me know if you have any issues. This one was tricky because there were 2 dates from 2023 and 3 dates from 2024. We can discuss how to handle this moving forward for the next events. I think it is better to keep separate events when they are spread out like this. But in this case, since it was confusing and already existed with 2 Artsdata IDs, I went ahead and merged them in CMS so I could learn if the code will support a single CMS event with 2 Artsdata IDs.

saumier commented 1 year ago

@troughc I am assigning this to you just to make sure you saw this incase Isabelle asks about it. I normally NEVER touch the live data but this one was really troubling me and I also wanted to see if my solution of adding 2 Artsdata Ids to the same CMS event would cause a problem with Aggregator. I still have to check on this.

troughc commented 1 year ago

@saumier thanks for letting me know. I will be interested to hear how it goes.

saumier commented 1 year ago

@sahalali Hi. Do you think there is any problem if an event like one "Christine Morency" has 2 Artsdata URIs in sameAs?

The event in CMS has the dates from both events in Artsdata. So I added both Artsdata URIs and my expectation is that both events from Artsdata will not create a new event because they both already exist in CMS https://api.footlight.io/events/645b93f27db98f0065dc623f

sahalali commented 1 year ago

@saumier Yes, Currently we expect only a single Artsdata URI in the sameAs. Nothing going to break in the backend. But the comparison of entities is done with any of the artsdata URL within the sameAs values. We will need to modify the comparison with Artsdata URI to make sure everything works as expected.

saumier commented 1 year ago

@sahalali I propose we add a unit test for this case. Let me know,

saumier commented 1 year ago

@sahalali Can I close this?

sahalali commented 1 year ago

@saumier

@dev-aravind is adding unit test for this.

dev-aravind commented 1 year ago

@saumier I wrote a unit test for this issue which was failing, so made some changes to the updateEvent API to fix this. It's up in staging right now.

saumier commented 1 year ago

@sahalali @dev-aravind I am having trouble running the API Docker and the tests locally. Please see issue https://github.com/culturecreates/footlight-calendar-api/issues/651.

Regarding the test, I added 2 comments to the code.

dev-aravind commented 1 year ago

@dev-aravind I went through both of your comments and replied to them in the commit thread and also made changes to the code to address the issues you mentioned.

saumier commented 1 year ago

@dev-aravind I have 2 questions:

  1. Can you explain why you removed type. Is it not needed? See https://github.com/culturecreates/footlight-calendar-api/commit/4573e0fdad8e7d38c9f3ef90b349a7d1f110f14a
  2. I was asking you to add a different Artsdata URI to test the case where there are 2 Artsdata URIs. Is there a reason why you did not? It does not make sense to me when reviewing the test that 2 identical URIs should be kept. In fact if there are 2 identical URIs then I could easily expect one to be removed in the future. So to make the test more meaningful please add a different URI when you add the second Artsdata URI. If this does not make sense let me know. I am blocked right now because I cannot run the tests locally, otherwise I would have made the change to show you instead of this lengthy comment ;-) See https://github.com/culturecreates/footlight-calendar-api/commit/cf0313a4ca0dac2abbb5797faf467ecfa4159180#r128340353
dev-aravind commented 1 year ago

@saumier

  1. The datatype for the property type was initially wrong in the CMS. So when I asked @sahalali about it, he suggested me to fix the test case and remove the type property as it was not necessary for Events.

  2. The Artsdata URIs are actually different. The first one that I used to define the event was http://kg.artsdata.ca/resource/K23-193, and the second one that I pushed for that specific test was http://kg.artsdata.ca/resource/K23-194. I think you didn't notice it because it was a minor change maybe? I'll try to make the tests more easier to comprehend as I learn more about Jest.

Also I discussed with @sahalali about the unit tests not running for you and I think it's probably a node version issue. @sahalali will follow up on that with you.

saumier commented 1 year ago

@dev-aravind Great! Thanks for the explanations. Indeed I see that the Artsdata URIs are different now.