Timestamp for questions #671
Conversation
There was a problem hiding this comment.
Hi, thank you for your PR! 🎉
Can you please rebase it on the current master? That makes reviews much easier (esp. because the master has an improved docker setup).
Another thing is that it definitely breaks unit tests as the majority of them runs on a in memory sqlite database which has no "legacy" tables like Questions. To prevent these errors please have a look at the other migrations :)
vendor/bin/phpunit -vvv --testsuite Unit --coverage-text --coverage-html /tmp/coverage/
PHPUnit 8.4.1 by Sebastian Bergmann and contributors.
Runtime: PHP 7.3.11-1+0~20191026.48+debian9~1.gbpf71ca0 with Xdebug 2.7.2
Configuration:~/code/engelsystem/phpunit.xml
.....................E..........EEEEEEEEE.E.EEEEE.............. 63 / 313 ( 20%)
......................EEE...................................... 126 / 313 ( 40%)
........................................................EEEE... 189 / 313 ( 60%)
........................E...................................... 252 / 313 ( 80%)
.................EEEEEEEEE................................... 313 / 313 (100%)
Time: 19.69 seconds, Memory: 46.00 MB
There were 33 errors:
[...]
|
Thanks for the feedback. I will update the branch. master branch fails on 4 test classes with the error 'Unable to connect to database':
and the LegacyDevelopmentTest throws a bunch of errors and then exhausts my memory. I am running the test through docker, do you have similar issues? |
|
If you run After rebasing your branch on the current master and running the dev containers you will be able to just run |
|
pff, was a lot of work but I think I have everything including a unit test for the questions model. |
MyIgel
left a comment
There was a problem hiding this comment.
A merge is not a rebase and i see some opportunities to squash some commits together to have a cleaner history ;)
You should really use your IDE's auto formatting feature or use a linter for that as there are some whitespace problems in the code ;)
Another benefit is that a proper IDE can use docblock hints to know which magic method is available for a class which is rally nice if working with models
Following are some comments on the code:
| /** @var \stdClass[] $previousQuestionsRecords */ | ||
| $previousQuestionsRecords = $this->schema->getConnection()->table('Questions')->get(); | ||
| foreach ($previousQuestionsRecords as $previousQuestions) { | ||
| DB::table('questions')->insert([ |
There was a problem hiding this comment.
This is the same as $this->schema->getConnection()->table('questions') so it should be used to make it clearer (and a $connection = $this->schema->getConnection(); to keep the two lines shorter) :)
| /** | ||
| * Recreates the previous News table, copies back the data and drops the new news table. | ||
| */ | ||
| public function down(): void |
There was a problem hiding this comment.
It would be clearer if the up() and down() methods (and thus all public methods) would be at the beginning of the class
There was a problem hiding this comment.
I can move it, I actually took the order and based the code on another PR
| */ | ||
| private function createNewQuestionsTable(): void | ||
| { | ||
| $this->schema->create('questions', function (Blueprint $table) { |
There was a problem hiding this comment.
Having two tables with the same name is not the best idea as some dbms are case insensitive wo this migration would fail. A workaround is to rename the old table first (like [TableName]Old).
| { | ||
| $this->schema->create('questions', function (Blueprint $table) { | ||
| $table->increments('id'); | ||
| $table->unsignedInteger('user_id'); |
There was a problem hiding this comment.
Using the referencesUser from the \Engelsystem\Migrations\Reference trait would be the clearer option here and on answer_user_id as it also sets cascade options.
There was a problem hiding this comment.
That's actually really handy :D
| 'id' => $previousQuestions->QID, | ||
| 'user_id' => $previousQuestions->UID, | ||
| 'answer_user_id' => $previousQuestions->AID, | ||
| 'question' => $previousQuestions->Question, |
There was a problem hiding this comment.
Proper indentation would be nice, i highly recommend the auto formatting function of your IDE if you are using one
includes/pages/admin_questions.php
Outdated
| if (!empty($question)) { | ||
| DB::delete('DELETE FROM `Questions` WHERE `QID`=? LIMIT 1', [$question_id]); | ||
| engelsystem_log('Question deleted: ' . $question['Question']); | ||
| $questions->delete(); |
There was a problem hiding this comment.
This looks like you want to delete all of them, just using $question->delete() is really better
There was a problem hiding this comment.
I agree, as mentioned above this was due to basing the model on the old table.
I will refactor
includes/pages/admin_questions.php
Outdated
| 'SELECT * FROM `Questions` WHERE `QID`=? LIMIT 1', | ||
| [$question_id] | ||
| ); | ||
| $questions = Question::where(['id' => $question_id]); |
There was a problem hiding this comment.
Using Question::find($question_id) directly returns the question or null
There was a problem hiding this comment.
You are right, I had built the model based on the old table I was forced to do it like this due to the delete not working correctly due to a lack of an id column.
I will refactor this as it should now be possible to delete normally.
includes/pages/admin_questions.php
Outdated
| DB::delete('DELETE FROM `Questions` WHERE `QID`=? LIMIT 1', [$question_id]); | ||
| engelsystem_log('Question deleted: ' . $question['Question']); | ||
| $questions->delete(); | ||
| engelsystem_log('Question deleted: ' . $question->Question); |
There was a problem hiding this comment.
Lowercase ->question and ->text ;)
includes/view/Questions_view.php
Outdated
| { | ||
| foreach ($open_questions as &$question) { | ||
| $question['actions'] = form([ | ||
| $question->actions = form([ |
There was a problem hiding this comment.
Updating the function docblock to contain * @param Question[] $open_questions is really helpful when using an IDE
There was a problem hiding this comment.
A merge is not a rebase and i see some opportunities to squash some commits together to have a cleaner history ;)
I am fairly new to rebase as I usually work with branches at work.
My apologies for this.
You should really use your IDE's auto formatting feature or use a linter for that as there are some whitespace problems in the code ;)
Yeah, I'm using phpstorm but I didn't set it up for the engelsystem. My bad, I will go over the changes again with a configured phpstorm.
Another benefit is that a proper IDE can use docblock hints to know which magic method is available for a class which is rally nice if working with models
I use phpstorm but I am strongly against the use of magic methods. I will implement them though to keep the project consistent and to reduce some complexity :)
Following are some comments on the code:
Thank you for taking the time to review, I will try to have the changes done by today
There was a problem hiding this comment.
PHPStorm has a nice auto formatter, setup should only be selecting PSR-2 (or 12).
Well as magic methods make it much more clearer they should definitely be preferred ;)
includes/pages/user_questions.php
Outdated
| 'SELECT * FROM `Questions` WHERE NOT `AID` IS NULL AND `UID`=?', | ||
| [$user->id] | ||
| ); | ||
| $open_questions = Question::where('answer_user_id', null)->where('user_id', $user->id)->get(); |
There was a problem hiding this comment.
Same here: Question::whereUserId($user->id)->where('user_id', $user->id)->get()
|
Hi, any updates on this PR? |
No sorry. I had reinstalled my computer a few weeks ago and will need to get the system back up and running again and tbh. I'll take a look at it on Friday if that's ok with you? |
|
Its totally ok, just wanted to know if you want to do something with it someday ;) |
|
@allentje any updates on this PR? |
f6acd70 to
8e5a67c
Compare
What does this PR do?
This PR adds timestamps to the question e.g created_at for when a new question is asked, answered_at for when the question is answered and an updated_at every time the question is updated.
This is intended to solve the issues #225
Some checks have been made simpler but in general I have kept everything the same only now it uses a model instead of static queries.
Where should the reviewer start?
The reviewer has to go to the
Frag den Himmelpage and create a question.The reviewer should also go to the
admin -> Fragen beantwortenpage to see the list of questions ready to be answered.How should this be manually tested?
Frag den Himmelpage and create a questionadmin -> Fragen beantwortenpageUnbeantwortete Fragen?Beantwortete Fragen?Documents
What should happen on deployment? (check all that apply)