search

JaysGitLab / cs5667-final-project-revdev

cs5667-final-project-revdev created by GitHub Classroom
MIT License
1 stars 0 forks source link

Event ejs #39

Closed caldwellc1 closed 6 years ago

caldwellc1 commented 6 years ago

Did have them on separate branches but couldn't really test them if they weren't together. Since I can't run the test on my end here is what I have.

@haietza edit: Resolves #24. Resolves #27. Resolves #26. Let's handle list views in a separate branch/pull request for simplicity.

caldwellc1 commented 6 years ago

@rivendell10 @haietza @patrickbeekman I fixed some of the errors and added more test, but since I can't run them I'm not sure where they stand.

caldwellc1 commented 6 years ago

@haietza Ok, how about it now? It also might be better to go over it on Wed if there are other changes. Also on your review it says need ren, what does that mean?

haietza commented 6 years ago

@caldwellc1 I pulled the branch down and made a few changes; I thought it might be easier for you to diff between your last commit and the last commit I just pushed to see what changes I am suggesting. They are also listed per file in the commit messages. The create event feature is working both on the web interface and all the tests are passing with the code I just pushed. Take a look and we can talk more on Wed if needed.

caldwellc1 commented 6 years ago

@rivendell10 @haietza That's weird. It's not a problem with the model is it? It should be working the same as the reservation with default values.

rivendell10 commented 6 years ago

in the ejs view "createRes.ejs" line 19 is a typo -> starttime instead of startTime

haietza commented 6 years ago

@rivendell10 @caldwellc1 https://stackoverflow.com/questions/45208127/mongoose-accepts-null-for-number-field Looks like null is a "valid" input for saving, so the defaults are not getting applied. We can either make these fields required or add custom validators for them. The former is probably the simplest and those fields probably should be required anyway. What do you guys think?

rivendell10 commented 6 years ago

@haietza ok, I agree. Let's make the fields required.

haietza commented 6 years ago

@rivendell10 Okay I've made all the event model fields required, the form seems to work (won't allow submission of null values now) and tests are all passing. Something interesting to note - the requirement tests are done in reverse order of the form - the last field's error message comes up first, then the next to last, etc.

We may want to revisit the user and reservation models with regard to this as well.