Skip to content

OBPIH-6707 Fix quantity shipped indicator on edit modal when a brand …#4923

Merged
awalkowiak merged 1 commit intodevelopfrom
OBPIH-6707-fix
Nov 5, 2024
Merged

OBPIH-6707 Fix quantity shipped indicator on edit modal when a brand …#4923
awalkowiak merged 1 commit intodevelopfrom
OBPIH-6707-fix

Conversation

@kchelstowski
Copy link
Collaborator

@kchelstowski kchelstowski commented Oct 31, 2024

…new row is added to the table

✨ Description of Change

A concise summary of what is being changed. Please provide enough context for reviewers to be able to understand the change and why it is necessary. If the issue/ticket already provides enough information, you can put "See ticket" as the description.

Link to GitHub issue or Jira ticket:

Description:
The comments before calling the groupShipmentItems sort of describe the issue that was caught by Jakub, but in a nutshell - constructor is called only once, so in the constructor I tried to rely on initial values of the form (original qty), and then, while opening a modal, relying on the current values of the form. The problem was that when we add a new row at the end (e.g. we edit the last item), this modal has not yet been built in the background, hence when creating it and having already edited an item (with decreased/increased qty shipped), the new row would look at it as the original qty and the indicator would not be shown in that line.
To fix this, I need to store the initial receipt candidates values separately, not to rely on the values of the form in the constructor.


📷 Screenshots & Recordings (optional)

If this PR contains a UI change, consider adding one or more screenshots here or link to a screen recording to help reviewers visualize the change. Otherwise, you can remove this section.

@kchelstowski kchelstowski self-assigned this Oct 31, 2024
@github-actions github-actions bot added the domain: frontend Changes or discussions relating to the frontend UI label Oct 31, 2024
Comment on lines 581 to +584
this.setState({ values: {} }, () => {
this.setState({ values: parseResponse(response.data.data) });
this.setState({
values: parseResponse(response.data.data),
initialReceiptCandidates: parseResponse(response.data.data),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to clear the values and then assign a value to this property?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ask the one implementing it initially 🙈
I didn't even see this clearing before, as I only cared about the place where I needed to pass the additional property.
Let's leave it as it is, because I don't know what potentially would be broken by that....

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to clear the values and then assign a value to this property?

This is a really good question, this clearing values seems to be pointless here, and I don't know/remember any reason why it went in. I guess we could remove it and just check out if the page loads correctly.

ask the one implementing it initially 🙈

:/

@awalkowiak awalkowiak merged commit 78aaf9b into develop Nov 5, 2024
@awalkowiak awalkowiak deleted the OBPIH-6707-fix branch November 5, 2024 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain: frontend Changes or discussions relating to the frontend UI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants