Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: .every vs. .forEach #20692

Closed
wants to merge 0 commits into from
Closed

fix: .every vs. .forEach #20692

wants to merge 0 commits into from

Conversation

uepselon
Copy link
Contributor

@uepselon uepselon commented Apr 13, 2023

Reopened here #20711

Description:
The function should return the last email which is part of the timeline according to the creation date of the email. Therefore, it is ordering the communication items in descending order. Since the forEach loop will not break on a return false, it will continue to loop through all items and will return whatever is the last item in the array. I assume this is not as intended. Instead we can use the .every method which breaks on a return false.

Steps to reproduce:

  1. Add multiple email items to a timeline
  2. Call the get_last_email function in form_timeline.js
    -> It will return the oldest element of the timeline instead of the newest.

@uepselon uepselon requested review from a team and shariquerik and removed request for a team April 13, 2023 14:37
@codecov
Copy link

codecov bot commented Apr 13, 2023

Codecov Report

Merging #20692 (16038c4) into develop (b62bb8b) will increase coverage by 0.01%.
The diff coverage is n/a.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #20692      +/-   ##
===========================================
+ Coverage    63.95%   63.97%   +0.01%     
===========================================
  Files          758      758              
  Lines        68755    68755              
  Branches      6208     6208              
===========================================
+ Hits         43973    43985      +12     
+ Misses       21281    21254      -27     
- Partials      3501     3516      +15     
Flag Coverage Δ
server-ui 32.02% <ø> (+0.01%) ⬆️
ui-tests 51.93% <ø> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@phot0n
Copy link
Contributor

phot0n commented Apr 13, 2023

This fix will very likely cause some headaches 😅 .

Currently how this works (getting the last communication in timeline) actually makes stuff easier for us internally.

looking at the code that doesn't seem to be the case how it was intended to work 😅 (which makes your fix valid)

from UX perspective getting the latest communication in timeline seems to be correct.

@surajshetty3416 thoughts?

@uepselon
Copy link
Contributor Author

Accidentally closed this one by pushing to my develop. Reopened here #20711

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants