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

Don't allow to set child element as parent. #19

Merged
merged 9 commits into from
Jan 14, 2019
Merged

Conversation

cabad11
Copy link
Contributor

@cabad11 cabad11 commented Dec 23, 2018

Resolves #15

src/routes/pages.js Outdated Show resolved Hide resolved
@@ -24,6 +24,18 @@ router.get('/page/edit/:id', async (req, res, next) => {
let page = await Pages.get(pageId);
let pagesAvailable = await Pages.getAll();

(function childrenRemove(parent) {
Copy link
Member

Choose a reason for hiding this comment

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

  • нужно оформить в виде нормального метода класса, с понятным названием и документацией.
  • эта логика не должна быть в роутах. Скорее в модели.

@@ -24,6 +24,18 @@ router.get('/page/edit/:id', async (req, res, next) => {
let page = await Pages.get(pageId);
let pagesAvailable = await Pages.getAll();

(function childrenRemove(parent) {
pagesAvailable.forEach((item, index) => {
Copy link
Member

Choose a reason for hiding this comment

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

метод не должен использовать внешние переменные

set function as method of Pages
/**
* Change wrong pages to null
*
* @returns {Promise<Page[]>}
Copy link
Member

Choose a reason for hiding this comment

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

она еще true|false возвращает. Надо описать, когда какое значение возвращается и для чего

Copy link
Contributor Author

@cabad11 cabad11 Dec 28, 2018

Choose a reason for hiding this comment

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

true false возвращает стрелочная функция а не метод а метод всегда возвращает array

static async getWithoutChildren(parent) {
let pagesAvailable = this.removeChildren(await Pages.getAll(), parent);

return pagesAvailable.filter((item) => item !== null);
Copy link
Member

Choose a reason for hiding this comment

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

тут может быть не только array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

можно по подробнее
pages.getAll всегда возвращает array

Copy link
Member

Choose a reason for hiding this comment

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

твоя функция возвращает не только array

Copy link
Contributor Author

@cabad11 cabad11 Dec 28, 2018

Choose a reason for hiding this comment

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

функция в любом случае возвращает array так как там будет хотя бы сам вызываемый элемент parent

static async getWithoutChildren(parent) {
let pagesAvailable = this.removeChildren(await Pages.getAll(), parent);

return pagesAvailable.filter((item) => item !== null);
Copy link
Member

Choose a reason for hiding this comment

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

твоя функция возвращает не только array

/**
* Change wrong pages to null
*
* @returns page[]
Copy link
Member

Choose a reason for hiding this comment

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

не правильно задокументированы типы

Copy link
Member

Choose a reason for hiding this comment

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

и не описаны параметры

}

/**
* Change wrong pages to null
Copy link
Member

Choose a reason for hiding this comment

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

не понятное описание

* @param {string} parent - id of current page
* @returns {Promise<Page[]>}
*/
static async getWithoutChildren(parent) {
Copy link
Member

Choose a reason for hiding this comment

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

getAllExceptChildrends

@@ -22,7 +22,7 @@ router.get('/page/edit/:id', async (req, res, next) => {

try {
let page = await Pages.get(pageId);
let pagesAvailable = await Pages.getAll();
let pagesAvailable = await Pages.getAllExceptChildrends(pageId);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let pagesAvailable = await Pages.getAllExceptChildrends(pageId);
let pagesAvailable = await Pages.getAllExceptChildrens(pageId);

src/controllers/pages.js Outdated Show resolved Hide resolved
@@ -41,6 +41,38 @@ class Pages {
return Model.getAll();
}

/**
* @static
* Return all pages without children pages
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Return all pages without children pages
* Return all pages without children of passed page

* @static
* Set all children elements to null
*
* @param {Array<?Page>} pagesAvailable - Array of all pages
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {Array<?Page>} pagesAvailable - Array of all pages
* @param {Page[]} [pagesAvailable] - Array of all pages

neSpecc and others added 3 commits December 30, 2018 12:24
Co-Authored-By: cabad11 <44175180+cabad11@users.noreply.github.com>
change comments
change metod name
* @static
* Set all children elements to null
*
* @param {Page[]} [pagesAvailable] - Array of all pages
Copy link
Member

Choose a reason for hiding this comment

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

разве этот параметр опционален? не нужны квадратные скобки

@cabad11 cabad11 merged commit dbfc594 into master Jan 14, 2019
@cabad11 cabad11 deleted the child-element-as-parent branch January 14, 2019 14:55
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.

3 participants