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

Reorder the user imports fields #3722

Merged
merged 10 commits into from
Jul 10, 2024

Conversation

reiterl
Copy link
Member

@reiterl reiterl commented May 31, 2024

Resolve #3719

@reiterl reiterl added enhancement General enhancement which is neither bug nor feature staging labels May 31, 2024
@reiterl reiterl added this to the 4.2 milestone May 31, 2024
Copy link
Member

@Elblinator Elblinator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add-on:
Please also change the order in the previews

In the account-import:

  • swap pronoun and gender

@Elblinator Elblinator assigned reiterl and unassigned Elblinator May 31, 2024
@reiterl
Copy link
Member Author

reiterl commented May 31, 2024

Added the swap. I think the order of the fields in the preview is delivered by the backend.

@reiterl reiterl assigned Elblinator and unassigned reiterl May 31, 2024
@reiterl reiterl requested a review from Elblinator May 31, 2024 10:02
@Elblinator Elblinator removed their assignment May 31, 2024
@Elblinator Elblinator self-assigned this May 31, 2024
Copy link
Member

@Elblinator Elblinator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see the file src/app/domain/models/users/user.constants now is not used anywhere and should be deleted

Copy link
Member

@luisa-beerboom luisa-beerboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am against separating these two header definitions, it's unnecessarily shaky, it would be better to implement a way to assign these properties a client-side weight separately and then do some column sorting in the import-list components. I'd suggest do it via the defaultColumns input of the import-list.component.ts

@reiterl
Copy link
Member Author

reiterl commented Jun 10, 2024

Adding weight to the datastructure would add complexity to the backend import component interface. And the interface would be uglier. I don't think @luisa-beerboom 's idea is a good idea. Having two definitions for two different imports should be no problem in my opinion. I think my solution is fine. @rrenkert what do you think?

@reiterl reiterl assigned rrenkert and unassigned reiterl Jun 10, 2024
@rrenkert
Copy link
Member

rrenkert commented Jul 4, 2024

The easiest way would be to define two const lists of userHeadersAndVerboseNames.
Currently userHeadersAndVerboseNames is only used for the mentioned imports and defining two lists will allow separate sorting of the columns for each of the imports without the overhead of additional attributes.

@reiterl reiterl assigned Elblinator and luisa-beerboom and unassigned reiterl Jul 4, 2024
Copy link
Member

@luisa-beerboom luisa-beerboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I said: Don't transform the HeadersAndVerboseNames objects. They have the expressed purpose of managing the header names and should be as unified as possible for the sake of consistency.

There are already client-side column configurations (see f.E. the AccountImportListComponent: It's an attribute called columns) add a new optional field weight to those and make the BaseViaBackendImportListComponent sort the columns by that if it is there.

Copy link
Member

@luisa-beerboom luisa-beerboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better, some minor improvements though

Comment on lines 46 to 48
public columns = Object.keys(participantHeadersAndVerboseNames).sort(
(a, b) => participantColumnsWeight[a] - participantColumnsWeight[b]
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If participantColumnsWeight already contains all relevant fields, why even import participantHeadersAndVerboseNames at all

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, changed it.

Comment on lines 14 to 33
export const participantColumnsWeight: { [key in keyof GeneralUser]?: any } = {
title: 1,
first_name: 2,
last_name: 3,
email: 4,
member_number: 5,
structure_level: 6,
groups: 7,
number: 8,
vote_weight: 9,
gender: 10,
pronoun: 11,
username: 12,
default_password: 13,
is_active: 14,
is_physical_person: 15,
is_present: 16,
saml_id: 17,
comment: 18
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using an array with the index being the weight may also work, would spare you the work of having to re-sort the keys every time you need the keys in-order

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now use an array.

Comment on lines 15 to 17
public possibleFields = Object.keys(participantHeadersAndVerboseNames).sort(
(a, b) => participantColumnsWeight[a] - participantColumnsWeight[b]
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments in participant-csv-export.service.ts and participant-import/definitions/index.ts

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -114,7 +114,7 @@ describe(`CsvExportForBackendService`, () => {

it(`test dummy export method with default settings`, () => {
service.dummyCSVExport(
{ proper: `Fancy`, tea: `Tea`, strength: `Muscle` },
Object.keys({ proper: `Fancy`, tea: `Tea`, strength: `Muscle` }),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use the list of keys at this point, no use keeping this as an object

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@reiterl reiterl requested a review from luisa-beerboom July 8, 2024 10:12
@reiterl
Copy link
Member Author

reiterl commented Jul 8, 2024

Updated: Use lists for the order config. Remove surplus import, no longer sort and simplify one test.

@Elblinator
Copy link
Member

please solve merge conflicts

@Elblinator Elblinator assigned reiterl and unassigned Elblinator Jul 9, 2024
@Elblinator Elblinator removed the request for review from emanuelschuetze July 9, 2024 12:55
@reiterl reiterl merged commit 6c66759 into OpenSlides:main Jul 10, 2024
2 checks passed
@reiterl reiterl deleted the 3719-reorder-user-import-fields branch July 10, 2024 06:56
@peb-adr peb-adr mentioned this pull request Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement General enhancement which is neither bug nor feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import pages: adjustments needed because of membership number
5 participants