Per 9261 search by tag public archive

[crisnicandrei]

This pull request adds the public tag search functionality to the app.

Steps to test:

1. Go to a public archive
2. Type a tag's name in the search bar
3. A dropdown will appear
4. Click on one of the tags
5. The search should be working

[codecov]

Codecov Report

Attention: Patch coverage is 65.45455% with 19 lines in your changes missing coverage. Please review.

Project coverage is 43.05%. Comparing base (eccee14) to head (1dadb08).
Report is 12 commits behind head on main.

Files with missing lines	Patch %	Lines
...ponents/archive-search/archive-search.component.ts	47.05%	18 Missing ⚠️
src/app/search/services/search.service.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #490      +/-   ##
==========================================
+ Coverage   42.98%   43.05%   +0.06%     
==========================================
  Files         360      362       +2     
  Lines       10985    11035      +50     
  Branches     1793     1799       +6     
==========================================
+ Hits         4722     4751      +29     
- Misses       6103     6121      +18     
- Partials      160      163       +3     

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

[meisekimiu]

Left some comments, but not all of them need to be explicitly addressed (some of them are more pieces of advice in general and do not need to be specifically applied if they aren't applicable).

[meisekimiu]

This functionality should probably be covered with tests. Also, are we only supposed to be able to search for one tag?

[meisekimiu]

Here's a piece of advice that can lead to better practices when writing JavaScript. Most of the time, when you use the let keyword, there's probably some way to rewrite all of it using the const keyword instead. There are some exceptions like loop structures, but in general it's preferable to use const whenever you can and things like string manipulation can be expressed with all const's:

// This is not necessarily a full suggestion of how to rewrite this code, see another comment for a more specific suggestion!
const endpoint = "/search/folderAndRecord";

const tagsQuery = tags.map((tag, index) => `tags[${index}][tagId]=${tag.tagId}`).join('&');

const queryString = `query=${query}&archiveId=${archiveId}&publicOnly=true&numberOfResults=${limit}&${tagsQuery}`;

return getFirst(
  this.httpV2.get<SearchResponse>(`${endpoint}?${queryString}`, {}, null, {
    authToken: false,
  }),
);

The benefit of this is just that it makes things a bit more stable; we can't accidentally reassign or delete the endpoint name and each constituent part of the query is also just broken up in a way that communicates how it's generated a bit more clearly.

[meisekimiu]

Unlike the "V1" HttpService, the HttpV2Service allows you to pass in data as an object and it will generate the query parameters and append them to the URL itself (see: HttpV2Service::getEndpointWithData). I'm not sure if this directly works with the array syntax the PHP backend is expecting, but we should be able to modify the HttpV2Service itself to handle this kind of query parameter if it doesn't. The point of this is so that we don't have to manually generate query parameters in every single API function every time and we can instead just pass in a data object like we do with POST requests.

[meisekimiu]

Please add a typing for archiveId here. Ideally it would be nice to have test coverage for this function as well, even if it's very simple.

[meisekimiu]

These values should have type annotations, since I think right now they are any[].

[meisekimiu]

We might not need to move this but lately I've been trying to catch myself when I end up writing something like this and asking myself: "Is something like this defined elsewhere in the application?" Ideally when we add a new type of file to support in the future, we shouldn't have to change hundreds of places in the code to support that new file type.

So my question is: Is there already some code somewhere that converts item types into human readable strings like this? I'm not sure but it might be useful to see if there's an opportunity here to make a reusable shared function that many components can use at once (and, for the sake of this component, may be used through an Angular Pipe or something like that). I know the design only points out folders, images, and videos as recognized file types but we probably want to support other types, right? And it'd be annoying to have multiple locations in the codebase that do this same conversion in a slightly different way each time.

[crisnicandrei]

@meisekimiu did the suggested changes. Thanks a lot for your suggestions!

[meisekimiu]

This is a bit on me for using this code as an example of how we can use const in most places in JavaScript, but I didn't actually want my example to be directly used here. As mentioned in this comment, we don't actually have to manually build up the query string as the HttpV2Service can take in a data object like a POST request, and generate and append a query string automatically.

My const suggestion was more a standard JavaScript tip, though it still had applicability in this function. Instead of building up a query string with consts, we should be building up a const data object we pass into the httpV2.get method. And since the parameters for this method are somewhat complex, we probably have to compose this data object out of multiple consts, or at least inlined expressions.

// I have not tested this code!
// This is just a sketch of what the code might look like.
// It definitely needs adjustments for typescript!

const tagQueries = tags.reduce((obj, tag, index) => {
    obj[`tags[${index}][tagId]`] = tag.tagId;
    return obj;
}, {});

const data = {
query,
archiveId,
publicOnly: 'true'
...tagQueries
};

if (limit) {
data.numberOfResults = limit;
}

return getFirst(
this.httpV2.get<SearchResponse>('/search/folderAndRecord', data, null, {
        authToken: false,
      })
);

[crisnicandrei]

@meisekimiu Thanks! Pushed the changes!

[yeslikesolo]

Search my tags working great, thanks Andrei!