arsmentis / coldfront

HPC Resource Allocation System
GNU General Public License v3.0
0 stars 0 forks source link

Add a ResearchOutput type freeform #8

Closed dmr-x closed 4 years ago

dmr-x commented 4 years ago

Current status is just barely a POC model.

When I last worked on this, I was having trouble getting Django's runserver.py makemigrations to work for the new model, but I resolved that in the past ~15m.

For WIP, I'll keep pushing commits here w/o force-push (unless it's cleaner to do so). I'll clean things up at stages where it seems good to, at which point I'll submit [INTERIM] commits and rebase.

Task list:

dmr-x commented 4 years ago

^ rebase onto ubccr/coldfront/master

dmr-x commented 4 years ago

Pushed a few commits. I've decided to drop the [FIXUP] ~label unless it's something worth calling attention to. If the commit message doesn't read like a "ready for upstream PR" message, then the commit is likely going to be squashed/fixup'd into others. (Or it needs to be reworded, but I'm now trying to do that before pushing to this PR.)

Updated the OP task list.

Here's a few things that are WIP, unpushed, for which I didn't update the task list but felt I should mention:

Task list:

* implementation
  * project view edits
  * new view/form/template for adding freeform research output

* unit/automated testing
  * form tests
  * view tests

* manual testing
  * templates
dmr-x commented 4 years ago

As of 0fd0ff28cd65fa0a1d0e8d63f0c78789147fb2eb (before any rebase/forcepush):

Task list:

* implementation

  * [snip]
  * model, refinements part 2

I don't see a need to refine the model further at this time, i.e. "part 2" is N/A.

* unit/automated testing

  * [x]  model tests
  * form tests
  * view tests

Use of ModelForm and CreateView with a few mixins, respectively, has eliminated most of the logic that has been present in prior PR's Views.

As such, I don't believe it's worth setting up automated tests for these, except for end-to-end testing (which is out of scope). @artlogic: please check out the Form and View implementation and let me know if you concur or disagree:

https://github.com/arsmentis/coldfront/blob/0fd0ff28cd65fa0a1d0e8d63f0c78789147fb2eb/coldfront/core/research_output/forms.py#L6-L9

https://github.com/arsmentis/coldfront/blob/0fd0ff28cd65fa0a1d0e8d63f0c78789147fb2eb/coldfront/core/research_output/views.py#L17-L46

For the time being, I'm going to cross them off the task list.

dmr-x commented 4 years ago

Updated the task list for:

Task list:

  • implementation
    • project view edits
    • restyling "manage project" card due to adding addition button, overflowing existing layout
    • new view/form/template for adding freeform research output
    • utility mixin for view: snake_case template name for multi-word model names
    • utility mixins: common copy/pasted code -> view mixins (not editing existing uses, just copy/paste to mixin instead of directly to view)
    • admin interface integration
    • Center Summary integration (minimal)
dmr-x commented 4 years ago

7ce16cf89b641d774f65961eedab5cb29ad68495 is a nice --no-ff merge commit that summarizes some of the key points (design or otherwise) of the contained commits.

Everything's been rebased/squashed/fixup'd/etc. now. (FYI: The upstream master didn't change since the last rebase done in this PR.)

artlogic commented 4 years ago

As such, I don't believe it's worth setting up automated tests for these, except for end-to-end testing (which is out of scope). @artlogic: please check out the Form and View implementation and let me know if you concur or disagree:

Agreed.

artlogic commented 4 years ago

Submitted to upstream, but please add the delete capability to this branch unless upstream has already merged.

https://github.com/ubccr/coldfront/pull/209

dmr-x commented 4 years ago

please add the delete capability to this branch unless upstream has already merged.

Added in 403d33cc211a5f0be18f4e258fceeb1d919b5fe4

dmr-x commented 4 years ago

@artlogic: Upstream has a comment on the PR:

Getting an error with the migrations when using MySQL:

_mysql_exceptions.OperationalError: (1170, "BLOB/TEXT column 'description' used in key specification without a key length")
django.db.utils.OperationalError: (1170, "BLOB/TEXT column 'description' used in key specification without a key length")

Looks like a length is required for the description column as it's being used in the constraint:

migrations.AddConstraint(
    model_name='researchoutput',
    constraint=models.UniqueConstraint(fields=('project', 'title', 'description'), name='unique_entry_in_project'),
),

It sounds like length is only required due to the constraint. In my opinion, it would be preferrable either to keep the length "unbounded" (at the db frontend anyways), or provide a suitably long length that would encompass any user's needs.

Between those choices, that'd be a question of database internals/optimizations - I can't speak to which might be better. Noteably, I could image the latter having some negative performance impact(s). But I simply don't know enough about the database implementations to judge here.

Perhaps we should re-think how we define uniqueness here. Maybe just simply use the ID at the db level and add some application logic for checking duplicates? The project, title, desc constraint doesn't seem very robust.

The main reason for the constraint here was to handle double-submitting at the database layer. (That it might catch some different-session same-content submissions was a bonus... with a freeform input I'd say the chance of that is super-unlikely unless the user was copy/pasting.)

An option in lieu of using length as a constraint is some finite-length hash on the content.

This was done in #4 to have a suitable unique_id for manually entered publications: https://github.com/arsmentis/coldfront/blob/73ed777550a29757ceec7f2980fe027d03614454/coldfront/core/publication/views.py#L294-L298

Please let me know your thoughts!