SF-3838 Add date and text filter on onboarding requests tab

[RaymondLuong3]

This PR adds the logic to allow filtering onboarding requests by date and strings in the request object. A new SearchRecordsComponent is now shared between the onboarding requests tab and the serval builds tab. Slight modifications were made to the template in the serval builds tab to align with the template in the Onboarding Requests tab.

Before

After

Before

After

---

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 86.74699% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.20%. Comparing base (f308afa) to head (62e6d0f).
⚠️ Report is 5 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...boarding-requests/onboarding-requests.component.ts	88.88%	4 Missing and 2 partials ⚠️
.../serval-administration/search-records.component.ts	20.00%	4 Missing ⚠️
...p/serval-administration/serval-builds.component.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4024      +/-   ##
==========================================
+ Coverage   81.09%   81.20%   +0.11%     
==========================================
  Files         665      666       +1     
  Lines       43247    43315      +68     
  Branches     7086     7074      -12     
==========================================
+ Hits        35072    35176     +104     
+ Misses       6994     6966      -28     
+ Partials     1181     1173       -8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[Nateowami]

I really don't think defaulting to a narrow date range on the onboarding request page is a good idea. The builds page and onboarding requests page are different in that:

• Onboarding requests are something that need to be dealt with (it's like an inbox, and limited to one per project)
• Builds are something you can review (it's like a log, and has no limit and can grow massive)

We probably do need to add some kind of filtering to avoid showing too many elements at once, but right now the default filtering already mostly addresses it.

It also creates some very non-intuitive views. If you click on "All" you might expect it might be filtered to recent issues and you could select to show more. But if you click on "mine" and it says there are none, you're probably not going to realize you have to select a wider time range to see all, and there's no option to show all.

@Nateowami made 1 comment.
Reviewable status: 0 of 10 files reviewed, all discussions resolved.

[RaymondLuong3]

You have a good point that setting a default date range does not make sense since there is not reason to filter the requests to a default range. I have removed the default filter so that onboarding requests are not filtered by date range until a range is selected.

@RaymondLuong3 made 1 comment.
Reviewable status: 0 of 14 files reviewed, all discussions resolved.

[github-actions]

✅ No screenshot differences — all stories are identical.

View the diff page at: https://pr-4024--sf-screenshot-diffs.netlify.app

[marksvc]

This is reviewable in Devin Review.

[marksvc]

Thank you for your work on this!

@marksvc reviewed 14 files and all commit messages, and made 9 comments.
Reviewable status: 14 of 15 files reviewed, 8 unresolved discussions (waiting on RaymondLuong3).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/date-range-picker.component.html line 13 at r3 (raw file):

      type="button"
      (click)="resetToDefaultDateRange()"
      id="reset-button"

Nothing will go wrong from having an id here just for the spec. But date pickers from Onboarbing Requests and Serval Builds ..sort of live on the same page, and I think I've also had suggested to me to use data-test-id for this sort of purpose. What do you think about changing this with something like

# src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/date-range-picker.component.html
       matSuffix
       type="button"
       (click)="resetToDefaultDateRange()"
-      id="reset-button"
+      data-test-id="reset-button"
# src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/date-range-picker.component.spec.ts
 
     // Clear the range
     expect(env.component.showReset).toBe(true);
-    const clearButton = env.fixture.nativeElement.querySelector('#reset-button');
+    const clearButton = env.fixture.nativeElement.querySelector('[data-test-id="reset-button"]');

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/date-range-picker.component.ts line 193 at r3 (raw file):

    if (inputRange.start == null && inputRange.end == null) {
      this.isDefaultRange = this.defaultRange == null;
      this.dateRangeChange.emit();

On the Serval Builds tab, if I click in and backspace the start date, then tab, and backspace the end date, and then tab, the date picker emits an undefined date range, which Serval Builds receives, and I end up with ServalBuilds.dateRange$.value equal to null. If I click "Export CSV" at Serval Builds, I get an error dialog saying "Date range is not set".

I suggest that we modify date-range-picker in this way, so that it will only emit null date ranges if a null range is "valid".

# src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/date-range-picker.component.ts
   private emitNormalizedIfValid(inputRange: { start: Date | null | undefined; end: Date | null | undefined }): void {
-    if (inputRange.start == null && inputRange.end == null) {
-      this.isDefaultRange = this.defaultRange == null;
+    if (inputRange.start == null && inputRange.end == null && this.defaultRange == null) {
+      this.isDefaultRange = true;
       this.dateRangeChange.emit();
       return;
     }

Oh, nice, Devin also identified that clearing the date boxes and exporting will result in an error.

Since DateRangePickerComponent.dateRangeChange can now possibly be undefined, when ServalBuildsComponent.onDateRangeChange receives the value I think it should defensively handle it by saying something like

   protected onDateRangeChange(range: NormalizedDateRange | undefined): void {
+    if (range == null) return;
     this.dateRange$.next(range);
   }                                                                                    

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/date-range-picker.component.spec.ts line 31 at r3 (raw file):

    expect(initialFormValue.start).toBeNull();
    expect(initialFormValue.end).toBeNull();
    expect(emitSpy).not.toHaveBeenCalled();

Okay. When the component does start with a date range, it emits that date range (in ngOnInit). Emitting a null date range would be symmetric, but I think it's also sensible not to emit the null range when starting with null, as is done at present.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.ts line 279 at r3 (raw file):

  }

  private applyFilter(request: OnboardingRequest): boolean {

Hmm. There is an increasing amount of "filtering" going on. I suspect a different term might be helpful for this method, since it relates to the new search rather than the existing filter capability. And I see that this method is also returning a boolean regarding whether or not a data record should or should not be shown, given the search term. Given this, what do you think of naming this method something like

• matchesSearch,
• isSearchResult,
• withinSearch,
• showFromSearch, or
• isSearchMatch ?

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.ts line 292 at r3 (raw file):

      this.getProjectName(request.submission.projectId),
      request.submission.formData.name,
      request.submission.formData.email,

We can listen for if it would be desired to have request.submittedBy.name and .email included in this list.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.ts line 316 at r3 (raw file):

      replaceUrl: true
    });
  }

BTW, if we type a search string, and click back and forth between the Serval Builds tab and Onboarding Requests tab, both retain the search string and filter by it.

Not a problem; just wanted to point out the behaviour.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.ts line 321 at r3 (raw file):

    const requestDate: Date | undefined = parseDate(request.submission.timestamp);
    if (requestDate == null) {
      return false;

AI is concerned that if somehow the date were to not be parseable, the record would never appear in the table, even if the date range selection is empty.

Now, this will probably never happen. But it does make a fair point that passing a record (even one with an invalid date), to a method called isWithinSelectedDateRange, when the date range is null, and getting back false is perhaps not optimal.(Given that records with valid dates would come back true.) And if we ever did get a record with an invalid timestamp, we would never see it in the table.

(AI suggests using

    if (requestDate == null) {
      return this.dateFrom == null && this.dateTo == null;
    }

)

I will let you decide.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.spec.ts line 28 at r3 (raw file):

};

describe('OnboardingRequestsComponent', () => {

It looks like this file should be rolled into onboarding-requests.component.spec.ts?

[RaymondLuong3]

@RaymondLuong3 made 8 comments and resolved 5 discussions.
Reviewable status: 14 of 15 files reviewed, 3 unresolved discussions (waiting on marksvc).

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/date-range-picker.component.html line 13 at r3 (raw file):

Previously, marksvc wrote…

Nothing will go wrong from having an id here just for the spec. But date pickers from Onboarbing Requests and Serval Builds ..sort of live on the same page, and I think I've also had suggested to me to use data-test-id for this sort of purpose. What do you think about changing this with something like

# src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/date-range-picker.component.html
       matSuffix
       type="button"
       (click)="resetToDefaultDateRange()"
-      id="reset-button"
+      data-test-id="reset-button"
# src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/date-range-picker.component.spec.ts
 
     // Clear the range
     expect(env.component.showReset).toBe(true);
-    const clearButton = env.fixture.nativeElement.querySelector('#reset-button');
+    const clearButton = env.fixture.nativeElement.querySelector('[data-test-id="reset-button"]');

Done

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/date-range-picker.component.ts line 193 at r3 (raw file):

Previously, marksvc wrote…

On the Serval Builds tab, if I click in and backspace the start date, then tab, and backspace the end date, and then tab, the date picker emits an undefined date range, which Serval Builds receives, and I end up with ServalBuilds.dateRange$.value equal to null. If I click "Export CSV" at Serval Builds, I get an error dialog saying "Date range is not set".

I suggest that we modify date-range-picker in this way, so that it will only emit null date ranges if a null range is "valid".

# src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/date-range-picker.component.ts
   private emitNormalizedIfValid(inputRange: { start: Date | null | undefined; end: Date | null | undefined }): void {
-    if (inputRange.start == null && inputRange.end == null) {
-      this.isDefaultRange = this.defaultRange == null;
+    if (inputRange.start == null && inputRange.end == null && this.defaultRange == null) {
+      this.isDefaultRange = true;
       this.dateRangeChange.emit();
       return;
     }

Oh, nice, Devin also identified that clearing the date boxes and exporting will result in an error.

Since DateRangePickerComponent.dateRangeChange can now possibly be undefined, when ServalBuildsComponent.onDateRangeChange receives the value I think it should defensively handle it by saying something like

   protected onDateRangeChange(range: NormalizedDateRange | undefined): void {
+    if (range == null) return;
     this.dateRange$.next(range);
   }                                                                                    

This makes a lot of sense. Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/date-range-picker.component.spec.ts line 31 at r3 (raw file):

Previously, marksvc wrote…

Okay. When the component does start with a date range, it emits that date range (in ngOnInit). Emitting a null date range would be symmetric, but I think it's also sensible not to emit the null range when starting with null, as is done at present.

Yes, I could see it either way, but presently not emitting seems to work fine.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.ts line 279 at r3 (raw file):

Previously, marksvc wrote…

Hmm. There is an increasing amount of "filtering" going on. I suspect a different term might be helpful for this method, since it relates to the new search rather than the existing filter capability. And I see that this method is also returning a boolean regarding whether or not a data record should or should not be shown, given the search term. Given this, what do you think of naming this method something like

• matchesSearch,
• isSearchResult,
• withinSearch,
• showFromSearch, or
• isSearchMatch ?

I like matchesSearch. Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.ts line 292 at r3 (raw file):

Previously, marksvc wrote…

We can listen for if it would be desired to have request.submittedBy.name and .email included in this list.

Yes, I see what you mean. I see there was a recent change and the data we expect no longer contains the submittedBy property. So this is no longer relevant.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.ts line 316 at r3 (raw file):

Previously, marksvc wrote…

BTW, if we type a search string, and click back and forth between the Serval Builds tab and Onboarding Requests tab, both retain the search string and filter by it.

Not a problem; just wanted to point out the behaviour.

Yes, I saw that too. It may or may not be an issue but good to note.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.component.ts line 321 at r3 (raw file):

Previously, marksvc wrote…

AI is concerned that if somehow the date were to not be parseable, the record would never appear in the table, even if the date range selection is empty.

Now, this will probably never happen. But it does make a fair point that passing a record (even one with an invalid date), to a method called isWithinSelectedDateRange, when the date range is null, and getting back false is perhaps not optimal.(Given that records with valid dates would come back true.) And if we ever did get a record with an invalid timestamp, we would never see it in the table.

(AI suggests using

    if (requestDate == null) {
      return this.dateFrom == null && this.dateTo == null;
    }

)

I will let you decide.

Yea, that is kind of a toss up for how to handle this. I accepted the suggestion and made a comment noting that it is probably not going to happen.

---

src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/onboarding-requests/onboarding-requests.spec.ts line 28 at r3 (raw file):

Previously, marksvc wrote…

It looks like this file should be rolled into onboarding-requests.component.spec.ts?

Wow, I somehow managed to write two spec files. I've removed the one that is not relevant.

[marksvc]

@marksvc reviewed 7 files and all commit messages, and resolved 3 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on RaymondLuong3).