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

PR #1

Open
wants to merge 9 commits into
base: comments-branch
from
Open

PR #1

wants to merge 9 commits into from

Conversation

@thestahan
Copy link
Owner

thestahan commented Feb 11, 2020

No description provided.

public class CategoryController : Controller
{
private readonly ICategoryRepository categoryRepository;
[BindProperty]

This comment has been minimized.

Copy link
@szymczakk

szymczakk Feb 13, 2020

Collaborator

jeżeli się bawisz w kontrolery, czyli lecisz jakimś MVC albo WebApi to nie używaj generalnie BindProperty.

to jest wynalazek dla Razor Pages, a w MVC/WebApi powinieneś dane przyjmować jako model do akcji

View(categoryRepository.GetCategoryById(id));

[HttpPost, ActionName("Edit")]
public IActionResult EditPost()

This comment has been minimized.

Copy link
@szymczakk

szymczakk Feb 13, 2020

Collaborator

np tutaj powinieneś przyjąć "Category" jak model (a tak naprawdę to jakiś View Model który się zmapuje na model bazodanowy, nie wysyłać modeli bazodanowych na front) a nie przez "BindProperty" tam gdzieś nie wiadomo skąd sie wziął.

public IActionResult EditPost(TaskEditViewModel taskEditViewModel)
{
if (ModelState.IsValid)
{
taskRepository.Update(Task);
TempData["Message"] = "Task updated.";
return RedirectToAction("List");
}
taskEditViewModel.Categories = categoryRepository.AllCategoriesWithIds();
return View(taskEditViewModel);
Comment on lines +70 to +79

This comment has been minimized.

Copy link
@szymczakk

szymczakk Feb 13, 2020

Collaborator

co tu się wgl dzieje O_o ?

przyjmujesz TaskEditViewModel a i tak dzialasz na jakimś "Task" (nie uzywaj takiej nazwy, jest mocno myląca z Task'iem z net'a) który gdzieś przyjmujesz inaczej.

mocno zaciemnia to kod, powinienes tu operować na taskEditViewModel

//ten błąd masz w wielu miejscach. nie będę każdego komentował


namespace ToDoList.Models
{
public class CategoryRepository : ICategoryRepository

This comment has been minimized.

Copy link
@szymczakk

szymczakk Feb 13, 2020

Collaborator

Ogólnie koncept repozytoriów jest średnio potrzebny gdy używasz DbContext, bo DbContext implementuje koncept repozytoriów. ale nie jest to błąd.

var entity = appDbContext.Attach(updatedCategory);
entity.State = EntityState.Modified;
appDbContext.SaveChanges();
return updatedCategory;
Comment on lines +56 to +59

This comment has been minimized.

Copy link
@szymczakk

szymczakk Feb 13, 2020

Collaborator

chyba lepszą opcją jest wyjąć obiekt po ID, przepisać property i dać SaveChanges, jak znajdę "dowody" to podrzucę, teraz nie pamiętam czemu tak,


public DateTime CreationDate { get; set; }

private DateTime deadline;

This comment has been minimized.

Copy link
@szymczakk

szymczakk Feb 13, 2020

Collaborator

w którym miejscu ustawiasz to pole? bo jest prywatne a nie ma tu konstruktora.

@@ -0,0 +1,8 @@
@model Category

This comment has been minimized.

Copy link
@szymczakk

szymczakk Feb 13, 2020

Collaborator

pchanie modeli DB na front to duży błąd. To powinien być ViewModel

@@ -0,0 +1,28 @@
@model TaskEditViewModel

This comment has been minimized.

Copy link
@szymczakk

szymczakk Feb 13, 2020

Collaborator

o tu masz view model i super

<input type="hidden" asp-for="TaskId" />
<input type="hidden" asp-for="Category" />
<input type="hidden" asp-for="CategoryId" />
<input type="hidden" asp-for="CreationDate" />
<input type="hidden" asp-for="Title" />
<input type="hidden" asp-for="Description" />
<input type="hidden" asp-for="Deadline" />
Comment on lines +5 to +11

This comment has been minimized.

Copy link
@szymczakk

szymczakk Feb 13, 2020

Collaborator

po co ci te wszystkie property tu?

Do "kończenia" zadania nie wystarczy ci tylko jego Id ?

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.