-
Notifications
You must be signed in to change notification settings - Fork 45
Adding buttons to sort assignment candidates by first/last name #5403
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
base: main
Are you sure you want to change the base?
Adding buttons to sort assignment candidates by first/last name #5403
Conversation
58af34d
to
d17b3b0
Compare
You can read the install here: https://github.com/OpenSlides/OpenSlides/blob/main/DEVELOPMENT.md To pull every service to main you need to run "$ make services-to-main" on the main-repo To set up your repo for the client see: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- a subscription is missing so that the new data is updated without a reload
- The current buttons should behave and look like hte sort options in different lists (e.g. participants)
- You should press "sort and then the sort menu opens where you can press "Given name" or "Surname"
- the sort should be possible to sort upwards and downwards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't fulfil the linter style requirements.
Quoting our readme:
Code can be cleaned and aligned automatically using `npm run-cleanup`.
This will take care of code alignment, import sorting and quotation marks.
To execute this inside the docker container, you can either use `make run-cleanup` while the client
container is already running or `make run-cleanup-standalone` if it's not.
Alternatively - if the latter doesn't work - you can try to run npm run cleanup
in the openslides-client/client
folder
expect(sortedCandidates.map((c: ViewAssignmentCandidate) => c.user.first_name)).toEqual(['Clara', 'Karl', 'Karl', 'Kurt', 'Rosa']); | ||
expect(sortedCandidates.map((c: ViewAssignmentCandidate) => c.user.last_name)).toEqual(['Zetkin', 'Liebknecht', 'Marx', 'Eisner', 'Luxemburg']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(sortedCandidates.map((c: ViewAssignmentCandidate) => c.user.first_name)).toEqual(['Clara', 'Karl', 'Karl', 'Kurt', 'Rosa']); | |
expect(sortedCandidates.map((c: ViewAssignmentCandidate) => c.user.last_name)).toEqual(['Zetkin', 'Liebknecht', 'Marx', 'Eisner', 'Luxemburg']); | |
expect(sortedCandidates.flatMap((c: ViewAssignmentCandidate) => [c.user.first_name, c.user.last_name])).toEqual(['Clara', 'Zetkin', 'Karl', 'Liebknecht', 'Karl', 'Marx', 'Kurt', 'Eisner', 'Rosa', 'Luxemburg']); |
This is shorter. Should also be done for the other test.
{user: {first_name: 'Karl', last_name: 'Marx'}, id: 1} as ViewAssignmentCandidate, | ||
{user: {first_name: 'Rosa', last_name: 'Luxemburg'}, id: 2} as ViewAssignmentCandidate, | ||
{user: {first_name: 'Kurt', last_name: 'Eisner'}, id: 3} as ViewAssignmentCandidate, | ||
{user: {first_name: 'Clara', last_name: 'Zetkin'}, id: 4} as ViewAssignmentCandidate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests below assume 5 users, so one seems to be missing here.
expect(component).toBeTruthy(); | ||
}); | ||
|
||
describe('candidate sorting', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't typically write tests for components (only services, pipes and the like). I don't even think we have any working component tests currently, not that we would be against getting one.
In any case, this test won't be called as long as the main test function (xdescribe('AssignmentDetailComponent', ...
) is x-ed out.
So there's two choices right now:
- Reactivate the main test function, and make it run. You may have to mock quite a few things to do that though.
- Just delete your test, the chances that this test file will ever be re-activated are quite slim, after all.
I still reviewed your test code, just in case you want to go with option 1.
*/ | ||
public async sortCandidatesByLastName(): Promise<void> { | ||
const sorted = [...this.assignmentCandidates].sort((a, b) => | ||
a.user?.last_name?.localeCompare(b.user?.last_name ?? '') ?? 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this'll sort particularly well if a
has no last name, as it'll always treat it as if a
and b
had the same last name in that case, if it doesn't crash of course. Perhaps consider this:
a.user?.last_name?.localeCompare(b.user?.last_name ?? '') ?? 0 | |
(a.user?.last_name ?? '').localeCompare(b.user?.last_name ?? '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for the function below. Also, since they're so similar, perhaps you could consider unifying them into a single function? Or maybe just join the first and last name with a ', '
for each user and then sort based on that. There is also the
There's also the question of what'll happen if there is neither first, nor last name. In that case the user will be shown as User <user id>
, this case should also be taken into account.
The ParticipantListSortService
from participant-list-sort.service.ts
and its base class may serve as an inspiration.
Hey there @Elblinator and @luisa-beerboom First of all - thanks for taking the time for the review.
ok so i was trying to set this up on my macOS device exactly as described in the so first of all the $ make run-dev
sed -i "1s/.*/go 1.25.0/" dev/docker/workspaces/*.work
sed: 1: "dev/docker/workspaces/a ...": extra characters at the end of d command
make: *** [build-dev] Error 1 I found out that there is a difference in behavior when using So this can be resolved by detecting the os and building the full command dynamically based on that: UNAME_S := $(shell uname -s)
ifeq ($(UNAME_S),Darwin)
SED_INPLACE = -i ''
else
SED_INPLACE = -i
endif
build-dev:
sed $(SED_INPLACE) "1s/.*/$(GO_VERSION)/" $(DOCKER_PATH)/workspaces/*.work
chmod +x $(SCRIPT_PATH)/makefile/build-all-submodules.sh
$(SCRIPT_PATH)/makefile/build-all-submodules.sh dev I think this is a kind of change that more people in the dev team or even future devs could benefit from, as the number of devs using mac-os devices is increasing. If you're interested i can do another PR for this :) Back to the original problem: When i run The postgres and the redis container are logging:
the datastore-writer container logs
So now about setting up only the client: I run these commands within the openslides-client repo: docker build . -t client-dev --build-arg CONTEXT=dev
docker run -it -v ${PWD}/client/src:/app/src -p 4200:4200 --rm client-dev The container started on port 4200 logging:
when i try to connect to it via curl localhost:4200
curl: (56) Recv failure: Connection reset by peer What did work was to run the But in this case i was not really able to do any testing in the UI, because it complained about using http and not https and did not show any login form ![]() |
It's very hard for me as being completely new to this codebase to find this out by myself. |
I was not able to find any kind of sorting button in the participants list. Would be helpful if you can point me to the exact piece of code that you are referencing :) |
That would be very lovely! The PR needs to be in the main repo |
I am sorry for the confusion with the setup. To work with the client (change stuff etc.) you need the steps from |
Yeah - that's exactly what I did. Unfortunately some of the containers exited right away as described above. :( |
|
Sorry for the rather short CR (change request), I'll eloaborate more, if anything is still an unknown just ask away :)
Our subscritions are build like this: To figure out which field(s) need a subscription you can use try and error, look at the models.yml (https://github.com/OpenSlides/openslides-meta/blob/main/models.yml) to figure out which fields you can subscribe and look at different subscriptions and what they sbscribe |
For more reference: I myself have not worked so much with this code, so I am not sure how or what to bear in mind in particular, but if you have any qustion or remarks please please just ask and someone will answer you |
Also your setup problem is neither ignored nor forgotten, we will answer as fast as we can |
If at any point you realise that something is just too much or you do not have time anymore, just write what you cannot do anymore and we will take over |
Hey @xela1601, I've tried to run the commands you have described and largely got the same output as you.
I got the same error as you did. The solution is to add
I had the same issue here, but resolving why |
After googling for this error here
it seems to heavily indicate that there is an architecture-based problem. We've tried setting up OpenSlides on a Raspi System with ARM64 Architecture, which should be the same that mac uses, and it worked fine. |
hey @Janmtbehrens thanks for having a look. I was also already looking into this and most resources out there indicated an arch-based problem. so now all of the images are running without exiting. Opening https://localhost:8000 also seems to work. |
Enable sorting candidates by first/last name
Resolves #5391
Since, I was not really able to test this with a local setup, it would be nice if the one who does the review can do this.