OKI Code review

[pwalsh]

• Please provide a stable URL to a staging version of the app (stable enough to be more or less up every day)

  • Deploy staging app with stable url #123 and maybe Automate deployment of staging app #124

• As far as I can see, zappa does not require one to add zappa-specific code into a codebase itself. However, our codebase has many references to Zappa. Why? If you need to inject one or a few Zappa variables, then do it as a context processor so such vars can be set cleanly in one place, rather then as part of function signatures throughout the codebase

  • Add required application variable to context processor  #127

• FYI Flask has a built-in CLI for management scripts, making the old Flask migrate package obsolete.

  • Migrate to Flask cli from Flask-script #252

• Please don't have fake/dummy data in the actual codebase. What is this function here for anyway?

  • This for pushing initial demo data into system

• More dummy data here makes me wonder if these commands work for general usage? Why are they in the codebase and not, for example, in the test suite?

  • We need initial user data. Atleast one admin user to start with.

• Please use Postgres 9.5 or 9.6

  • When we first deployed 9.5 was not available in travis. Right now it is there. Will do it [fix] Fixes small issues from OKI code review #126

• Is there a valid reason, such as a dependency that we don't need, to not support Python 3?

  • Python 3 support was started with Flask 0.10. As of 0.11 most libraries (including Werkzeug and Flask) might not quite as stable on Python 3 yet. You might therefore sometimes run into bugs that are usually encoding-related.

• There are a range of roles here. I need some documentation on users, publishers and roles please.

  • There is some detailed doc in [epic] [auth] User authorization for actions #108 . But if we need that in code as docstring will add [doc] Authorization setup #161

• Tests that are "not 404" do not test that a page exists ;)

• All new users are OWNERS?

  • For every new user a publisher is created of which it is the owner

• These bitstore tests as in the models test module. it is a bit confusing.

  • There exists a model namedBitStore , it has all s3 related operations. Thats why it is in test_model.py file

• Small issue, but way repeat info across inheriting classes in config? e.g.: here and here. Related, if there is a development config, why is there not a production one too?

  • [fix] Fixes small issues from OKI code review #126

• Related to similar notes here, I need a good explanation of the interaction between auth0 as a provider, our use of JWT, and the fact that we also have email/password.

  • See [doc] Authentication - auth0 and JWT #162
  • Add auth0 setup docs datopian/datahub-docs-tech#14

• We need more docstrings. For example, what actually is Catalog? I know data packages and this problem space in generally really well, but looking at the fields on the model, I'm not sure what catalog objects actually are. [fix] Fixes small issues from OKI code review #126

• Is this a validate function for a data package? if yes, why like this, and if not, what is it?

  • It was a very early implementation. Opening Validate metadata in data packages #125

• PUT and not POST - why?

  • PUT is always used for creating and POST for update and modify. Although we can update through this api, but wanted to specify that this api used for creation

• this pushes metadata to a database, but this only purges the filesystem and not the database

  • this line delete data packages from filesystem and this line deletes data from DB

• We don't yet have any search functionality at all, and some other API methods from the requirements are not implemented (ref.).

  • We are not implementing again the product requirements but against the priority user stories which were more limited.

• We don't cache resources if the DP has remote resources

  • We don't. Ditto to last point. Nice to have but not on the critical path we identified 😉

• While the client expects the server to validate data in data packages, I can't find any functionality where the server does this validation.

  • Another item where this is not on the immediate roadmap. No issue for that for that reason but we do have a user story.

[pwalsh]

About docs for users/permissions/roles being in #108 well, it is a closed issue :). Please add the documentation to the README so it is discoverable, and also add docstrings.

About documenting Catalog - I gave catalog as an example, but the whole codebase needs useful docstrings, so #126 does not (adequately) address this.

About PUT and POST - your usage is at odds with the general usage of PUT and POST. Here is a simple description. I don't care for sticking to conventions for convention's sake, but in this case, it makes little sense to deviate from pretty clear usage of PUT and POST in rest API design.

About purging, you are right, I missed that line. However, it made me realise that it is possible to purge one or the other and thereby lose the sync between bitstore and DB.

Search and caching need to be done. They are really foundational aspects of the registry. We can discuss when the other major points are addressed.

[rufuspollock]

Moving to Backlog. A good amount was done in 19 Dec sprint and remaining items will get done at different times.

[rufuspollock]

FIXED. Mainly fixed. Main outstanding items are large things that are about major features (and Search is in progress).

We can def revisit this in next round of reviewing.