Labels in datasets list filter

[nitrosx]

Issue Name

Labels in datasets list filter

Summary

When the latest PR for the Search Ui has been merged, the FE show "default label" instead of the correct filter label in the "filters and conditions" form in the datasets list page

Steps to Reproduce

Start the latest FE from master and load the datasets list page

Current Behaviour

Expected Behaviour

The expected behaviour is to have each label matching the dataset property the associated filter works on.

[Ingvord]

@nitrosx Thanks for reporting!

However, I can not reproduce it:

All labels are displayed correctly!

Could you please provide more details on how to reproduce? Thanks

[nitrosx]

Let me do some more digging. Thanks for checking

[nitrosx]

A little bit more mistery: The labels are shown correctly in the modal window

[Ingvord]

Yeah, in the first case code uses this.constructor.name to resolve actual filter label, while in the later it uses string value from the state. The first one, I suspect, maybe platform dependent... See #1571 for details

[Junjiequan]

During the Docker image building process, all the code is minified, which results in class names being altered or shortened. However, the current implementation directly uses the class name to retrieve the corresponding filter label from a hardcoded mapping (labelMap). This approach fails in production because the minified class names do not match the original keys in the labelMap.

[Junjiequan]

I suggest hardcoding the labels directly in the HTML and removing labelMappings altogether. Based on the current structure, it appears that each filter has its own corresponding HTML, and it doesn’t seem likely that we will allow users to modify the label names.

[Ingvord]

There is another place in filter-settings, where we can not easily access actual instance of the component, see https://github.com/SciCatProject/frontend/blob/7d5457b540c58048e681541e3214206515b30514/src/app/datasets/datasets-filter/settings/datasets-filter-settings.component.html#L15

Platform dependent code has been removed in #1571

[Junjiequan]

In that case, I suggest storing the labelsMapping in the frontend.config.json in the backend, instead of hardcoding it in the frontend.

Additionally, we don’t really need the default-filters.config.json in the backend, as it clearly belongs in the frontend configuration. I realize it’s a bit late to comment on the default-filters.config.json after it has been merged, but I didn’t have the full picture of how everything works together at that time.

[Ingvord]

I am confused. How you would do then https://github.com/SciCatProject/scicat-backend-next/issues/604 ? Specifically, to provide GET settings/default endpoint?!

And how storing mapping on the backend will help display those in both places on the frontend?

My idea is to store on the backend simple key-value (filter - enabled), while all labeling and stuff belongs to FE. Co-incidentally, key is the name of the corresponding filter class.

[Junjiequan]

If you’re asking about the feasibility of the approach I mentioned above, we have some examples on the backend that show how to return values from frontend.config.json for the settings/default endpoint:

import config from "../config/frontend.config.json";
import theme from "../config/frontend.theme.json";

@Injectable()
export class AdminService {
  async getConfig(): Promise<Record<string, unknown> | null> {
    return config;
  }
}

The retrieved config values can be stored in the userSettings state or another appropriate place and used by different components on the frontend.

This is just my idea—it’s doable, but I’m not saying it’s the optimal way.

[Ingvord]

Well, I am confused even more then...

That is exactly how default filter settings work now, see e.g. https://github.com/SciCatProject/scicat-backend-next/blob/master/src/users/interceptors/default-user-settings.interceptor.ts

Let's start from the beginning - what problem are you trying to solve here?

[nitrosx]

@Ingvord, let me jump in and try to clarify. The BE should be completely agnostic about the settings for the FE. It should only offer the functionalities to serve the FE configuration based on the site installation and store the user settings as they are.

With your approach, FE and BE are sharing the hard coded filters config in code, which create a limiting dependency. FE config updates would require changes to both the FE and BE. It will also be hard to decouple FE config from BE and where it can be stored.

Regarding the defaults, we think that it should be the task of the FE, through its configuration, to set them. The FE requests the user settings to the BE. Once it receives the data, the FE checks if the relevant settings are present. If they are not, it adds the relevant defaults from its configuration. This approach concentrate all the relevant logic in the FE and limits the interaction between FE and BE to the API defined.

While reviewing your PR, @Junjiequan and myself discussed extensively and, thanks to your work, found some issues due to the original requirements. Instead of wasting everybody's time in lengthy discussions, we think it is more productive to provide our suggestions through a PR directly on this PR/branch, so we can have a more focused discussion.

[Ingvord]

@nitrosx thanks for the detailed explanation!

I see your point and totally agree - we should avoid lengthy discussions.

To express your design decision in code, you have to split current "be" into "be per se" and "fe server side". Then it's fe server side which will handle fe related stuff talking to be via API. However, this is a costly endeavor.

Also, from an alien perspective current architecture tends to be more a 3 tiers monolith. Which is even a recommended way for small teams, and also in fact in bigger infrastructures, like site, labs etc SciCat is indeed a Monolithic microservice.

So again, I am totally see your point. Have my doubts though. Anyway, please consider to merge #1563 before the refactoring. As that is the last piece that introduces save/load of the user filters and conditions.

If you need anything from my side, I am open to lend a hand.

P.S. as I mentioned earlier, there is no bind between fe and be, related to naming, it's just co incidence, with the same effect we can call filters xyz and have naming resolution on the fe

[Ingvord]

Resolved via #1571