Skip to content
This repository has been archived by the owner on Jul 17, 2020. It is now read-only.

Questions #88

Merged
merged 25 commits into from
Apr 13, 2020
Merged

Questions #88

merged 25 commits into from
Apr 13, 2020

Conversation

misha130
Copy link
Contributor

@misha130 misha130 commented Apr 8, 2020

Pull Request summary

  • Handles requests for a question list
  • Displays a page for a question list
  • Validation on said request
  • Unit tests on said requests

Link to Project Card/Issue

#78

Pull Request Tasklist

  • Pull request references an issue
  • Branch follows contributing guidelines format
  • Pull request targets the develop branch

@misha130
Copy link
Contributor Author

misha130 commented Apr 9, 2020

Well this is what we have so far @luap42
Image from Gyazo

@misha130 misha130 marked this pull request as ready for review April 11, 2020 13:18
@luap42 luap42 requested review from luap42 and asynts April 12, 2020 17:37
@luap42
Copy link
Member

luap42 commented Apr 12, 2020

Reviewing this now. I think the seed script is nice, but we'll need some "set up a community" script later, when we come closer to deploying it live.

@misha130
Copy link
Contributor Author

Reviewing this now. I think the seed script is nice, but we'll need some "set up a community" script later, when we come closer to deploying it live.

Something more complete, yes. Also pull since I added category handling

src/WebApp/Pages/Questions.cshtml Outdated Show resolved Hide resolved
@misha130 misha130 force-pushed the questions branch 3 times, most recently from f32cbcd to e7c7468 Compare April 12, 2020 23:39
src/WebApp/Pages/Questions.cshtml Show resolved Hide resolved
src/WebApp/Pages/Questions.cshtml Outdated Show resolved Hide resolved
src/WebApp/Pages/Questions.cshtml Outdated Show resolved Hide resolved
src/WebApp/Pages/Questions.cshtml Outdated Show resolved Hide resolved
src/WebApp/wwwroot/js/questions.js Outdated Show resolved Hide resolved
Copy link
Member

@luap42 luap42 left a comment

Choose a reason for hiding this comment

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

Looks fine now. Running the tests and then this should be ready for merge.


<div class="grid has-margin-4">
<div class="grid--cell is-flexible">
<h1 class="has-margin-0">Q&amp;A posts</h1>
Copy link
Member

Choose a reason for hiding this comment

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

Q&A posts sounds strange. Here should be the category name and/or the post type. So something like "Questions" or "Main Questions".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed this by adding to the query result the category but ideally I should have an area/component/whatever that does this separately from the questions query like some community data.

Also added a short explanation below the category name.

    <div class="grid--cell is-flexible">
        <h1 class="has-margin-0">@Model.Result.Category.DisplayName Questions</h1>
        <h3>@Model.Result.Category.ShortExplanation</h3>
    </div>

@luap42
Copy link
Member

luap42 commented Apr 13, 2020

Running the tests fails with this message:

screenshot of error message

Error Message:
System.Exception : Category not found:
Stack Trace
at Codidact.Core.Application.Questions.Queries.QuestionsQuery.QuestionsQuery.AddFiltersToQuery(QuestionsQueryRequest request, IQueryable`1 questionsQuery) in XXX\CodidactCore\src\Application\Questions\Queries\QuestionsQuery\QuestionsQuery.cs:line 69
at Codidact.Core.Application.Questions.Queries.QuestionsQuery.QuestionsQuery.Handle(QuestionsQueryRequest request) in XXX\CodidactCore\src\Application\Questions\Queries\QuestionsQuery\QuestionsQuery.cs:line 34
at Codidact.Core.Application.IntegrationTests.Questions.QuestionsQueryTests.QueryShouldReturnResultsByDefaults() in XXX\CodidactCore\tests\Application.UnitTests\Questions\QuestionsQueryTests.cs:line 30

@misha130
Copy link
Contributor Author

Running the tests fails with this message:

Updated to handle categories

Copy link
Member

@ArtOfCode- ArtOfCode- left a comment

Choose a reason for hiding this comment

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

This is mostly a syntax/style review, rather than techniques. Couple of minor changes, but mostly good to go.

</div>
</div>
<div class="button-list is-gutterless has-margin-top-4 has-float-right">
<a asp-page="questions" asp-route-sort="@QuestionsQuerySortType.Best" class="button is-muted is-outlined @(query.Sort == QuestionsQuerySortType.Best?"is-active":"")">best</a>
Copy link
Member

Choose a reason for hiding this comment

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

You're missing some spacing in the ternary expression in the class attribute here (and likewise in the next two lines).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could've been something like:

    @foreach (var sortType in (QuestionsQuerySortType[]) Enum.GetValues(typeof(QuestionsQuerySortType)))
    {
        <a asp-page="questions" asp-route-sort="@sortType" class="button is-muted is-outlined @(query.Sort == sortType ? "is-active" : "")">@sortType</a>
    }

But for now just formatted the condition

@@ -124,5 +133,137 @@ private void ApplyDatabaseMigrations(IApplicationBuilder app, ILogger logger)
}
}
}

private void SeedDatabase(IApplicationBuilder app, ILogger logger)
Copy link
Member

Choose a reason for hiding this comment

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

DB seed should probably be split off into a separate file/task at some point. Not one for this PR, though.

* Utilities to handle dates
*/
function utcDateTimeToLocalDisplay(date) {
Copy link
Member

Choose a reason for hiding this comment

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

Have we got Babel preprocessing set up yet? If not, we should make that a priority. If we do, this file should be reviewed and updated to ES6 - notably, const and () => { }.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet basically. Keeping it ES3 so far

Copy link
Member

Choose a reason for hiding this comment

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

Works for now. Just bear in mind that the longer it takes to get it set up, the more JS there'll be to rewrite when it's ready :)

@luap42
Copy link
Member

luap42 commented Apr 13, 2020

@misha130 please merge develop into this PR and resolve the conflicts, theb we can merge this PR.

@misha130
Copy link
Contributor Author

@misha130 please merge develop into this PR and resolve the conflicts, theb we can merge this PR.

Ok done

@luap42 luap42 merged commit ec407c4 into codidact:develop Apr 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants