TT-85 fix: fix overlapping seconds on time entries #591

PaulRC-ioet · 2020-12-17T23:17:26Z

fix a bug that not allow to update changes on "time entries".

codecov · 2020-12-17T23:26:56Z

Codecov Report

Merging #591 (1685b88) into master (13de42e) will increase coverage by 0.72%.
The diff coverage is 98.75%.

@@            Coverage Diff             @@
##           master     #591      +/-   ##
==========================================
+ Coverage   93.09%   93.82%   +0.72%     
==========================================
  Files          85       95      +10     
  Lines        1564     1700     +136     
  Branches      107      112       +5     
==========================================
+ Hits         1456     1595     +139     
  Misses         67       67              
+ Partials       41       38       -3

Impacted Files	Coverage Δ
src/app/app-routing.module.ts	`100.00% <ø> (ø)`
...nents/time-range-form/time-range-form.component.ts	`100.00% <ø> (ø)`
...project-list-hover/project-list-hover.component.ts	`88.37% <ø> (ø)`
...dules/time-entries/pages/time-entries.component.ts	`83.58% <87.50%> (+0.76%)`	⬆️
src/app/modules/users/store/user.reducers.ts	`87.50% <87.50%> (ø)`
src/app/guards/login-guard/login.guard.ts	`100.00% <100.00%> (ø)`
...nologies-report-guard/technologies-report.guard.ts	`100.00% <100.00%> (ø)`
src/app/modules/login/login.component.ts	`100.00% <100.00%> (+16.66%)`	⬆️
...app/modules/login/services/azure.ad.b2c.service.ts	`86.95% <100.00%> (+4.60%)`	⬆️
...ponents/details-fields/details-fields.component.ts	`90.69% <100.00%> (+3.51%)`	⬆️
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e23d02...1685b88. Read the comment docs.

josepato87 · 2020-12-17T23:26:23Z

src/app/modules/shared/components/details-fields/details-fields.component.spec.ts

+  const startHourTest = moment().subtract(3, 'hours').format('HH:mm:ss');
+  const startDateTest = new Date(`${dateTest}T${startHourTest.trim()}`);


I would move these two lines before the end variables

I have change all the tests, please check them.

josepato87 · 2020-12-17T23:27:45Z

src/app/modules/shared/components/details-fields/details-fields.component.spec.ts

+  const dateTest = moment().format('YYYY-MM-DD');
+  const endHourTest = moment().format('HH:mm:ss');
+  const endDateTest = new Date(`${dateTest}T${endHourTest.trim()}`);
+  const startHourTest = moment().subtract(3, 'hours').format('HH:mm:ss');
+  const startDateTest = new Date(`${dateTest}T${startHourTest.trim()}`);


Do you think we could have any issue about the timing when the tests run? For example, if we put this test running at 1:00 am, due you are subtracting 3 hours, it means that the current hour would be 23:00 of what day?

I have deleted that block of lines, it's different now.

josepato87 · 2020-12-17T23:31:25Z

src/app/modules/shared/components/details-fields/details-fields.component.spec.ts

+    expect(component.starDateValue).toEqual(startHourValue);
+    expect(component.endDateValue).toEqual(endHourValue);


How are you asserting that the other fields, like description are being updated?

there's a new test to prove that.

josepato87 · 2020-12-17T23:33:22Z

src/app/modules/shared/components/details-fields/details-fields.component.spec.ts

+    const startHourValue = moment().subtract(3, 'hours').format('HH:mm');
+    const endHourValue = moment().format('HH:mm');


not sure if this should be the best approach for setting the start and end hours values, as I understand, you are setting those entry values in the before each section. Could you read the current entry "hours" and setting the test variables with those values instead?

I have also deleted those hours, now is in the test those values.

josepato87 · 2020-12-17T23:49:24Z

src/app/modules/shared/components/details-fields/details-fields.component.ts

+      new Date(`${startDate}T${this.entryForm.value.start_hour.trim()}`).toISOString();
+    this.endDateValue = this.entryForm.value.end_hour === this.endDateValue ?
+      this.entryToEdit.end_date :
+      new Date(`${endDate}T${this.entryForm.value.end_hour.trim()}`).toISOString();


I see an opportunity to extract lines 213 and 216 into a method that receives the entryForm.value.X_VALUE and returns everything that is being done in that large sentence.

If you find that in other files, could be an utilitary. :)

It's Done thanks.

scastillo-jp · 2020-12-22T22:15:21Z

src/app/modules/shared/components/details-fields/details-fields.component.spec.ts

    expect(component.saveEntry.emit).toHaveBeenCalledWith(data);
  });

+  it('should not modify the start_date when start_hour has not been modified', () => {
+    const dateTest = moment().format('YYYY-MM-DD');


change:
dateTest to currentDate
startHourTest to startHour

I think is necessary use other name of variable per example dateTest to dateCurrent

Ok it's changed

Angeluz-07 · 2020-12-23T19:22:43Z

src/app/modules/shared/components/details-fields/details-fields.component.ts

+    const isEndDateInTheFuture = moment(endDateToSubmit).isAfter(moment());
+    const timeEntryIsInTheFuture = isStartDateInTheFuture || isEndDateInTheFuture;
+
+    if (timeEntryIsInTheFuture) {


I remember we made a change to extract timeEntryIsInTheFuture into a function. I guess that caused issues in existing tests and you ended up doing it like this. Am I right, or is it an error?

I just tried the function of not allowing a "time entry" in the future and it allowed me to add. It ignores the way we put it so I left it as it was. but with the tests, there was no problem.

It's ok. Then, keep in mind that, when we may want to retake that little refactor.

josepato87

LGTM

PaulRC-ioet requested review from enriquezrene, scastillo-jp, Angeluz-07, josepato87 and gqroot December 17, 2020 23:17

josepato87 reviewed Dec 17, 2020

View reviewed changes

PaulRC-ioet force-pushed the TT-85FixOverlappingSeconds branch from 3b7454d to 35d21e8 Compare December 18, 2020 20:51

PaulRC-ioet added 4 commits December 22, 2020 11:21

TT-85 fix: fix overlapping seconds on time entries

8b50d4f

TT-85 fix: correct word fit

3155406

TT-85 fix: refactor start date and end date

29da434

TT-85 fix: refactor code and improve tests

85a4c93

PaulRC-ioet force-pushed the TT-85FixOverlappingSeconds branch from 35d21e8 to 85a4c93 Compare December 22, 2020 16:27

scastillo-jp reviewed Dec 22, 2020

View reviewed changes

Angeluz-07 reviewed Dec 23, 2020

View reviewed changes

TT-85 fix: refactor code of time fields

1685b88

josepato87 approved these changes Dec 23, 2020

View reviewed changes

josepato87 merged commit df0ae39 into master Dec 23, 2020

josepato87 deleted the TT-85FixOverlappingSeconds branch December 23, 2020 22:20

		const startHourTest = moment().subtract(3, 'hours').format('HH:mm:ss');
		const startDateTest = new Date(`${dateTest}T${startHourTest.trim()}`);

		expect(component.starDateValue).toEqual(startHourValue);
		expect(component.endDateValue).toEqual(endHourValue);

		const startHourValue = moment().subtract(3, 'hours').format('HH:mm');
		const endHourValue = moment().format('HH:mm');

TT-85 fix: fix overlapping seconds on time entries #591

TT-85 fix: fix overlapping seconds on time entries #591

Uh oh!

Conversation

PaulRC-ioet commented Dec 17, 2020

Uh oh!

codecov bot commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josepato87 Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josepato87 Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josepato87 Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josepato87 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Dec 17, 2020 •

edited

Loading

josepato87 Dec 17, 2020 •

edited

Loading

josepato87 Dec 17, 2020 •

edited

Loading

josepato87 Dec 17, 2020 •

edited

Loading