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

add possibility to restrict all stations by wildcards in network/station #52

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

megies
Copy link
Collaborator

@megies megies commented Sep 28, 2017

code.

Essentially makes it possible to hide all stations (or specific networks) for most users while making visible for some other users.

Restrictions seem to be untested in REST interface.. that's clearly something that should be done

@megies megies force-pushed the station_restriction_wildcards branch from dea3de9 to 32e0874 Compare September 28, 2017 20:07
@megies
Copy link
Collaborator Author

megies commented Sep 28, 2017

Probably it's also a good idea to relax the length restrictions on the network/station code fields, at some point. New developments in SEED or StationXML will most likely allow longer codes in the future..

Repository owner deleted a comment from codecov-io Sep 29, 2017
@krischer
Copy link
Owner

Looks pretty good - can you add a small test to this? ;-)

Also maybe add a small sentence to the documentation at the bottom of this page: http://krischer.github.io/jane/waveforms/index.html

@megies megies force-pushed the station_restriction_wildcards branch from 32e0874 to e5e8286 Compare September 29, 2017 10:52
@megies
Copy link
Collaborator Author

megies commented Sep 29, 2017

Test is not perfect, as it uses what was there already, with only a single station in the inventory.. but at least it's something

Repository owner deleted a comment from codecov-io Sep 29, 2017
station = models.CharField(max_length=5, db_index=True,
help_text=help_text)
users = models.ManyToManyField(
User, db_index=True,
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure if this has to be indexed - each index slows down the DB and I assume the other restrictions would already cut down the query enough so the user queries would be cheap? Not sure to be honest - might be worth to benchmark for a large database but the waveform table will be by far the biggest one so it has to reasonably fast.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didnt change the indexing, just added the help text.

Copy link
Collaborator

Choose a reason for hiding this comment

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

btw: changing the help_text requires a makemigrations too - even if it doesn't touch the database schema

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

huh.. on my local jane the help text shows up immediately..

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure - its working right away - but I bet you get also a new warning that a schemamigration is required ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

where should this warning pop up? at least in the debug mode runserver I dont see anything..

Copy link
Collaborator

Choose a reason for hiding this comment

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

python manage.py migrate

should show something like

  Your models have changes that are not yet reflected in a migration, and so won't be applied.
  Run 'manage.py makemigrations' to make new migrations, and then re-run 'manage.py migrate' to apply them.

Copy link
Owner

Choose a reason for hiding this comment

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

Please also commit the migrations.

Copy link
Collaborator Author

@megies megies Sep 29, 2017

Choose a reason for hiding this comment

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

tbh, the whole migrations stuff is still beyond me, I actually get errors due to some plugin changes when running python manage.py migrate, so hard to tell about warnings. usually I just scrap the whole DB and fill it in new right now (it's still pretty sandboxy)

@krischer
Copy link
Owner

Did you forget to push the tests? Also the new users field it not used currently.

@megies
Copy link
Collaborator Author

megies commented Sep 29, 2017

Did you forget to push the tests? Also the new users field it not used currently.

Yeah, forgot to commit the tests, so they weren't in the push.

The users field is not new, I just added the help text.

Repository owner deleted a comment from codecov-io Sep 29, 2017
for waveforms help text changes
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.

3 participants