search

datopian / ckanext-opendatani

GNU Affero General Public License v3.0
4 stars 5 forks source link

Feature/report resources by organization #8

Closed mpolidori closed 4 years ago

mpolidori commented 4 years ago

@zelima I made the requested changes. I'll be able to work on the last part (download button) in the morning.

mpolidori commented 4 years ago

@zelima I made the changes. I also got the download page and functionality working. There are some things I think I need to work on still visually.

I haven't figured out how the user is going to get to the page without directly typing the URL yet. A button on the organization page that directs there?

I also started experimenting with a preview of the report, but I haven't got it working yet.

EDIT: I just added the JSON report output and re-implemented the button. For some reason, the JSON is only displayed and not downloaded. All of the code is the same as the CSV, so I'm not sure on what's going on with that yet.

zelima commented 4 years ago

Also, a general comment would: You don't need to create a commit for every small change - that makes it hard to review. I would recommend doing commit per item/area you are working on. Eg Enable download button on the organization page and that would include every change that was related to that like button on HTML page, a helper function that is called by that button, etc... (that is just an example, not a direct parallel 😀)

zelima commented 4 years ago

@mpolidori also just noticed this

I haven't figured out how the user is going to get to the page without directly typing the URL yet. A button on the organization page that directs there?

Yes, the button should be on the organization page. Also, that button should be only visible for Sysadmins and admins of that organization (use the helper function that checks for that )

I also started experimenting with a preview of the report, but I haven't got it working yet.

We don't need a preview, do we? Do not complicate things :)

I just added the JSON report output and re-implemented the button. For some reason, the JSON is only displayed and not downloaded. All of the code is the same as the CSV, so I'm not sure on what's going on with that yet.

It's OK that JSON opens in a new tab.

mpolidori commented 4 years ago

@zelima Okay. I think everything looks good now (sorry for all the commits again, I kept missing small things).

zelima commented 4 years ago

Awesome. Think we can merge now and see this in prod.