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

1648 rozbic osoby zapisane na grupy #1676

Open
wants to merge 20 commits into
base: master-dev
Choose a base branch
from

Conversation

julgitt
Copy link
Collaborator

@julgitt julgitt commented Mar 29, 2024

Przed:
Screenshot from 2024-03-29 20-27-40

Po:
image

@lgpawel lgpawel linked an issue Apr 26, 2024 that may be closed by this pull request
@julgitt julgitt self-assigned this Jun 7, 2024
@julgitt
Copy link
Collaborator Author

julgitt commented Jun 7, 2024

Tak to wygląda obecnie:
image
image

Comment on lines 62 to 63
{{ group.num_enrolled }}/{{group.limit}}
niegwarantowanych{% for gs, taken_spots in spots_by_role %}{% if taken_spots != 0 %},
Copy link
Contributor

Choose a reason for hiding this comment

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

Napis "N / M niegwarantowanych" wypisywałbym tylko pod warunkiem, że któraś z liczb N, M jest niezerowa (od razu mówię, że N > M = 0 jest technicznie możliwe wskutek ręcznych dopisków, zmian limitów etc.)

@lgpawel
Copy link
Contributor

lgpawel commented Jul 2, 2024

Obecnie metoda taken_spots_by_role nie działa dobrze, jeśli np. grupy są puste – ona wtedy też zwróci pusty rezultat, a oczekiwalibyśmy pewnej kolekcji słowników z wartościami równymi 0. Wydaje mi się, że łatwość użycia defaultdict zwiodła nas tu na manowce – możliwe, że trzeba podokładać tam ręcznie te brakujące zera (a jak już to się zrobi, to może w ogóle wykorzystanie defaultdict okaże się zbędne?).

data = {
'students_in_group': students_in_group,
'students_in_queue': students_in_queue,
'guaranteed_spots': enrolled_data.get('guaranteed_spots_rules'),
'spots_by_role': [(gs, enrolled[group.pk][gs.role.name]) for gs in group.guaranteed_spots.all()],
Copy link
Contributor

Choose a reason for hiding this comment

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

Przeskoczyliśmy jeden niezainicjowany klucz w słowniku, trafiamy na kolejny:
image
przy czym jestem zdecydowanie zdania, żeby rozwiązać to w taken_spots_by_role.

Comment on lines 343 to 345
Method returns a dictionary containing:
int: Group index
dict: A dictionary where:
Copy link
Contributor

Choose a reason for hiding this comment

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

Tu powinno się napisać, co w zewnętrznym słowniku jest kluczem, a co wartością.

{{students_in_group|length}}/{{group.total_limit}}{% if group.num_enrolled_by_role %}, w tym:
{% if group.num_enrolled != 0 or group.limit != 0 %}
{{ group.num_enrolled }}/{{group.limit}}
niegwarantowanych{% endif %}{% for gs, taken_spots in spots_by_role %}{% if taken_spots != 0 %},
Copy link
Contributor

Choose a reason for hiding this comment

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

Czy to wykluczanie rezerwowanych miejsc z zerową zajętością mieliśmy już przedyskutowane? Bo jednak efekt

image

lub w drugim widoku analogicznie

image

nie jest za dobry (i to jeszcze zanim zabierzemy się za dopracowywanie pojawiania się spacji i innych separatorów).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wcześniej było ustalone, że wyświetlamy napis „n/m niegwarantowanych” jeżeli n lub m jest niezerowe, ale rzeczywiście chyba lepiej wyglądałoby bez tego.

Copy link
Contributor

Choose a reason for hiding this comment

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

No ale podobnie "n/m dla isimu" chcemy zobaczyć przy m niezerowym nawet, jeśli n jest zerem.

student_roles = {r.name for r in set(student.groups.all()) & group_to_roles[group.id]}
if not student_roles:
student_roles = {""}
enrolled[group.id] += Counter(student_roles)
Copy link
Contributor

Choose a reason for hiding this comment

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

Okazuje się, że + dla Counterów usuwa te zera, które tak pracowicie dodajemy; żeby tego uniknąć, trzeba użyć nieco bardziej rozwlekłego .update() (ale za to wtedy można podarować sobie jawne rzutowanie na Counter).

Suggested change
enrolled[group.id] += Counter(student_roles)
enrolled[group.id].update(student_roles)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Rozbić osoby zapisane na grupy
2 participants