Skip to content

Add DB models & views for fitting specifications#222

Merged
jonc125 merged 34 commits intomasterfrom
203-split-entity-table
Jan 17, 2020
Merged

Add DB models & views for fitting specifications#222
jonc125 merged 34 commits intomasterfrom
203-split-entity-table

Conversation

@jonc125
Copy link
Copy Markdown
Contributor

@jonc125 jonc125 commented Nov 28, 2019

Part of #203.

I've managed to generalise the entity views so most can now be used as-is for fitting specifications, for which the FittingSpec DB model has been added. It inherits from Entity but unlike the model & protocol cases has its own table in the DB to hold the protocol link.

The only views that have different versions for fitting specs are the following, so they can use different forms:

  • FittingSpecCreateView (adds the protocol link)
  • FittingSpecNewVersionView (removes the re-run experiments option)

All the following views are re-used for fitting specs at present. In several cases the templates or views have been adapted so some elements are only used for models/protocols.

  • EntityListView
  • EntityView
  • EntityVersionView (the Protocol link is not yet shown)
  • EntityVersionJsonView (planned experiments are not checked for fitting specs)
  • EntityComparisonView & EntityComparisonJsonView
  • EntityTagVersionView
  • EntityDeleteView
  • EntityVersionListView
  • EntityArchiveView
  • FileUploadView
  • ChangeVisibilityView
  • EntityFileDownloadView
  • EntityCollaboratorsView
  • EntityDiffView
  • EntityAlterFileView (not actually used, but trivial to include and saved avoiding linking to it)

The other entity views don't apply to fitting specs, although variations on the first two might when we have fitting results:

  • EntityCompareExperimentsView
  • EntityRunExperimentView
  • CheckProtocolCallbackView
  • GetProtocolInterfacesJsonView

To make the reuse work several things were needed:

  • EntityTypeMixin now knows about FittingSpec
  • We have url_type and display_type fields as well as entity_type to help with user-friendliness
  • A current_namespace context item is available in all templates so URL reversing can tell which app we're currently in (fitting or entities)
  • Changed entity tags significantly to work with the above

Other bits of note:

  • Created EntityManager.visible_to_user query for filtering protocols that can be linked to a new fitting spec. Also improved shared_with_user so it can be combined without using .union(). This will be handy elsewhere too: at least when creating datasets, and possibly in matrix views.

For future consideration:

  • The JSON views that serve up version & file info are almost-duplicated in lots of places. It might be best to move e.g. the bulk of EntityVersionJsonView into a mixin, and let subclasses override the url reversing.
    It's similar to DatasetJsonView & ExperimentVersionJsonView (though not quite identical), and parts of ExperimentComparisonJsonView.
    There's also overlap with EntityComparisonJsonView.

This also involved adding a new `create_fittingspec` permission and
linking it up in a few unexpected places.

Probably some more basic tests are still needed but it's a start.
@jonc125 jonc125 changed the base branch from master to 217-multiple-caches November 28, 2019 17:10
@jonc125
Copy link
Copy Markdown
Contributor Author

jonc125 commented Nov 29, 2019

With 9f77284 I'm thinking about how to implement views for fitting specs, initially around creation/upload. I don't want to have to duplicate lots of code and templates from the entities app. But equally some things will be different (like the new spec form having a protocol link) so some overrides will be necessary. I think doing it neatly will need some alterations to the generic entities views; I'm still thinking about how best to do so. Some will just be stylistic, e.g. adding a 'fitting specs' button alongside models and protocols; making the 'Your fittingspecs' heading decent English!

If running this locally you can see what I've implemented this far at http://localhost:8000/fitting/fittingspecs/

More thoughts added to PR description.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 29, 2019

Codecov Report

Merging #222 into master will decrease coverage by 0.4%.
The diff coverage is 40%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #222     +/-   ##
========================================
- Coverage    95.1%   94.7%   -0.5%     
========================================
  Files          65      65             
  Lines        2858    2662    -196     
  Branches      316     280     -36     
========================================
- Hits         2720    2522    -198     
- Misses         98     100      +2     
  Partials       40      40
Impacted Files Coverage Δ
weblab/conftest.py 99.3% <ø> (-0.1%) ⬇️
weblab/accounts/models.py 100% <ø> (ø) ⬆️
weblab/repocache/populate.py 78.5% <0%> (-8%) ⬇️
weblab/core/combine.py 100% <100%> (ø) ⬆️
weblab/repocache/models.py 96.8% <100%> (-1%) ⬇️
weblab/entities/repository.py 96.8% <57.1%> (-1.1%) ⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7970a24...2c04797. Read the comment docs.

@jonc125 jonc125 changed the base branch from 217-multiple-caches to master January 6, 2020 15:02
From my manual testing, it looks like everything we need is now
implemented! Except for automated tests, of course...
@jonc125 jonc125 marked this pull request as ready for review January 10, 2020 16:59
@jonc125
Copy link
Copy Markdown
Contributor Author

jonc125 commented Jan 13, 2020

This needs unit tests to be written for the fitting spec views, but from manual button pressing everything seems to work as expected. So probably worth reviewing and merging, with testing in a separate PR @sroderick-g5sro ? I've updated the description so it explains what was done.

And moved the list versions / add version links to buttons so we
don't just have a single button on its own! (And so the drop-down
doesn't mess up spacing.)
@jonc125
Copy link
Copy Markdown
Contributor Author

jonc125 commented Jan 15, 2020

Note before this is merged: we'll need to merge the latest master in to this branch then do manage.py makemigrations, since the fitting spec cache will need to have the new fields added.

@sroderick-g5sro
Copy link
Copy Markdown
Contributor

sroderick-g5sro commented Jan 15, 2020

I've started my review of this, but its going to take me sometime to understand the background of this change and then work through it. I am keen to try and use it from a user's perspective.

@jonc125
Copy link
Copy Markdown
Contributor Author

jonc125 commented Jan 16, 2020

I've started my review of this, but its going to take me sometime to understand the background of this change and then work through it. I am keen to try and use if from a user's perspective.

That's expected! I have no meetings this morning, so could do a call to walk you through the user's POV. It's discussed in text form in #135. For this PR, basically it's treating fitting specifications like models & protocols, in that they're versioned collections of files that a user can upload, backed by a git repository. So the same creation/view/etc UI applies. But with slight tweaks because they link to a protocol.

Which reminds me - I don't think I've included that protocol link in the detail view yet! Will try to add that today...

@sroderick-g5sro
Copy link
Copy Markdown
Contributor

Just a trivial point that when user is deleting an entity or fitting spec. Get a confirm button but not a cancel one. So have to use browser back. Can't remember if it was always like this - assume so

@sroderick-g5sro
Copy link
Copy Markdown
Contributor

sroderick-g5sro commented Jan 16, 2020

Sorry about v. minor point - when clicking on model / protocol /etc. you get some "flash of unformatted content" I think this is becuase takes time for the version number and detail to show up. probably not worth worrying about. I've attached a video

2020-01-16_16-28-06.zip

@sroderick-g5sro
Copy link
Copy Markdown
Contributor

I'm not too keen on the way that the "Compare..." button works. It took me a while to figure out that it contained a clickable link.

@jonc125
Copy link
Copy Markdown
Contributor Author

jonc125 commented Jan 16, 2020

Just a trivial point that when user is deleting an entity or fitting spec. Get a confirm button but not a cancel one. So have to use browser back. Can't remember if it was always like this - assume so

Yes, I think that was always the case. A cancel button as well makes sense though!

Sorry about v. minor point - when clicking on model / protocol /etc. you get some "flash of unformatted content"

Yes, that's in the delay while the Javascript code fetches the JSON metadata for the fitting spec and uses it to fill in the remaining page content. We could make the interim page a bit nicer though!

I'm not too keen on the way that the "Compare..." button works. It took me a while to figure out that it contained a clickable link.

For models & protocols there are at least two links there, which is why it behaves like a drop-down menu on hover. There may be a better way to do it, but that was the best I came up with, without using up too much screen space!

@sroderick-g5sro
Copy link
Copy Markdown
Contributor

sroderick-g5sro commented Jan 17, 2020

From the fitting specifications "tab" clicking on the name and "add new version" go to the same location. I expected to be presented with a list of files when clicking on the name. (I felt at one point it was doing this).

Please see video clip for replication. This is not the same behaviour as for models / protocols / datasets.

2020-01-17_14-37-33.zip

@jonc125
Copy link
Copy Markdown
Contributor Author

jonc125 commented Jan 17, 2020

That should happen if the original creation process was interrupted, i.e. if the spec was created in the DB but no files were added at the time. (It also happens for models and protocols I believe.)

@sroderick-g5sro
Copy link
Copy Markdown
Contributor

I get error building site when trying to compare to versions of a fitting spec

image

@jonc125
Copy link
Copy Markdown
Contributor Author

jonc125 commented Jan 17, 2020

I saw that, but thought I'd fixed compare.js so it doesn't happen. Have you re-built the static resources with gulp? I tend to keep gulp watch running in the background so it happens automatically. See the 'build statics' section of the readme.

@sroderick-g5sro
Copy link
Copy Markdown
Contributor

I have tried refreshing gulp and same is still happening. I have also put a couple of minor questions in the review

@jonc125
Copy link
Copy Markdown
Contributor Author

jonc125 commented Jan 17, 2020

That's odd, it's working locally. Maybe refresh the page just in case your browser has cached the JS?

@jonc125
Copy link
Copy Markdown
Contributor Author

jonc125 commented Jan 17, 2020

I have noticed that the 'click to show single version' feature in the drop-down isn't working once you get to the page though. But then, it's not working for models or protocols either!

Edit: that's because EntityComparisonJsonView doesn't provide a url field for versions. Will fix in #234.

@sroderick-g5sro
Copy link
Copy Markdown
Contributor

That's odd, it's working locally. Maybe refresh the page just in case your browser has cached the JS?

yes sorry was a caching problem - thanks

Comment thread weblab/fitting/tests/test_models.py Outdated
Comment thread weblab/entities/views.py
@jonc125 jonc125 merged commit fac3ac0 into master Jan 17, 2020
@jonc125 jonc125 deleted the 203-split-entity-table branch January 18, 2020 09:05
@jonc125 jonc125 mentioned this pull request Feb 11, 2020
MauriceHendrix pushed a commit that referenced this pull request Jul 22, 2021
Add DB models & views for fitting specifications
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