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 #1008: Sourced conflicting holidays being imported by default #1011

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

tupaschoal
Copy link
Collaborator

@tupaschoal tupaschoal commented Oct 8, 2023

Related issue

Closes #1008

Context / Background

When sourcing holidays, the ones matching existing waivers were tagged as conflicts, but still picked to be added nonetheless

What change is being introduced by this PR?

Now when a conflict is detected, the checkbox will be unchecked by default and also disabled. Turns out that all we needed was to either set the checked attribute or not, but we were instead doing checked='' or checked=checked and it always thought it was checked.

  • Do we really want to disable it? If the user wants to import conflicting waivers, then we might as well allow it.

image

How will this be tested?

Test manually, but there are automatic tests for it that I hope still work

@araujoarthur0
Copy link
Collaborator

Thanks @tupaschoal.
I think we can go for the unchecked by default, but not disable the checkboxes.
If any conflict is checked, then we display the alert saying "conflicts will be overwritten".
And then we actually overwrite them (which seems to already be happening, although currently we display two at the table until the window is refreshed).

@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #1011 (9f9d886) into main (237e6b3) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1011   +/-   ##
=======================================
  Coverage   75.04%   75.04%           
=======================================
  Files          26       26           
  Lines        2188     2188           
  Branches      345      345           
=======================================
  Hits         1642     1642           
  Misses        546      546           
Files Coverage Δ
src/workday-waiver.js 76.98% <100.00%> (ø)

@tupaschoal
Copy link
Collaborator Author

@araujoarthur0 I think we should move forward without the pop-up, and add that as another task, since it involves more work and writing new tests (for covering the pop-up and etc)

@araujoarthur0 araujoarthur0 merged commit e25523f into main Oct 18, 2023
9 checks passed
@araujoarthur0
Copy link
Collaborator

\changelog-update

tupaschoal added a commit to tupaschoal/time-to-leave that referenced this pull request Nov 1, 2023
araujoarthur0 pushed a commit that referenced this pull request Nov 1, 2023
* Remove unused variable from #1018

* Amend changelog to include fix from #1011
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.

Loading holidays twice ignores conflicts and generates duplicate entries
2 participants