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

Added filtering to request list on itinerary details. #1615

Merged
merged 3 commits into from Dec 16, 2016

Conversation

dchristensen
Copy link
Contributor

Fixes #1100

@tonysurma
Copy link
Member

@stevejgordon can you give this a quick review, @dchristensen thanks

@dpaquette dpaquette assigned dpaquette and unassigned dpaquette Dec 8, 2016
Copy link
Member

@stevejgordon stevejgordon left a comment

Choose a reason for hiding this comment

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

@dchristensen Firstly, thanks for this PR. It looks close but I ran into some issues when loading the site due to the form. (Please see comment).

Fixing up the form should make it function correctly. One further point though is that if the filter returns no requests the message we'll see is "There are currently no assigned requests for this itinerary". We might need to do something to ensure that this is more clear. Something like "No requests to show with current filtering". Of course if we actually have no records in the first place we should always show the original message. Does that make sense?

Thanks for including and expanding on the tests.

@@ -125,6 +127,20 @@
</div>
</div>
}
<form method="post" action="@Url.Action("Details", "Itinerary", new { area="Admin" })#requests" class="form-horizontal">
Copy link
Member

Choose a reason for hiding this comment

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

There seem to be a couple of problems with this form. Firstly it's not sending an anti forgery token so we get blocked by MVC (bad request) on the POST. The best option to fix this is to use the form taghelpers which will actually render an anti-forgery token for us automatically.

<form asp-action="Details" asp-controller="Itinerary" asp-area="Admin">
    // You inputs etc here
</form>

Secondly, I'm not clear why the #requests string is in there currently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stevejgordon The #request is being appended to the URL (and the tag helpers are not being used) so that the posted URL ends up as /Admin/Itinerary/Details/5#requests, for example. I'm adding the target hash because when the filter button is clicked and the page reloads you are left back at the top of the page. This felt like a bad experience for the user, especially if there are several team members assigned to the itinerary pushing the requests further down the page. This addition makes sure the the requests are shown at the top of the screen when the page loads.

I can just add in the older style @Html.AntiForgeryToken() inside the form unless you have a suggestion for an alternative solution.

Copy link
Member

Choose a reason for hiding this comment

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

@dchristensen - That's a fair reason! I'm not sure if it can be done within the form tag-helper. @dpaquette might have some better knowledge on that. But adding in the AntiForgery token might be the pragmatic approach.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have not tried adding a hash to a form action using the tag helper approach. If it doesn't work then adding @Html.AntiForgeryToken() should be sufficient here.

@tonysurma
Copy link
Member

@stevejgordon can you take a re-review? looks like @dchristensen updated post your comments. Thanks

Copy link
Member

@stevejgordon stevejgordon left a comment

Choose a reason for hiding this comment

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

Thanks very much @dchristensen. This worked fine with the anti-forgery added so we're good to :shipit:

Thanks for the contribution and the rapid review changes.

@tonysurma tonysurma merged commit 3b14821 into HTBox:master Dec 16, 2016
@dchristensen dchristensen deleted the issue-1100-request-filtering branch December 19, 2016 00:00
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

4 participants