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

Reservation rate excel report #338

Merged

Conversation

HotStew
Copy link
Contributor

@HotStew HotStew commented Mar 2, 2022

Created capability of downloading the new reservations rate excel report.
Backend PR: andersinno/respa#52

To be done

  • Tests
  • Making sure project runs on newer node versions
  • Reset state in modal after closing (Elaborated below)

My components somewhat differ from other components in that my have their own state defined within the components. I don't have too much experience with how things are done in this project with the reducers and the selectors so maybe having their own state is wrong?


Modal location

image


Unhidden modal with some initial data.

image


Input some data

image


Clicking download downloads report
image


Correct filters in excel file

image
image

Kevin Seestrand added 2 commits March 9, 2022 19:10
Create the action for fetching the reservation
rate report excel.

Also add extensions for filenames for all report actions
and modify the downloadReport function to not append
.docx to filename anymore. This had to be done because
all previous reports are .docx but this new one is xlsx.
Conditionally render datepicker in DateTimeRange
component. Defaults to always rendering it unless
specifying not to render it in controlprops.

The Time range part of the DateTimeRange component
is nice and useful, so now basically you are able
to just render that part.
@HotStew HotStew force-pushed the reservation-rate-excel-report branch 2 times, most recently from f77c422 to 0f5e550 Compare March 9, 2022 17:46
Copy link

@frwickst frwickst left a comment

Choose a reason for hiding this comment

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

Overall this is looking good. I would have prefered having the DateTime component be split up into two different components, since having something called DateTime and then you might only render a Date component can be strange to a developer.
But since I don't know how much refactoring work that would require in this case, I will not require that now.

Just a few things that would be good to check before merging this :)

Good work!

Copy link

@frwickst frwickst left a comment

Choose a reason for hiding this comment

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

wups, wrong selected state by me :D

@HotStew HotStew force-pushed the reservation-rate-excel-report branch from 0f5e550 to 8e9efab Compare April 4, 2022 10:22
@HotStew
Copy link
Contributor Author

HotStew commented Apr 4, 2022

I'd also like an opinion/input on the resetting modal state I mentioned in the PR description.

@HotStew
Copy link
Contributor Author

HotStew commented Apr 4, 2022

The warning messages below are due to package called shelljs which only eslint depends on. This error was introduced when I ran the project on node 14. Apparently it is no big deal. Though, I did try to upgrade eslint 4.x to get rid of the warning messages, but it would have been to much of a hassle because it created a chain effect of updating all the peer dependencies and re-configuring some things which I have no experience of really.

See:
nodejs/node#32987
shelljs/shelljs#973

(node:1328200) Warning: Accessing non-existent property 'cat' of module exports inside circular dependency
(Use `node --trace-warnings ...` to show where the warning was created)
(node:1328200) Warning: Accessing non-existent property 'cd' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'chmod' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'cp' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'dirs' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'pushd' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'popd' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'echo' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'tempdir' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'pwd' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'exec' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'ls' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'find' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'grep' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'head' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'ln' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'mkdir' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'rm' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'mv' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'sed' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'set' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'sort' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'tail' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'test' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'to' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'toEnd' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'touch' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'uniq' of module exports inside circular dependency
(node:1328200) Warning: Accessing non-existent property 'which' of module exports inside circular dependency

Copy link

@frwickst frwickst left a comment

Choose a reason for hiding this comment

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

LGTM

@joonvena
Copy link
Contributor

I'd also like an opinion/input on the resetting modal state I mentioned in the PR description.

So the problem is that when the modal is closed the previous state inputted there persist instead of resetting the values?

Kevin Seestrand added 4 commits April 12, 2022 11:17
Create actions reducers and selectors for Modal.
Create component ReservationRateReportModal. This modal lies
in the /reservations page and can be unhidden/shown in
the dropdown of <Lataa Raportti> -> Varausasteraportti.

This modal will take the basic props the modal itself requires
along with units and the action which downloads the report.

Within the modal there are fields where the user can select/
input data: Units, Date range and Time range. Unit seletion field
is a Typeahead component which is also a new package. Lastly
there is the download button which when clicked, will call
the api action to download the report.
Errors were mostly due to incorrect proptyping
Install node-sass that is compatible with
node version 14.X.X

Add a css loader for testing.
@HotStew HotStew force-pushed the reservation-rate-excel-report branch from 8e9efab to df247f4 Compare April 12, 2022 08:17
@HotStew HotStew force-pushed the reservation-rate-excel-report branch from e3e150d to df247f4 Compare April 21, 2022 08:29
Copy link

@frwickst frwickst left a comment

Choose a reason for hiding this comment

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

👍

@HotStew HotStew force-pushed the reservation-rate-excel-report branch from ceaf600 to df247f4 Compare April 27, 2022 10:32
Remove travis.yml for now so we can merge PRs.
@antti-mikael antti-mikael merged commit 8e0e08a into City-of-Helsinki:develop Apr 28, 2022
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