Skip to content

Conversation

@artem-blazhko
Copy link
Contributor

@artem-blazhko artem-blazhko commented Apr 19, 2022

Purpose

Enable error message when entering an item barcode without a patron barcode

Refs

UICHKOUT-770

Screenshots

image

@github-actions
Copy link

github-actions bot commented Apr 19, 2022

Jest Unit Test Statistics

0 files  ±0  0 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit c22a6ba. ± Comparison against base commit 0f149be.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Apr 19, 2022

BigTest Unit Test Statistics

    1 files  ±0      1 suites  ±0   10s ⏱️ ±0s
126 tests ±0  124 ✔️ ±0  2 💤 ±0  0 ±0 
127 runs  ±0  125 ✔️ ±0  2 💤 ±0  0 ±0 

Results for commit c22a6ba. ± Comparison against base commit 0f149be.

♻️ This comment has been updated with latest results.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@artem-blazhko artem-blazhko requested review from mkuklis and zburke April 19, 2022 09:25
Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

Nicely done!

Do you have any insight into what changed that caused this to stop working/this event to stop bubbling? I understand what this change does, but not why it was suddenly necessary.

@pavelspichonak
Copy link
Contributor

pavelspichonak commented Apr 19, 2022

@zburke actually we tried to find what caused this to stop working but did not find it.

As far as I remember onSubmit subscription and triggering submit event without bubbling appeared even before release where this issue was not reproducible and after that there were no changes related to subscription and triggering event. So it is very interesting why it worked before at all in previous releases. We decided not to spend more time to find out it...

But I agree it would be great to find what caused this to stop working.

@zburke
Copy link
Member

zburke commented Apr 19, 2022

@pavelspichonak, I totally understand and I appreciate you taking the extra time to provide the history. "This used to work, this stopped working, wait how did this ever work, I don't have time to investigate further but at least it works again now" is an all-too-common storyline in programming 😆 .

@pavelspichonak
Copy link
Contributor

pavelspichonak commented Apr 19, 2022

@zburke fortunately we decided to be better and found what caused this to stop working :)

This pull request caused this to stop working.
React was updated and looks like previous version of react had issue related to bubbling and in new version it was fixed and that is why our logic stoped working.

@pavelspichonak
Copy link
Contributor

@zburke looks like this issue was fixed in react 17.0.0

@zburke
Copy link
Member

zburke commented Apr 19, 2022

Ah, yes, this all snaps into focus. I now recall that the first time we tried to migrate to React v17 ~one year ago, we had to back out because of changes to event handling/bubbling. (Many bugs are linked to STRIPES-722 if you are interested in more archaeology.)

Rephrasing (so I can repeat it to myself to confirm my understanding), old code exploited a bug/loophole in React v16, and that loophole was closed in React 17. Thus, we needed to adjust our code here. With so many changes between the React 17 and update in September and now, I'm impressed that you were able to identify the cause so concisely! Nice detective work, @pavelspichonak 🕵️ !

@artem-blazhko artem-blazhko merged commit 5de410f into master Apr 19, 2022
@artem-blazhko artem-blazhko deleted the UICHKOUT-770 branch April 19, 2022 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants