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

maked test task #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

maked test task #3

wants to merge 1 commit into from

Conversation

NikitaNO
Copy link

@NikitaNO NikitaNO commented Mar 1, 2018

No description provided.

Copy link
Owner

@aichholzer aichholzer left a comment

Choose a reason for hiding this comment

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

  • No DELETE methods have been implemented,
  • Controllers have not been changed,
  • Changes in the current .pug files are unnecessary,
  • Code quality does not pass,
  • No description in the PR about what has been done (and why)

doctype html
html(lang='en')
head
meta(charset='UTF-8')
Copy link
Owner

Choose a reason for hiding this comment

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

These meta, link and script tags are now duplicated and will need to be updated in two places if anything changes. The reason for using a .pug mixin is to prevent exactly that. These should have been placed in the mixin so they can be used everywhere.

Most of these were already defined in the mixin: /ExpressBoilerplate/app/core/views/_components/html.pug

+body()
doctype html
html(lang='en')
head
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above; duplicates.

form.form-container#form
input(type='text', id='name', placeholder='Please enter name', name='name')
input(type='email', id='email' placeholder='Please enter name email', name='email')
input(type='text', id='city' placeholder='Please enter city', name='city')
Copy link
Owner

Choose a reason for hiding this comment

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

The user model has a reference to cities. Cities, in this case, should have been a drop-down selector with available cities. The connecting reference would be the city id.

From the user model:

city: {
  type: mongoose.Schema.Types.ObjectId,
  ref: 'City',
  default: null
}

e.preventDefault();
const validationFail = validation();
if (!Object.keys(validationFail).length) {
$.post(`/cities/${name}`, $('#form').serialize())
Copy link
Owner

Choose a reason for hiding this comment

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

POST /cities does not use the request paramenter. In order to create a city the request body is used;

const city = await m.city.create(req.body);

And where is the user creation being handled?

link(href='https://fonts.googleapis.com/css?family=Lato' rel='stylesheet')
link(href='/css/home.min.css', rel='stylesheet')
link(href='/css/form.min.css', rel='stylesheet')
script(src='https://code.jquery.com/jquery-3.3.1.min.js')
Copy link
Owner

Choose a reason for hiding this comment

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

script tags should not be placed in the head, but rather in the body, at the end of the closing tag.

@@ -0,0 +1,43 @@
(function() {
Copy link
Owner

Choose a reason for hiding this comment

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

Code does not pass lint rules; npm run lint
(Rules in /ExpressBoilerplate/.eslintrc.json must be updated)

ESLint errors

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.

None yet

2 participants