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

Remove container level treemaps #261

Merged
merged 5 commits into from
Dec 13, 2019
Merged

Remove container level treemaps #261

merged 5 commits into from
Dec 13, 2019

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented Dec 12, 2019

This PR will refactor the treemap to only show container counts per collection, and not size of containers - and we do this because not all avenues of adding a container support knowing the size.

@Aneoshun - it was actually the case that I had refactored the view to not rely on the third party (singularity) library, and it just wasn't showing up because the library pushed containers didn't have an associated size. It used to be that after a certain number of containers were added we flipped the view to show collection counts instead, so to fix this I removed the container level views and the variable, and now the map will just show collection sizes based on numbers of containers. The cron job was an artifact of a previous implementation that read in data from a json file -the rendered data for a few years now just comes from a csv dynamically rendered as a view (and I just forgot about it, heh).

Do you want to test this out? You should see a treemap for your collections based on containers per collection.

Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
Copy link
Contributor

@Aneoshun Aneoshun left a comment

Choose a reason for hiding this comment

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

This PR runs well on my side.
However, I am not sure that I see the new treemap.
To test, I have 2 collections, and a total of 3 containers. This is what the collections page shows me:
Screenshot 2019-12-13 at 14 02 52

And when I click on a collection I get this (the button view is gone as expected I think):
Screenshot 2019-12-13 at 14 03 06

Is it what I am supposed to see?
Also, when I go to tools/sizes (link provided on the index page and on the tool page) I get this:
Screenshot 2019-12-13 at 14 03 18

This says that there are 0 container in 0 collection, while this is wrong. I am "staff" on django so I should be able to view all collections... especially mine.

shub/apps/base/templates/main/tools.html Outdated Show resolved Hide resolved
Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
@vsoch
Copy link
Member Author

vsoch commented Dec 13, 2019

Ah - so for the container treemap, we do just show public collections, let me take a look at other permissions to see if staff/admin are allowed to see.

@Aneoshun
Copy link
Contributor

Indeed, making my collections public shows the collections in the treemap view.

Signed-off-by: Vanessa Sochat <vsochat@stanford.edu>
@vsoch
Copy link
Member Author

vsoch commented Dec 13, 2019

okay all set - I refactored the filter so that an admin/staff sees everything, and otherwise it's based on permissions (owner sees their own collections plus those of others that are public).

@Aneoshun
Copy link
Contributor

It's working on my side! As a staff, I see all the collections and containers, and as a normal user I can only see the public collections.

There is just something I don't understand, probably because I don't have the whole picture. Why do we need another library (singularity) to get the container size? Can't we use python to find the info (https://docs.python.org/3/library/os.path.html#os.path.getsize) as the files are actually hosted locally and the path/filenames are in the DB if I am correct?

@vsoch
Copy link
Member Author

vsoch commented Dec 13, 2019

We don't - that was a very old implementation (years ago) that I had originally built in to use the singularity python module. I refactored it (also years ago) to not need this, and have all the views directly with the server. So, we don't require that dependency anymore, I had just forgotten :)

@vsoch
Copy link
Member Author

vsoch commented Dec 13, 2019

@Aneoshun we can't do that because there are plugins for external storage that don't use the local filesystem.

@Aneoshun
Copy link
Contributor

Ahh I see. I did not have the whole picture!

Looks good to me then.

@vsoch
Copy link
Member Author

vsoch commented Dec 13, 2019

Great! Thanks again for your help :)

@vsoch vsoch merged commit 6bc2975 into master Dec 13, 2019
@vsoch vsoch deleted the remove/treemap branch December 13, 2019 17:38
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.

2 participants