Magento Triage, 31 Jul 2020

Previous Meeting Next Meeting

⏯

youtube image

►

From YouTube: Community Contribution Triage 07/31/2020 (Dev.Experience)

Description

https://github.com/magento/magento2/issues/25955
https://github.com/magento/magento2/issues/25937
https://github.com/magento/magento2/issues/25889
https://github.com/magento/magento2/issues/25413
https://github.com/magento/magento2/issues/25111

A

Let me share my screen.

A

Yeah, for today, we prepared five issues related to that.

A

A

Yeah first one is about about the additional plugin for order to bar block.

B

Can I ask you to share link for each ticket in chat? It will be also useful if.

C

You can copy link from your document and post to our chat here. Thank you.

A

There is all the link to all tickets that will look today: okay,.

A

Yeah looks like contributor tried to add.

A

A

B

I see that he tried to reuse some select query, but honestly, it's not clear all of steps, because I see that with joint table, it's good but picks and displays the true number in amount.html honestly, I can recommend to ask more details, especially about precondition, because now it's not clear how many products do you have?

B

What is conditions, because current actual result is get only one item of product, but it's not clear. Why only one? How many do you have? Is it separate model, or so I think we can.

C

Remove label and guys, like one uh one thing that I have in mind, so we have like a toolbar collection. We need to actually double check.

C

What is there, but we are adding joins like a for the sales order, and it's like a red flag for me, uh because we do support like a split db and like a hosting of the sales uh tables on the like a separate database server, and if this collection, toolbar collection, pulls the data, like I say from from one server and sales will be uh hosted on the another server technically. This will be you know. Fatal error so, like this join is, is not so easy here and we need to double check, split db deployment.

B

Yeah, for me it looks like some kind of misconfiguration, so let's ask guy more details and what label is needs update.

A

Yeah I agree. I already removed the confirmation label and.

C

Voldemort, could you please add comment about the split db uh just in the pull request, so that we won't forget this before.

A

C

Guys, maybe you have any other comments, ideas about.

C

A

Okay, I believe we can go to the next one.

D

uh I would say that maybe we also uh sorry we need to check how it's going to work with the staging functionality, because I can see that we have some linkedin with entity id for product.

D

So I would recommend to also check. I guess we need to double check this like, but this bark for uh our enterprise edition.

C

So anyway, I believe it should be covered, uh but okay, but by the test, it's uh like a at the end, uh but I mean so when our test should catch uh such stuff, but yeah good point. It makes sense to double check on this as well.

A

I believe that the contributor tried to do his own implementation, his own plugin and for his own project, and maybe he don't use staging and he don't need to know, ask about the staging, etc.

A

That he don't propose some solution, it's not solution and we will not deliver this code to magento to a code base. He just mentioned that we he said he have some problem when he write his own plugin to the total to the bar collections.

A

Okay, so yeah we will not deliver. Is this code too much of the tools so.

A

We don't need to check it with staging et cetera.

E

A

Just need to clarify why it's not work correctly when we write some custom plugin.

A

Okay, let's go to the next one and it's a pretty small issue. As I see there is a message, error message, as I understand your validation.

B

A

Looks like yeah and thomas proposed to replace messages say to website is required where you need to website. It is a quiet value because it looks like.

B

I don't think that we need to move to some feature requests. Yes, according to this ticket, we need change this message and okay. We can say that it's bach, but for me, priority is p4 or s4, because in generally you can understand what is happening from previous message. Yes, it's it's good. If we will have some more the same message as it uh done for another field,.

A

I'm not sure that it is a real bug at all because yeah for us maybe a website it is more readable, but for some other people, maybe a 68 website is critically europe. I don't know it. We use website id in our terminology in our daily work, so it's preferable for us, but for some people outside of our company- and maybe a society website- is also pretty clear.

C

I mean, from my standpoint, like a website ready uh again we're talking about the api right now, it's not a ui um correct. It's not like.

A

C

So uh from the api standpoint, it makes sense to point out specific field uh which is missing, which, like for me, has much more sense for uh ui validation and just a messaging definitely associated website has more sense, but we need to double check uh if this field used only here and these type of messages I mean only on the api or we are using the same logic and same messaging for somewhere else in uh admin ui so like there is like a two different clients of this message: programmatic and human, or store admin.

C

So if it's not only about the api, it makes sense to change. If it's about the user.

B

You see that it's registrations, so it's only api because, if you use, if you try to register via ui website, will be assigned automatically based on website, whereas restriction happened.

C

Valerie, I wouldn't be surprised uh if we have the same model or validator class, which generates actually this message.

A

Yeah but as I recognize that from the ui, we don't need to pass any website id and we will never see this error.

C

Yes, that's right, so again it's about issue if this issue about the api integration, this absolutely makes sense for me, but during the implementation we will need to double check that we are not affecting any uh like a user based messaging.

B

We can put command for this for qa stage like please verify that we will not affect trigger station ui.

C

Sure yeah make.

C

B

Now you can recopy this input and it.

A

Happens: okay, guys they have any ideas of it or we can proceed to the next.

E

One, my only one concern is that any kind of messaging exception messaging. This kind of messaging, is the part of our api, and it will be a breaking change to change that.

A

Yeah, but I believe that we can approve it, we can try to approve it and deliver it to them.

C

Elena, you are saying that the message itself text yes of the message.

D

C

Yes, all right.

D

Yeah, someone killed your yeah right now. We have like policies that all translatable uh strengths should be changing of any translatable. Strength should be considered as minor changes, because it can break customization uh and, for example, some themes or some logic which was built based on um on the stream.

C

Got it yeah, it makes.

B

B

So we can move to next.

D

B

I know this issue, it's very very looks very promising. I recommend to pay more tensions on it.

B

So generally, it was changing the logic of collection, loading and honestly, it's very very interesting result like decreasing time from one hour to a couple of minutes, so it looks like maybe something was broken, because it's so significant improvement.

B

uh That's why we already asked to review this issue. I I'm sorry to make some additional checking of this issue, because for me it's it's very great, absolutely great result. If it's really true.

A

Yes looks like it's looks like this small changes: it's not possible to improve uh performance, so so, let's see numbers yeah, but we already asked to reject this issue rtg guys. So we are waiting for results from the.

B

So after checking out the internal qa, it will be final decision, but consider if it's true for me, priority is at least p2, maybe even p1, or we set p2 s1 for this, because it's related to performance and if it's through the level of delivery.

A

This one because yeah it's improvement, it's performance improvement, it's important, but it is it's not a an issue. It doesn't block anyone. So let's do p2 like pre-priority and s1, because it's really important performances.

B

Honestly, I put this ticket also in my uh some read me in some txt file, because I would like to check this himself. It's very interesting result. So, yes, we changing just only logic how to load this object. uh Maybe it's related to loading of some plugins. Maybe it's related to loading of some. It does not perform an additional logic, but in general there are was changed just for each replace it on while cycle and according to loading, we started to use fetch item instead loading.

B

So it's very interesting, so it's not possible that uh replacing for each while will lead to such kind of results. So maybe there are some broken logic, maybe maybe it's related to something else, but I would like to make double check from qa side also from some technical from engineering side.

A

Yeah pattern is almost real query, but yeah improvement is too much.

B

So but yes, I think we need to check so maybe some suggestion from another.

A

Guys, okay or not, let's proceed the next one.

A

Better in static test framework depending dependency, whole tests.

A

So contributed prior to one php world test and the expected result that all tests should be passed, but contributors receive some errors.

A

Yeah, it's real errors related to the puzzles.

B

Sorry, I cannot understand, is it related to issuing static tests or is it related to some issue in our ac module.

A

It's related to static tests.

B

So, let's clarify steps, so I install magento via composer after that I try to run static test so expected result that all tests should be passed. Yes, it's it's correct and actual results that we have some issues with siemens.

B

So, yes, we have some tests, we check your dependency and what is here.

B

That we have no such exception.

A

I believe that if you will run this test from the roots folder instead of the data static, primo test unit, everything will be fine. I think the main reason that we run it from the unit folder with yeah provided possible.

A

B

Back, I think uh ac.

B

Honestly, I am, I don't know, can we run our static test if installation was over composer and code is located in a vendor folder, because I run it mostly when you clone code from github, but of course I think yes in general, we are going to improve our tooling, so it will be great. Also if you, if you test, will work with swandersu, okay, it looks like bug, but for me it's b3, maybe s3, maybe even before I did not be free before.

A

I think we can set this real pc for this issue, but I really I for sure that we have ability to run this test. uh We should play a little bit with the puzzles uh from the which folder we should write this comment, but I believe that we have some work around.

A

I don't know: okay, let's do p3 for this issue. This way, s3.

A

Guys do you have any suggestion.

A

Okay, if not, let's go to the last one.

A

The performanceation product page is all product type. Okay,.

A

Go to any product and review all sql for this okay, only one time. Okay, the question is.

B

A

B

It's related that we have some mysql query in this cycle. uh If you're familiar, let's call it one, plus one n, plus one problem.

B

uh It's related that we collect all of ndt5s and if I understand correct, I propose to collects all of ids. In one query: we have pretty familiar issue in graphql, but in graphql it was more easy to fix it because we can collect in some parents resolvers and need to make this in cycle.

B

So, according to this issue, you can say: yes, it's problem doesn't matter enable it cash or no, because it will be reproducible only if our cash will be enabled, uh but the amnesty easy to implement in current implement in car in current codes.

A

You think it should be implement.

B

A

B

Something like that, you should collect all of this and make one query, but you should not do it in the log. Could you open this layout, plugin and product list.

B

That's what I call it that we.

B

B

B

uh You know in current structure: it's I don't see how to fix it without some using register or some global state, because it's child blocked, and it will be strange if parent blog will be asked this data in chat book. Okay. Now we talk not about implementation, but mostly about priority. uh For me, this issue requires some hld, maybe some design, maybe some additional recommendation.

B

B

For me, it's p3 s3, not p2, because it will be not least to some significant signature, significant improvements. Also, it will be reproducible only if cash disabled. It's it's my proposal, but.

A

I prefer to set a s2, because it's also related to performance and.

B

Okay, okay, maybe.

A

Increase the severity of this issue, at least at least investigate how I can figure out with it. I really I don't understand what you mean when you say that we can't implement some some so bulk operation. I mean I don't understand why we cannot do it yeah, but yeah. As you said. Currently, we talked not about implementation. Just about the issue and yeah I mean prioritizability is correct uh the case. That's it from my side for today.

B

It's good because we uh started this meeting a little bit late due to maintainers calls. So we know we're out of time.

A

Yeah, thank you guys for the participating.

A

A