GitLab Backend Pairing, 8 Nov 2021

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: BE Pairing - 20211108 - Add customer relations contact quick actions

Description

No description was provided for this meeting.
If this is YOUR meeting, an easy way to fix this is to add a description to your video, wherever mtngs.io found it (probably YouTube).

A

I guess they're good to record items. Do you know where these get uploaded or they aren't unfiltered.

B

So did you do record a record in the cloud I.

A

B

A

B

Okay, so I think it's a manual process. That's.

A

Fine, I figured it out later.

B

Yeah, even though.

A

But okay, let me get some more up.

A

I can tell you right now: I have no recollection of what we talked about last time.

B

I'm sure you're going to be like ladding back, so the the couple of things I'll I'll, try and um reintroduce, and also uh preempt which, if I was a little bit more prepared if I hadn't just come off another pairing session, I would have updated the mr description a little bit and preempted and put a few comments on it myself. So there's definitely a few specs. That is missing.

B

um So I I think we'll we'll identify those for sure, um but the the merge request, a piece of work specifically was around um adding quick actions so whole customer relationship management, crm work that I've been focusing my time on the last couple of months is, is making good headway.

B

We now have lots of graphql endpoints and some services to allow you to create contacts, update contacts attach contacts to issues all that kind of greatness, but you have no way through the gitlab ui to um attach contacts to issues or remove contacts from issues for that matter. So this is, um and I'm now starting to think. Maybe it wasn't with you- or I might have spoken to you guys about some of this. But my brain is is absolutely fried.

B

It might have actually been on the front end pairing session, because um I was initially thinking about making a sidebar um component and thinking this was going to be more like a view component, etc, etc, and then off the back of a conversation, we decided easiest way. um The quickest, smallest iteration, to be able to add the functionality to add and remove contacts from an issue would be to create a new quick action.

B

um So this merge request introduces uh two new quick actions, one to add contacts to an issue and one to remove contacts from an issue.

A

I think we discussed some of this because I seem to remember. I think um there was another engineer on the phone and we were talking about where to put the like, where clauses. That's.

B

Right mispronounced, but um alina or elaine, alina.

A

Yeah selena, that was on yeah.

B

So um would you are you happy to still do you want to share your screen and go through your process, how you would always say too much pressure.

A

You can do that. That's fine, um peter I'd be happy to have you do the review with me. If you want, um I did have a cup of coffee this morning, which is not normal for me. So maybe I'll be a little bit more awake.

C

B

So um actually we have also got another cool one that we could talk about because as part of this, mr um I broke some things. um I found that there were a lot of references in the issue, actions um to quick action, target dot project.

B

So I tried to dry that up by creating a method for project an alias or method or whatever you want to call it, and it turns out that there's some kind of collision. um So we could look at that afterwards. That would be pretty cool as well.

A

Yeah, that would definitely be my first comment, because there are failing specs that look related.

B

Yep yep, so um I I've cancelled the pipeline, um but I've I've got I've fixed, um I'm just trying to think I yeah. I know you don't want to just take my word for it. So, um but we'll definitely talk about that afterwards, because yeah I tried to dry things up, because I thought it seemed crazy everywhere to keep referencing quick action, target dot, project dot, this dot, this dot. This uh yeah.

A

That's definitely something that I would um look at first, a lot of the times when I do reviews is I go and look at the issue to see.

A

If there's like a ton of detail in there, I know you gave us a background, so I might not do that, but I'm just going to open up the issue anyway, so I can have it reference.

A

Let's look at the code, can you see my screen.

B

We can indeed so let me and.

A

I think this is maybe what we were discussing last time is like whether these should be.

B

A

Yeah exactly so, and.

B

A

Where we landed was because of the pluck that it would be classified.

B

That's a really good place. To start, to be honest, um peter do you have thoughts is? Does this look like a sort of good approach, a good place for this to live or.

C

Good place to live for this method in general.

B

Yeah so initially as I was, um I don't know what, if you have a lovely technical term for brain dumping, um kind of pseudo coding, whatever initially this lived in a service, um it was just calling like customer relations, contact dot where dot pluck.

B

Clearly that breaks a few of our best practices or rubricops or whatever you want to call it so moving it to an active model or an active record or whatever the brilliant term is um was, was the suggestion the sort of logical way forward, but I think there's a few different and it could be a scope or it could be a method and it could be potentially an instance method or it could be it so yeah. That's that's the sort of.

C

Or finder, so usually we try to to put like query logic into finders nowadays, so it's basically kind of service, but for for querying data.

C

So but I I think it's it's fine to start with, with the model for now.

B

C

B

If we find ourselves having a lot.

C

Of a lot of functionality inside the model, like creating data, I would consider like thinking about.

B

So if, if we did a finder out of interest, um would do you just have to disable what the ruby cop stuff or or is it does it understand? Does it allow you to do query logic like your wares and stuff inside a finder.

C

Yeah, I think the cop is actually configured this way that it allows to to certain amount of uh active record api being used within finders and models. I think it is with me.

A

Yeah, I feel, like I don't remember, seeing those um like comments that disable the cops and the finders.

B

Yeah, it would wouldn't make sense because it looked like we were sort of violating and hacking around it, so yeah it makes sense. It exclusively.

A

I guess, if you're, finding you're coming back in here and adding more of these into these methods right now, I think these are the only two that exist.

B

A

B

A

I'm not sure it's worthwhile to go through.

C

Yeah, I think right now, it's it's fine. uh The second question I would have for this methods in particular was would be okay. Where can we or where do you do we want to use the ids, because I remember a guideline being updated about trying to avoid plug in certain situations, because we we could use if you use the objects active record could, in certain situations, use the subquery, which is more performant, but let me try to this one.

C

I actually I'm not suggested right now, but this is like my thought process during the review like having a at least a mental question mark for me. Okay, let's review the docs or the our guy.

B

C

Let's try to check whether we should maybe avoid using plug in in this this scenario. Again, I'm not sure I'm not suggesting it. No, no.

B

I'm definitely reading reading to see what.

C

So basically I was actually while skimming the code. I was already trying to understand the service code to and try to figure out whether the ids is being used in a way where we actually should avoid plug plug, so where active record automatically would uh create a sub select for us I'll.

B

C

B

So we could run one select and insert together, rather than a select, separate, select and a separate, insert yeah exactly.

C

B

Is telling me that that, um like I'm thinking, n plus one and that sort of thing, but I I guess every little helps to some extent but um yeah. I I don't feel like that's a huge concern in this instance, but I'm open to so so.

C

To vote the uh the guidelines- and actually it was new for me for a couple weeks ago after coming back from parental leave, um so when using plug, sometimes it's not unnecessary to to to allocate this amount of memory. If we can avoid it, we should avoid it.

C

So it seems it's better better to create a single query using a subquery as instead of doing two queries, fetching ids for the first query and use the same ids for the second query and the amount of memory building allocated so, for instance, for a huge amount of ids, it could be uh significant to to to allocate a lot of memory for those ids.

C

This is, I think, the main argument to to avoid plug in certain situations. Again, I'm not sure if it's if it applies to the perhaps.

B

Perhaps that's something that um I'll tell you what I'll do is I'll. Add a comment now to the mr to sort of say: oh, um like we're we're thinking about this, so we don't forget about it, but we'll continue going through and we'll see when we kind of come full circle. If, if we make a decision.

C

But I I didn't want to interrupt the floor. So uh sorry, terry, oh.

A

No, that's fine. I was looking for the. um I wasn't able to find it. So thank you for sharing the link in the chat.

A

um So I will move on to the service.

A

So it looks like we're grabbing.

B

Contact yeah, so I'm going to avoid like feel free to tell me this is wrong, but I'll avoid saying anything because I think the beauty of this is that if it's not self-explanatory, then- and this there's something not great about the code so I'll, let you.

A

A

If we want to have this thing uncomfortable, you know, if you want to.

A

A

A

I guess we're working through these one by one.

B

Yeah, so we could potentially, I think, that's where the whole plucking stuff maybe comes in um it may well be that we could optimize and and write some more raw sql or like some bulk insert technique, um but I guess more than anything we would lose this um validation um that occurs when we manually or not manually, when we try and individually yeah exactly. I.

A

Guess my only wonder here is kind of on the same line as maybe what peter was saying is: if there's a lot of ivs, could that cause us so.

B

Did you see the very first change at the top of this um service.

A

Not this one, this one.

B

Do you know what it wasn't I did it before and I completely forgot it, but uh on line seven there we've actually got a max okay, okay. I thought it was a change here, forgive me, but I included that in the last, mr so yeah with the maximum. I guess that somewhat alleviates.

A

That concern okay too many, like you, can't add more than six.

B

A

B

I don't know what they call it like: a denial of service or an attack vector or.

A

Yeah yeah, if you let people do things, they will do them. That's all I don't know I feel like that's like from their back end perspective. If you let people do things, they just do them because there's like there's, no, of course, there's allowed so I'll, do it, but, um okay, so that's kind of that uh concern. There.

A

And that's already like happening right. This is, is this behind a feature flag or anything or is it yep.

B

Daisy date.

A

So it's not being used right now. Yes, correct.

B

C

Maybe we could add a label to feature flag.

A

C

Yeah, so I I tend to use it, I'm not sure if it's the real intent of the label, but I tend to use it where ever I I introduce remove or actually keep using a feature, float flag or developing a feature behind or center flag.

A

Yeah or at least or something I know you probably don't. This isn't quite ready for like the review, because, but I would at least put in the description like this is behind the feature flag so that yeah. So what.

B

I try and um normally do is um on the steps I would put on the recreation or how to test. I would put like enable the feature flag, create some contacts like this and post some sort of um code to actually create some contacts and um then yeah.

C

I've got a question about him: sorry about the maximum amount of contracts to be added. So how do we communicate with the user that they are allowed only allowed to to add six.

A

Oh in the in the ui or just in general,.

C

B

So you'll see that there is a um uh and or at least I thought there was an error, but maybe I've um messed up when I've resolved my conflict somewhere. They should oh.

A

B

Yeah, um we might.

A

Not have it exposed so.

B

Yeah, I think that's all. It is sorry yeah, so error too many, so we but we do um that, does get exposed. I think it would be a nice thing when we start writing docs. I guess we want to include it in the docs.

C

Do we um is this error message being propagated to the ui, actually just checking so.

B

Yeah, no, it's a very good point. It's a very bad! At the moment. I basically say something went wrong or it was successful. So that is that's another really good one to discuss and understand. That's something! I think terry. If, if your memory is a little bit better than mine, that we touched on and to sort of say like as the first iteration like yeah, how good should we be really explicit and say we added these people? We removed these people or or is it okay to just kind of say?

B

Oh it worked or it didn't work. You know be quite vague, but um you're right, like um I'm happy, if we sort of say well actually, if something went wrong, we do get a good error back from the back end. So why not just push that to the front end, um we might need to talk about internationalization.

B

I don't know what the um thinking is there, but I'm I'm happy to to sort of.

C

Yeah yeah: these are good points, especially in this case. We already have the error message, message being compiled or used in the back in the back end in the service yeah and in in the front not front and in the used code in the quick action we could it it's. It is actually as easy as using result.message in that case. So.

B

Yeah, so can I can I internationalize that by putting it between the brackets or do I need to do something cleverer than that or do it could we should I not internationalize it or.

C

I, in that case I I would, I would do in internationalized, so every every uh content we display to users should be internationalized. Exceptions are, for example, rake task, error messages, something like that. I I'm not sure we are doing that, but in this case I would do it. um The only trick we have to do is for the error to many uh message.

C

We can't interpolate the uh the constant here, but we need to do like the uh formatting stuff, where we pass the uh max additional context um as a format input. So I'm not yeah. I'm.

B

Trying to understand how this would work, uh I think terry did you.

A

I shared a link that has the ruby files and I think, at the very bottom of the ruby section it it says, be careful when translating strings at the class or module level since they're only evaluated at class time. So I think what uh peter's saying like the con, the constants not going to be, I think the ones that are just straight strings, I think, are fine.

A

C

Yeah right and the last one online, uh 87 or 125 pick one so this I think this one we need to to uh use. I'm not sure if it's documented actually.

B

So we so so I think I think that I think that's answered my where I was getting a bit confused, so we can't so we have to do the internationalization in set crm contact service. We can't do it in the issue. Actions gotcha.

C

B

C

These, in the end, these uh has they have to be static, strings which are actually passed by by um by process by by rake task in order to create the profiles or pot files or whatever. So uh we cannot do dynamic. You cannot do underscore braces and then dynamic string. This it doesn't work. It has to be a static, static string, yeah, dynamic string, meaning passing a variable you. You cannot do that.

B

C

You you're, I just found the format stuff, it's actually this very same page, terry shared but another section, speaking about how to actually format doing uh yeah formatting. So it's it's the same. It's not as complicated in this example, but it's actually using oh it's all. We have even interpolation. So okay interpolation is different right. No, it's the same! Okay, yeah interpolation! It's section, interpolation,.

B

C

And yeah, you are right, you sh, you should do it where the actually messages, uh error, messages or whatever are are created or emitted. So it's.

A

C

A little bit weird to to have it in this service, but yeah you cannot do it dynamically.

A

So I think it will be something like this.

B

B

Like that awesome.

A

I'll just add that as a comment, so we don't lose it and maybe throw this link in.

B

There that's really helpful. Thank you.

C

Rather unrelated question, why is message a list of strings just curious.

B

um So I think this is maybe a topic of debate, point of contention, whatever you want to call it, and if I was a bit better prepared, I'd, have an issue link or something for you, but there's this seems to be this more relates, I think, to graphql than anything um and there's a bit of. uh Is it supposed to always be a an array or a list of strings, a list of errors, a list of messages or is it can?

B

It is acceptable for it sometimes to be a string and sometimes to be a list of strings, and we were sort of airing on the side of well. If it can sometimes be a list, let's always make it a list.

C

I I fully agree on that. I'm not sure if it's the right place to to create an array actually so because, when looking into the service code itself, you don't know the users, so you so from from the service perspective, it only makes sense to to return a single message, because you can we don't have we don't support multiple response service responses. We just this is just one response.

C

Well, it could, in theory, have multiple messages, but in this case it doesn't make really sense to to create to to return multiple messages. So um if we want to have multiple messages in graphql, we should do it like on on the graphql side, and I I've seen some code doing actually doing this right doing like 2a or something like that.

B

Yeah, do you know what I could, even if I, if I really hadn't, wanted to do this, I could have even updated the service response class itself, um because I think I've done this somewhere, where, if you call a method, I think it's just called array the method and you pass it either a string or an array. It will. If it's already an array, it won't do anything. If it's a string, it will add it to an array. So um not.

C

Talk of use with array.wrap, which is a race method which actually wraps always raps.

B

So, even if it's a radio in a right, yeah.

C

So not to confuse just saying sorry, um yes again, this is like I totally uh easy to to get sidetracks on that. uh So I I I.

B

A

C

If we try to establish like a pattern where we actually want to return a list of messages, always um yeah, I feel this should be like, like very consistent across the database data code base, because currently I.

A

I've never seen that.

C

A

The example for the service response right now that I found in the docs, which is probably not the only spot, it's used, but it definitely has one message.

A

Well, that might be confusing. um I'm.

C

Always asking okay, if I ask questions how many uh other folks will.

B

No, it's really good and again, I'm gonna. Add I'm gonna. Add that now, um because I just want to make sure that, even if I don't change it that you know we, I find an issue and I reference that or if.

A

There is a discussion somewhere that would be good to yeah. Like you said, the whole visit to reference.

A

C

So doing a quick grab during through our code base, I only see a couple of instances in the production code actually one other instance which uses a array or a list of messages and a couple of in specs. I'm not sure I understand this one, but it may be something different related to some other behavior. I don't know so it's it's a very new pattern, which is not good or not bad, but just stated.

B

C

Fact that it's uh it's, it's not already being done, and I missed it for for some reason so and as terry mentioned it it's not mentioned in docs, and uh it's it's a good discussion to have and a good problem to solve, to have multiple messages in graphql and I've. Seen the as you mentioned, the array, stuff era message wrapping um somewhere but um yeah, I I would be just curious in my match in the review.

B

Absolutely it's just exactly it's really helpful and what we're trying to achieve so pop that on and I'll yeah I'll definitely take a look at tweaking. That.

A

um You know- and I know we're still on this page, but what's the code review, so it's probably a lot slower and more in-depth than normal. um These, these uh errors are when people are trying to add or remove too many contacts at once. um I guess my question would be so. We are.

A

Removing the existing contacts from what they've selected um before we do the operation on that does that have any impact? Let's say like I select 10 contacts and you know six of them already exist and I want to add- and I say I want to add all 10.

A

um should that be allowed because then technically it would only be four. Does that mean.

B

A

B

That, just like.

A

Two crate, I feel like that, wouldn't be a good first iteration, but I think it's some.

B

I I think, let let me just have a look. I've got. I've got the service up here myself, I'm sure I have someone that I think. If you specifically say I want to add these 10 yeah, then more, not more, for you, that's a bit harsh, but then tough. If you say I want to set the contacts on this issue to these ten yeah, then it actually calls the method that determines oh there's already, these six people on there we're only adding four and it will allow it.

B

The flow so look at the determine changes method, and that is probably the one. That's really important here so determine changes. If you don't.

A

Know you set them okay,.

B

Yeah, so if you call this um service, but instead of passing and saying, I want to add these five people instead, you said set it equal to these five people. Then it then says: oh well, we've already got these four on, so we're only adding one and then that should should be okay.

A

That makes sense.

A

A

I want to try to trace this through. I am going to give. I am slightly awake, but this is a rather large, mr, so I probably would have waited to do this until the afternoon this. um I guess this line that's happening, so I'm going to trace this back up. So we have add contacts by id here with this crayon, which I guess I'm wondering. If is this line needed, or is it already taken care of here when we're passing.

B

A

B

Am I like completely.

A

Reading that wrong.

B

Yeah yeah, so what you can call this method in three different ways or probably more than three different ways, but you can provide um the the parameter, um crm, contact, ids without a prefix of add or remove, and it's it's the same as adding labels. Actually like the on an issue labels, you can do add labels remove labels or you can just say labels.

B

This is the same and handle determine changes is only called if you say, um crm contact ids, so that one is like.

A

It's setting these parameters that wouldn't be set if they just use that quick action. Okay, that makes sense.

B

A

I've actually never seen the code that handles any quick action before so. This is good.

B

um It's it's something. That's um you know I. I would like to see a an improvement, a better pattern, because it does seem like a lot of code, a lot of jumping through hoops. Quite a lot of um I don't know what you want to call it. That should be quite. It should feels like. It should be simpler to do all these little like.

B

Oh, we want to be able to add remove, but we also want to be able to set or replace or whatever we want to call it and all the kind of little bits and pieces of logic and test checks that come along with that.

A

That makes sense. Okay, I don't have any other comments right off the bat of this one thing, I'm missing, is I really like it when it has the little testing coverage thing, and I guess that's not on there, because the pipeline didn't complete. I really do rely on that. Sometimes.

B

Oh that's interesting.

A

um It'll be green, highlighted in red if it's not covered by testing. So that's really nice. When we have it.

B

So um we'll probably look at the spec in a minute. I I think that I've got good coverage on the service. um What I haven't got any coverage on at the moment is the actual quick action um itself. So I I don't know that I've got any questions at this point in time. I think the onus is just on me to crack on and write a few, um but- and I wanted to cycle back through.

B

Forgive me if I'm breaking your trailer for here, but I wanted to cycle back to that array thing that we were talking about.

B

So what I have noticed is- and this might only be me that is doing this- it might not happen anywhere else, but um I think that peter, maybe you had a quick look to a bit of grippy action to see if you could see if we were ever calling service response um error with with an array- um and I wonder if, because I can see in this set crm contact service at the moment, there's an instance where I'm passing it like the model errors.

B

So I'm calling serviceresponse.error and if, if we want to visualize it all together, it's on line 32.

A

B

A

B

So that might be an almost my answer when I actually take my brain away and try and look in hard and say well, okay, this is a scenario where I'm returning an array. um I think I could just call doctor sentence like I do in the the thing above it. um I don't know whether it's good or bad, in a way, I think an array might be better because something on the other end might be able to handle that better.

B

um I could be wrong, though um it might be like let's always return a string rather than let's always return an array, but I think that's probably what happened. I I had that array coming back from that one and wanted to make sure that they all therefore returned an array.

C

Yeah, that's a good good one. I I actually could look up other places where we are actually doing two sentences in those cases, because I remember from at least from from our team reviewing some code and actually creating some code, which exactly does uh two sentences or yeah.

A

I found I found one where they do a two sentence and.

C

Okay, um okay, maybe it is us.

B

But I mean it's it. You know it's it's one of those where, like I, I think that it's it's great, that's having a little bit of to and fro, but uh I or one of us say take away and see if we can find an existing issue or share it in slack and see. If um then, we can just improve the docs right and and sort of explicitly say do this or don't do this or.

C

Yeah, especially if it like, like having an array, would help actually, uh for example, presenting error messages in in other places like graphql as you mentioned, would be totally great too, and we should discuss how to realize that. So how actually to pass a list of messages messages, should we use message or should we find another way, maybe some some good way for service response to handle?

C

B

So maybe there's still just one message, but there's like an array of errors or an array of details or something yeah.

C

B

C

Point we would need this uh place, we you would do a time in place. We would need to think about legacy code as always. So uh so, what's what if we change stuff? What's what impact do we have on or unexpected impact, even on yeah in other places? I've? That's that's a good one, so um I just wanted to before moving to another uh code or class. I I just had a question about too many methods.

C

um We are checking so too. Many uh is a composition of too many too many ids. So if, if you go to the definition of a too many methods.

B

We're I'm sorry one line. 104 is terry. Sorry,.

A

Oh okay, I thought you were saying. There's too many methods. I'm sorry.

A

B

You you could totally argue that I'm I'm for that and I've. Actually, I did go a bit crazy from the beginning. I I like I'm not sure why I did it. I actually quite like um this kind of super verbose way of splitting off little methods, but I don't know some people could be against it for sure. Oh.

C

No, I'm I'm totally fine with that. Actually, it's not too many, I'm also preferring the uh very terse.

B

C

Yeah right, it's: okay, uh yeah! I sorry I was talking about the method too many questions.

B

C

So it checks too many ids. This one. We discussed um how it actually checks or validates or does the subtraction of existing ids do is do we have a similar.

C

Computation of contact emails, meaning subtracting the existing contact emails as well.

B

Not if I'm interpreting that correctly, because the quick actions available at the moment are just add and remove, there's no set available if that makes sense, so the my sort of understanding or expectation for this first iteration is that you know, if you want to, you will be very explicitly saying right. Okay, I already know who's on this issue. I want to add these ones or I want to remove these ones rather than it being. Oh, I don't know who's on there or I don't care who's on there.

B

I want to change it, and all I want left is dave. Okay,.

C

B

C

I I I'm not sure this answer well, I maybe I do not understand the the uh intent behind um removing the existing contact id from the contact id list. We are doing it elsewhere. I think it was up. We are not doing the same the same stuff for for email. So I'm wondering why. So if you go to the execute method, you you'll see that yeah at line 16, we are fetching all existing contact ids and with that we are using this list to actually uh remove the existing ones from the past ones right.

C

B

B

C

B

Good point: let's have a look so.

C

So, for example, for example, if I uh use the quick action and add the same email address twice.

B

So um yeah, so what happens is there's there's a couple of um so add contacts by email wraps, add context. Get my word. Add contacts by id, yeah and I'll add contacts by id removes the existing ones.

B

So the actual code does do that, but I I didn't think it was worthwhile doing all of that effort before the check, because I don't know, I think it's yeah, I'm going to call it expensive. It might not be that expensive. But again I am I'm sort of pushing the onus back onto the user to sort of say.

B

Well, you know we're limiting you to six and you know if if you're gonna, um I don't wanna, I don't wanna be mean to the user, um but if you're gonna try and add 10 people, seven of which are already on the issue anyway, then tough um like. If, if you need to go to the extra effort of looking at who's on the issue and just adding the ones that you actually want to add.

C

Okay, but you have like the specific use case of quick action in mind with with your assumptions right now right so, but as far as I can, I understand the service could could, in theory and in the future, could be used by other folks so um or other places where the user maybe is not involved.

B

So yeah, so what we would do, then we would expose another method or another parameter. Another argument, so where we've we've now got five on this method, so we've got um crm contact ids, which is a replace operation.

B

um That's it. We've got ad crm contact ids, which is a sort of append type operation. We've got remove crm contact ids, which is a remove action. We've then got add crm contact emails, which is an append operation and remove crm contact emails which is removed, but we don't have a replace operation for emails because because I'm not exposing a quick action to do that, I've not written the service method to do that. But if we um that's how we would achieve that, does that help or.

C

um Yeah, I was just just by reading the code and maybe adding a documentation to just to to the execute method, which would be actually very helpful too.

B

So so you're you're sort of almost reading and saying well, we currently do this for id, so why don't we do it? For emails is kind of yeah.

C

Yeah this is this is one one question. The other question is uh was what my suggestion would be to actually add some code, documentation to the to the execute method and describe all possible parameters, because.

A

C

Don't here yeah yeah, because it's um or not execute in that way, it should be like the the initializer right, because it's so because you did a great job explaining me all the different ways of of uh changing contact. Ids- and I wasn't aware you- you cannot replace emails right now, so maybe it could could be useful to hide yeah.

B

Yeah yeah, no, I I see exactly where you're coming from. I wonder how I can um do it in a nice succinct way without too much waffle or something to.

C

And again, by just reading the code, I I was surprised that the that we are doing the.

C

Subtraction of contact ids, but not like the um intersection right of contact ids, but not the emails. So it's.

B

Just okay, so so um I'll just write something real quick. While you guys are looking.

C

Maybe it's totally obvious for everyone else, but no.

B

C

B

This is all really really valuable, and that's um I do to appreciate is me trying to get this down into something. That's clear, concise and sort of yeah, so I'll, just type something real, quick, I'll drop it in the chat and we'll see if it looks any good.

C

For some reason, I'm I'm very good at pattern matching and like even without understanding the intent. So my my peta match uh like spotted okay, there's something different, not odd or wrong, but different from why? Why is it different? That's why I'm asking the questions so.

B

So I I think it's probably fine in this instance as long as we make sure we try and update the the comments, if and when but like what. What I was going to say is that through graphql, we can do this through quick actions. We can do this and then sort of explain a little bit more around that, um but we, I guess we just got to be careful- that if we change any of that that the the comments get updated to yeah.

C

Yeah, that's maybe I wouldn't wouldn't try to explain who uses it but like what or best.

B

C

I suppose that's not important.

B

No you're right. I could just say that at the moment we can add, remove or replace contacts by id and when replacing them, it's necessary to do this, um but at the moment we can only add or remove by email. So it's not necessary to do this.

C

Yeah or maybe even the the last sentence would be sufficient saying replacing emails is not supported, yet that's why we don't need to calculate like amazing.

B

That sums it up perfectly.

A

And it'll be recorded, so you can go back and watch it.

B

Get it written.

A

B

Oh god, then I have to listen to my own voice.

C

I don't do that, usually so not to your voice to my voice as well.

C

Okay, bye again sorry for for derailing.

A

No, no! That's! Okay! um I guess was there anything we're coming on the top of the hour? Is there any spots that you are very interested in looking at? Besides what we've covered already, because I feel like we didn't get through all of the mr but yeah. I feel like an mr. That's got this many files in it. It would take me a little bit of time to review so.

B

So I I suspect that that's where the me is um in the service, really, um I don't have I'm sure I should have the time in the corner of the screen. I don't know if you want to just jump into each other file and just see how like clearly, the translations we're not interested in um yeah.

A

I know you mentioned this project, uh I don't know if we have a chance to look at this, but I think I've.

B

Undone that I've undone, that oh you.

A

Understood it: okay.

B

um But but yeah that's if we had more time, I would love to talk about that, because I'm I'm really confused um by this and I'll I'll I'll go I'll, be a good citizen I'll raise an issue or I'll drop it in slash and see if anyone's already looked at it and then raised an issue.

B

Project already seems available in certain contexts in this file, but not throughout the entire file and my brain can't comprehend how or why that is happening, and when I yeah, when I create this definition here, it causes some issues. So I've just reverted it for now to make life easy. Okay, um yeah.

C

I'm not sure I fully agree. I also stumbled across quick actions or the dsl syntax or the actually, the mixed syntax of dsl and defining method in the quick actions sometimes also confusing, and I also lost couple of times like the scope of or I wasn't sure what in which scope am. I am right now so, for example, for me, it's uh really difficult or strange to see like a dsl syntax like, for example, commands remove contacts.

C

But yeah later you see like at updates in line three three or five or or using an instance variable accessing the instance variable it's it doesn't that doesn't compute in my brain, because this l is it's another layer of abstraction, a higher layer, a layer of abstraction or.

A

Definition and an.

C

Instance variable is very implementation, detailee, and so it's it always confuses me here, but I haven't got the time to to wrap this things up in my brain and to create an issue or even start working on it on a replacement. So I'm just took here to complain.

C

Did I feel you.

B

So I'm just sorry my brain can't multitask, so I'm just typing: what's going to tag you.

C

I one one last uh question I had was about the actual parameters to the service. They are called ads, crm contact emails and the word crm is always there is do we need to distinguish like, or do we need to to have the extra words crm to to activate.

B

C

B

C

Because look looking at this, so the the the word.

A

C

Is already contains contained in the service, so not sure if we should add it to every parameter. So I'm.

B

Finally, contact.

C

I so adds contact emails. It's I think it's fine, one could argue even add. Emails could be okay, but I'm fine with using the contact. Emails. Prefix, it's it's! It's the same for the contact ids. I think it's add crm contact, ids right when you look yeah.

A

I think it's following the pattern: that's established in the in the um in the existing code.

B

You both happy if I was because looking at it now in its context, um I I think you're you're right.

B

I was initially I think, looking at it, and I know that everywhere, where I refer to this stuff, it's really important that I prefix it with crm, because although we don't have any contacts in any other context, at the moment there might be going forwards, um so I've kind of been made to make sure asked to make sure that I do kind of classify or specify whatever you want to call it, but you're right because we're in the set crm contact service, um I think just add emails, add ids, so um remove emails, remove ids um the only one that would be a bit harder is.

B

I think I could change crm contact ids to just set ids or replace ids, or something like that. Yeah.

A

C

Really, like the.

B

A

Because that's like really clear about what that's being used for um yeah my.

B

A

At this, that's a really good comment, peter my initial look at this is like I like it when things are same, and so this is only adding to things that already have crm, so it necessarily wouldn't have like jumped out at me. But um it's a good comment.

C

And this is obviously non-blocking, so it's I would. I would uh create a follow-up if, if at all, you could also argue and and say, okay- I don't want to change. I would be fine with that. So it's kind of nitpick for me because.

B

C

I've clearly got.

B

More work to do on this, so I it's as easy for me to to tidy this up um now as it is at any point. So no, I appreciate and that's why I again, I know async world and all that, but um these are really really valuable sessions to me, because it sort of not only gives me the feedback I can start applying it. I can ask you how about this, how about that and and got quite a strong way forward to to crack on this evening?

B

um Have you just got a few more I'm just writing the the slack message um so just make sure you guys um that makes sense if you've got just a few minutes. Yeah.

C

Sure I I already put something in into slack, so I basically copy-pasted the zoom chat.

B

uh Did do you put it? Is it a channel I'm likely to have access to alex oh yeah.

C

Right, I keep forgetting sorry.

B

So where, where did you put it out of interest peter.

C

It's in the pairing uh beckham pass.

B

Empires, okay, um sorry, would you I don't know what the best way to do. I tell you what, if I share in the chat here, real quick, what I had started to type and if you see if it looks vaguely like what you were asking, um then I'm happy. I guess.

C

Okay, yeah, I can. I can try to retype it.

B

No, I mean yeah either share share across or, like you know, there's no clever way of overcoming a lack of access. Unfortunately,.

C

Well, this, I think the this question is generic enough enough to to be asking in the back engine which you don't have access to as well right. I.

B

Do have access to the generic backend channel yeah yeah.

C

I think this could be an action, uh a question being asked in in this channel. So it's not too specific in the back and I I can share your message in the back and repairs to to crosslink and so on. So.

B

So, do you want me to post that message in back end.

C

Yeah sure it's.

B

Oh, I think it's.

C

B

C

It's it's a relevant.

B

I'll just pop it in there for now. um Okay, brilliant, I think that's really helpful. So I'm just gonna remind myself where, uh where we got so put the comment about maybe just making the parameters a little bit more succinct, um the error messages, um I think I'll probably keep them as um or sorry. The message, not necessarily error, um keep them as an array for the time being, but definitely raise an issue or try and find an existing issue about it and um like work out you know, is it good practice, bad practice?

B

How should we move forward with that um that I've reverted and we've started a discussion for it? So that's awesome. I need to add some more specs and.

A

I think you, if the specs pass after removing that project, then you're, probably fine. I didn't look into the failures in detail.

B

um And then the little comment I was adding as well replacing contacts by email is not currently supported would did. I need to elaborate a little bit on why I've put that comment in there. Hence, why we're not determining changes or something like that or or is that sufficient? Do you think.

C

Now having the context for me, it would be so sufficient, but we will we will try. We will see how the reviewers will handle this comment. So.

B

Okay, brilliant: um I will uh push something up uh I'll tag, you um terry, is there anything? Will you like later on today, look for any more of this, or do you think that that's that's you've kind of done everything you need to do really anyway, but um once I've finished I'll pass it back to you for kind of uh a more formal, sash official approval, yeah.

A

I'm happy to get another review through and then I probably pass it to a maintainer afterwards, because I like having the two review process and I don't mind doing the initial review I will say um depending on when you get it to me- I'm probably gonna stop taking reviews sometime hold on what date is it today sometime around the 22nd, I will not.

B

It'll be tonight, I'm sure it will be okay.

A

B

This week is fine, yeah amazing. Thank you so much. Thank you and look forward to speaking to you later in the week.

A

Have a good rest of your day, everyone.

B

You too, bye-bye likewise.

A