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

Feature/subject registration progress list #206

Merged
merged 12 commits into from
Jun 2, 2024

Conversation

Xopabyteh
Copy link
Contributor

Ukazuje to splněno A/N a basic seznam toho co chybí:
image

Implementačně jsem hodně bojoval s paginací na serveru, takže jsem to teď ponechal jako request.ApplyTo(data) na gridu.

Taky je tam docela neoptimalizovaná práce s query v SubjectRegistrationProgressValidationService.cs z důvodu zachování logiky NextGrade() a taky se to lépe čte, ale způsobuje to, že místo jednoho dotazu do databáze pro všechny data je to rozdělený na 2 * počtetStudentů requestů.

To bych vyřešil nějakým takovýmhle dotazem, který by si přednačetl studenta, budoucí ročník a jeho registrace přednačetl. Je tu ale logika NextGrade() implicitní + je to trochu chaos:

	private readonly IStudentDataSource _studentDataSource;
	private readonly IGradeDataSource _gradeDataSource;
	private readonly IStudentSubjectRegistrationDataSource _studentSubjectRegistrationDataSource;

(...)

	public async Task<Dictionary<int, StudentRegistrationProgress>> GetRegistrationProgressOfAllStudentsAsync(
		StudentSubjectRegistrationProgressListFilter filter,
		CancellationToken cancellationToken = default)
{
		// Filter students:
		// 1. Don't fetch oktava students (they are not subject to progress)
		// 2. Fetch based on given filter
		// Select { StudentId, FutureGrade, Registrations }
		var filteredStudentVMs = await _studentDataSource.Data
			.WhereIf(filter.StudentId is not null, s => s.Id == filter.StudentId)
			.WhereIf(filter.GradeId is not null, s => s.GradeId == filter.GradeId)
			.Where(s => (GradeEntry)s.GradeId != GradeEntry.Oktava)
			.Select(s =>
			new
			{
				Id = s.Id,
				FutureGrade = _gradeDataSource.Data
					.Include(g => g.RegistrationCriteria)
					.FirstOrDefault(g => g.Id == s.GradeId - 1),
				Registrations = _studentSubjectRegistrationDataSource.Data
					.Include(r => r.Subject)
					.ThenInclude(rs => rs.Category)
					.Where(r => r.StudentId == s.Id)
					.ToList()
			})
			.ToArrayAsync(cancellationToken: cancellationToken);

		// Calculate progress of each and store into dict
		var result = new Dictionary<int, StudentRegistrationProgress>(filteredStudentVMs.Length);
		foreach (var student in filteredStudentVMs)
		{
(...)
		}
		return result;
}

@Xopabyteh Xopabyteh requested a review from hakenr May 26, 2024 17:50
Comment on lines +58 to 79
private List<string> GetMissingCriteriaMessages(StudentRegistrationProgress progress)
{
//Todo: Implement
throw new NotImplementedException();
var missingCriteria = new List<string>();

if (!progress.HoursPerWeekProgress.IsProgressComplete)
{
missingCriteria.Add("Počet hodin týdně");
}

if (!progress.CsOrCpRegistrationProgress.IsProgressComplete)
{
missingCriteria.Add("Počet hodin v ČS/ČP");
}

if (progress.LanguageRegistrationProgress.IsLanguageRequired &&
!progress.LanguageRegistrationProgress.HasRegisteredLanguage)
{
missingCriteria.Add("Jazyk");
}

return missingCriteria;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je ještě otázka, jestli by se tahle logika neměla sloučit i s výstupem pro jednotlivého studenta a potom na klientovi foreachovat namísto toho if else if else if co tam teď je

Comment on lines 33 to 44
private string GetStudentLastName(int studentId)
{
return StudentsDataStore.GetByKey(studentId).LastName ?? "-příjmení nenačteno-";
}

private string GetStudentGradeName(int studentId)
{
var gradeId = StudentsDataStore.GetByKeyOrDefault(studentId)?.GradeId;
return gradeId is not null
? GradesDataStore.GetByKey(gradeId.Value)?.Name
: null;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tahle logika se v klientské aplikaci taky opakuje celkem často, ale nejsem si jistý, jak to DRY nout

@hakenr
Copy link
Member

hakenr commented May 28, 2024

@Xopabyteh PERF se řeší tak, že si dopředu posbíráš seznamy věcí, které budeš potřebovat a ty seznamy si načteš z DB hromadnými dotazy, aby ses vyhnul načítání záznamů po jednom. Až se k tomudle PR dostanu (potřebuju teď dodělat něco do práce), tak to zkusím doladit a ukážu ti to pak.

Copy link
Member

@hakenr hakenr left a comment

Choose a reason for hiding this comment

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

lgtm, drobnost upravím a nasadím

@hakenr hakenr merged commit 729fb1a into master Jun 2, 2024
1 check passed
@hakenr hakenr deleted the feature/subject-registration-progress-list branch June 2, 2024 23:30
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.

2 participants