Errors page

[Sammidysam]

I'm so glad you are working on this! It will help so much to have.

[Sammidysam]

Some usage comments:

• I think we should have some margin on this page, it looks like it's really crowding the edges.
• I think the instructions can be bullet points and not have so much spacing so that they are closer together.
• I am worried that a lot of my query string suggestions didn't make it across, so I guess we might need another call about this. I really care about this because this is an educational thing that will help you incredibly immensely.
• It is weird having the datetime chooser in AM/PM and the messages displaying 24-hour time. I would switch to 24-hour time for the chooser.
• "You can also filter add more lines or filter by date range (Advanced Options)" does not quite make grammatical sense, but isn't a huge deal.
• Could "error-log" be a decent name? Just sounds like standard. No big deal though.
• I think a title on the page might look nice.

Overall: functionality is super there and nice job! Just some things are done in weird ways which is why there are so many comments. Maybe it's not worth fixing all these things, but I think they will be easy since you have already done everything in kind of more complex ways.

[Sammidysam]

Ethan: good step! The moving of tailRead is not needed, so definitely feel free to just resolve that and move on if you prefer.

[ethanmichel0]

Some usage comments:

• I think we should have some margin on this page, it looks like it's really crowding the edges.
• I think the instructions can be bullet points and not have so much spacing so that they are closer together.
• I am worried that a lot of my query string suggestions didn't make it across, so I guess we might need another call about this. I really care about this because this is an educational thing that will help you incredibly immensely.
• It is weird having the datetime chooser in AM/PM and the messages displaying 24-hour time. I would switch to 24-hour time for the chooser.
• "You can also filter add more lines or filter by date range (Advanced Options)" does not quite make grammatical sense, but isn't a huge deal.
• Could "error-log" be a decent name? Just sounds like standard. No big deal though.
• I think a title on the page might look nice.

Overall: functionality is super there and nice job! Just some things are done in weird ways which is why there are so many comments. Maybe it's not worth fixing all these things, but I think they will be easy since you have already done everything in kind of more complex ways.

[ethanmichel0]

Below are my responses to your review, hopefully this is close to done. All other points were already addressed I believe.

• I think we should have some margin on this page, it looks like it's really crowding the edges. (Easy fix, but see comment below)
• I think the instructions can be bullet points and not have so much spacing so that they are closer together. (Actually started with bullet points but thought this looked better. I am using a list group right now, maybe I can make the padding smaller?
• Could "error-log" be a decent name? Just sounds like standard. No big deal though. Would "listOfErrors" be better? Or maybe "loggedErrorsList?"
• I think a title on the page might look nice. What do you think of imitating the "Locations" title on the locations page? This is a simpler header in the top left I think.

The only other thing I can think of potentially holding this back has to do with my comment above and with deciding what to do with redirects and throwing AccessDenied exceptions

[Sammidysam]

@ethanmichel0 https://www.environmentaldashboard.org/community-voices/api/locations The API provides a type of AccessDenied in the errors, so I'm thinking I'll just modify the ApiProvider to throw an AccessDenied on the frontend too when we get one from the API. It's not the most elegant, but it'll direct us to the access denied page fine.

[Sammidysam]

I'll do a final test soon which should help me figure out if I see anything else. I'm sure everything would be minor. One thing I just saw in the code was that /error-log/{lines} still exists. No need to if you're using a query string for customization like that now?

I did think smaller padding for the instructions would be nice, not sure what I would think if I looked again. It probably doesn't matter much, just whatever is easy.

I was discussing a different naming than you were in your reply for the error log, so no worries there.

Yes, "Locations" header is great to imitate.

[ethanmichel0]

You could define the number of lines to add as query->get('numLines') ?? 500

I don't think that this would work because if 'numLines' is empty this would evaluate to 500 instead of 0, which is the number of lines it should add, but I think something similar would work.

[ethanmichel0]

@ethanmichel0 https://www.environmentaldashboard.org/community-voices/api/locations The API provides a type of AccessDenied in the errors, so I'm thinking I'll just modify the ApiProvider to throw an AccessDenied on the frontend too when we get one from the API. It's not the most elegant, but it'll direct us to the access denied page fine.

Okay, and then we won't worry about rerouting at all?

[ethanmichel0]

I'll do a final test soon which should help me figure out if I see anything else. I'm sure everything would be minor. One thing I just saw in the code was that /error-log/{lines} still exists. No need to if you're using a query string for customization like that now?

I did think smaller padding for the instructions would be nice, not sure what I would think if I looked again. It probably doesn't matter much, just whatever is easy.

I was discussing a different naming than you were in your reply for the error log, so no worries there.

Yes, "Locations" header is great to imitate.

You could define the number of lines to add as query->get('numLines') ?? 500

I don't think that this would work because if 'numLines' is empty this would evaluate to 500 instead of 0, which is the number of lines it should add, but I think something similar would work.

Nevermind, it looks like the null coalescing operator evaluates the empty string as not null. I assumed it would evaluate it as null which was why I was confused.

[ethanmichel0]

I'll do a final test soon which should help me figure out if I see anything else. I'm sure everything would be minor. One thing I just saw in the code was that /error-log/{lines} still exists. No need to if you're using a query string for customization like that now? I did think smaller padding for the instructions would be nice, not sure what I would think if I looked again. It probably doesn't matter much, just whatever is easy. I was discussing a different naming than you were in your reply for the error log, so no worries there. Yes, "Locations" header is great to imitate.

You could define the number of lines to add as query->get('numLines') ?? 500

I don't think that this would work because if 'numLines' is empty this would evaluate to 500 instead of 0, which is the number of lines it should add, but I think something similar would work.

Nevermind, it looks like the null coalescing operator evaluates the empty string as not null. I assumed it would evaluate it as null which was why I was confused.

Would 'system errors' be a better name? This is what I titled the page. Also if you have a better idea than "errors-log" for the path name and the JSON return, I am totally happy for a suggestion

[Sammidysam]

@ethanmichel0 You can pass numLines in every request to the route after the first one, though, right? With a default value of zero? I'm a little confused why it needs to be empty except for on the first request, as you define it.

You can remove your rerouting code and I'll take care of it.

System errors is a great name. I like error-log as the route a tiny bit more because "Errors Log" just doesn't sound like how we express it in English, but I may be wrong there.

Also, you're going to want to merge master in and resolve conflicts sooner rather than later.

[Sammidysam]

@ethanmichel0 Check out my recent commit and see if it makes your redirecting unnecessary. You could also redirect upon catching an Access Denied, but I prefer letting the user know a bit.

[ethanmichel0]

@ethanmichel0 Check out my recent commit and see if it makes your redirecting unnecessary. You could also redirect upon catching an Access Denied, but I prefer letting the user know a bit.

I got rid of redirecting in this commit. I see this is something that could be nice maybe after seeing access denied, but this is also not very essential and out of the scope of this pull request.

[ethanmichel0]

@Sammidysam Merge conflict resolved. All of your comments are addressed this point I am pretty sure. Take a look to see if you like the style -- I added a title, reduced padding, and made the datetimepicker 24hr. Hopefully this is ready!

[Sammidysam]

Great!

• Can you make the usage instructions match the new URL? It says "errors" log.
• Can we have equal margin for all the content? It is weird having the title have margin and nothing else.
• I'd still have all instructions be one paragraph, but I mentioned that earlier so nbd.

Actual testing:

• I should send you the live server log file to test against. Unfortunately, I kind of blindly trusted that you used the efficient option I linked and it looks like you grabbed a different answer? Are you using https://stackoverflow.com/a/41634468/2397924 ? The efficient answer is at https://stackoverflow.com/a/15025877/2397924 and is https://gist.github.com/lorenzos/1711e81a9162320fde20 I believe. I'm just having reading 100 lines at the end of this file take 30 seconds and time out, but maybe we should just never allow log files this size? Could you take a look?

[Sammidysam]

I slightly feel like it's our PHP solution because running tail itself on the file results in an instantaneous read.

[Sammidysam]

Fixed two things - a bug I introduced in https://github.com/EnvironmentalDashboard/community-voices/pull/57/commits/91e6081f01abc1712f777fb7514b9c7fb3a2d465 and making sure that access control is run on the entire page (in a hacky way). Only want to go so far as make the default page work, so I will merge this and move on from here.