#bookbrainz

/

      • agatzk has quit
      • agatzk joined the channel
      • agatzk has quit
      • agatzk joined the channel
      • agatzk has quit
      • Shubh
        monkey: ping
      • monkey
        pong
      • Latency pretty good :)
      • Shubh
        for some reason if i tried to add relationships on same entity for more than one time like first authors to work then work to edition, then the later one override the previous relationship. so expected result should be work having two relationships but found only one :(
      • that is work to edition...
      • monkey
        Can you point me to the code? Looks like the relationships array/map get overwritten
      • Shubh
        might be issue with orm?
      • monkey
        To figure out where it comes from, I would: 1. check the redux state before submitting (with Redux Devtools) 2. put a breakpoint in the server code submit route to check the content of the body that is sent 3. Put breakpoints in the part of the server code that creates the new relationshipSet for the revision
      • Shubh
        this line https://github.com/metabrainz/bookbrainz-site/b..., giving null value even if entity have relationshipSet
      • monkey
        Hm. Can you check in the database if the master revision of the entity you're modifying has a relationship_set_id ?
      • If it does, then the ORM is returning something it shouldn't (or some other part of the code is broken)
      • Shubh
        yep it does have
      • monkey
        Also check in the DB if the relationship set has the relationship(s) you expect it to have
      • This is all happening in the uf-frontend branch I suppose? I can test locally
      • Shubh
        here as you can see, work links with author properly but later revision(*85) modifies relationship on work with edition and removes the old relationship with author https://usercontent.irccloud-cdn.com/file/jkw2W...
      • monkey: hmm, any branch which have POST route PR merged
      • monkey
        So there's something I notice straight away: the revisions are not created in order
      • So I'm guessing at some point of the transaction the entity model doesn't have the updated relationship set id
      • Shubh
        hmm but does revision order have any effect?, serial number of revision looks fine to me that is adding relationship on edition at last
      • monkey
        It's more a hint that the issue is with the creation of the revisions rather than with the ORM I guess
      • Shubh
        does seem like an issue with transaction, like we had previously when doing thing async
      • monkey
        Look at this piece of code: I had to call .refresh on a relationship set that might have been modified by some other part of the transaction, precisely for the same reason: https://github.com/metabrainz/bookbrainz-site/b...
      • You'll need to find where exactly you need to get a more up-to-date representation of the relationshipset before modifying it again. Since we are in a transaction, we don't necessarily have access to the updated rel-set, instead we still have info about the previous rel-set before it being modified.
      • What I mean by that is that in a transaction we can't assume that the operations we want to do are happening in order and the later operations can assume the previous ones have happened already. So we can't rely on a change having happened, even though the code reads pretty linearly.
      • However we can push the ORM to return up-to-date data for each model with the `.refresh({transacting…` incantation
      • Obviously it gets a bit more complex when you are creating/modifying more and more entities :)
      • Perhaps it would be as simple as calling .refresh(… in getNextRelationshipSets ?
      • Seems pretty redundant, bu it would be a start to see if that fixes your issue
      • Shubh
        4:10 PM <Shubh> this line https://github.com/metabrainz/bookbrainz-site/b..., giving null value even if entity have relationshipSet
      • my bad, but this should be null since at that time edition didn't have any relationship thus resulting in null
      • monkey
        Ah, OK, that makes sense
      • But I guess that's the trick: it's null in the database, but might have been created in the transaction (i.e. not-commited-yet info which you can retrieve with refresh)
      • Shubh
        not sure if refresh will work since entity will have relationshipSetId as null initially
      • monkey
        But in revision 84 you add a relationship as part of the transactions, no?
      • The question is whether this line is able to retrieve the relationship set added to the model during rev 84 in the transaction https://github.com/metabrainz/bookbrainz-site/b...
      • Shubh
        Yes but it does return correct relationshipshipSetId for work ( that has relationship with author_
      • monkey
        So #84 results in a new rel-set, and while adding rels in #85 we get the correct rel-set id, but not the relationships in it are replacing the existing ones instead of being added, is that it?
      • "but not the relationships in it are" -> "but the relationships in it are"
      • Shubh
        Yep!
      • thats why i suspected something might be related with orm
      • monkey
        OK, we're narrowing it down. Looks like withRelated: 'relationships' does not fetch the updated rels.
      • I think my suggestion above might work then (https://www.irccloud.com/pastebin/AVSicJiw/)
      • Or maybe instead of doing fetch then refresh, you can just *replace* fetch with replace. Not sure if that works but worth a try
      • Yeah, looking at the implementation that might work
      • Shubh
        ok just check not an issue with orm
      • i tried same steps with entity editor
      • monkey
        Right. This definitely looks like a not-updated-during-transaction issue.
      • Shubh
        and relationships linking properly!
      • lets try refresh then
      • ... it worked!
      • monkey
        Huzzah !
      • Good, cause I was out of ideas if that wasn't it :)
      • Shubh
        like magic, Thanks so much!
      • btw what does replace do? couldn't find any references to that in docs
      • monkey
        Where sorry?
      • The only replace I can think of it on strings
      • Shubh
        replace method on model
      • monkey
        Is that a thing? Where do you see that?
      • Shubh
        4:39 PM <monkey> Or maybe instead of doing fetch then refresh, you can just *replace* fetch with replace. Not sure if that works but worth a try
      • wait wait you mean substitute fetch with replace, right?
      • monkey
        Yes, sorry if that wasn't clear 😅
      • To be clear then:
      • I think this might just work (without the need to call fetch then call refresh right after). But I'm not 100% sure
      • Shubh
        Ah now that make sense
      • monkey
        I left the `require:false` out, but maybe it would be needed as well to avoid an error being thrown
      • Shubh
        i'm wondering if it even hit this line of code, since id is null for both entities at that time
      • Ahh maybe promise.all on processing relationship might be an issue
      • making it sequential does seem to fix the issue :)
      • monkey
        Ah, great
      • Yes that makes sense. Running everything in parallel could cause issues, didn't think about that
      • Shubh
        but now submitting is kinda slow :<
      • monkey: we want to copy authors to existing works as well, right?
      • for this i'm thinking of making POST route to do edit as well, does this sound sane to you?
      • like we have for entity-editor
      • ansh
        monkey: for fetching the reviews from CB, should I use axios or node-fetch ?
      • monkey
        ansh: we have superagent installed that should work well: https://visionmedia.github.io/superagent/
      • Shubh: I think that makes sense. I would suggest doing that in a separate PR though.
      • Shubh
        should i include fix for relationships on that PR as well?
      • monkey
        Good idea.
      • agatzk joined the channel