search

arsmentis / coldfront

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

[FOR-REVIEW] Add support for specifying publications manually #4

Closed dmr-x closed 4 years ago

dmr-x commented 4 years ago

NOTE: I'm basing this pull request on #2, since they deal intricately with the same area of code. It wouldn't make sense to write this off master - there'd then be a lot of conflicts (merge or otherwise). As such, I've set the base on the PR to #2's branch to avoid confusion in the commit listing.

https://github.com/arsmentis/coldfront/commit/371fa1c77d28614aaea1ec141a72dabc3a7173fa through https://github.com/arsmentis/coldfront/commit/9c8ae3fe4698dde84398b64cc9e776002078f8f4 are related to this feature only by necessity - reworking/fixing* the same area of code this PR makes use of.

* as in: fixing the security vulnerability (and misplaced client trust) in https://github.com/arsmentis/coldfront/commit/9c8ae3fe4698dde84398b64cc9e776002078f8f4

The crux of this PR is in https://github.com/arsmentis/coldfront/commit/5510d80a0e12c2dc602a1bbb799d6409bdd442c5 - but I broke out a few other commits because it's easier to review those separately.

@artlogic this is ready for review

Here's a task list:

dmr-x commented 4 years ago

fix publication-deletion endpoint [[out of scope]] (see PR comment)

Known issue:

POST /publication/project/{project}/delete-publications/ fails for almost-duplicate publications. The Delete Publications feature does not make use of guaranteed uniqueness in selecting publications to delete, but rather pulls them by Title and Year.

code snippet:

publication_obj = Publication.objects.get(  
    project=project_obj,                    
    title=publication_form_data.get('title'),
    year=publication_form_data.get('year')  
)                                           
publication_obj.delete()

This PR doesn't cause the issue, but it makes the problem much more noticeable. Hence, I'm calling it out of scope.

@artlogic let me know how you'd like to have this handled.

artlogic commented 4 years ago

fix publication-deletion endpoint [[out of scope]] (see PR comment)

Known issue:

POST /publication/project/{project}/delete-publications/ fails for almost-duplicate publications. The Delete Publications feature does not make use of guaranteed uniqueness in selecting publications to delete, but rather pulls them by Title and Year.

code snippet:

publication_obj = Publication.objects.get(  
    project=project_obj,                    
    title=publication_form_data.get('title'),
    year=publication_form_data.get('year')  
)                                           
publication_obj.delete()

This PR doesn't cause the issue, but it makes the problem much more noticeable. Hence, I'm calling it out of scope.

@artlogic let me know how you'd like to have this handled.

Please make an brief issue for this which includes a succinct fix and the time it will take. Thanks.

dmr-x commented 4 years ago

@artlogic I pushed a few more commits which you may want to review. 3670f4a6da5376e52c4ce81f06f1fa6c0ad376c7 makes PublicationAddView ~reverted, in so far as this PR is concerned. Any changes left there are from parts of the PR that won't be forwarded upstream under this context (i.e. out of scope).

b85b964d49ce704ae2eb5f296ae7523344805f04 is a worthwhile change to review - it's taking the useful parts from the prior multi-purpose PublicationAddView and putting them into a single-purpose view. (There are some changes to views.py higher up in the file, but they're largely minor e.g. imports.)

finally: 61ade14f69c01cf2109ba617132599a403c14a5a should make the rest of the review easy on you - it changes the files you've already reviewed... to use the new view.

Now rebasing... will report back...

dmr-x commented 4 years ago

Force-pushed a rebased commit set. Making sure it works...

Note: this is based on the HEAD of #2. If that's not appropriate, we'll have to come up with a strategy that makes this work pre-#2 and post-#2. Either we:

(I think the former makes the most sense.)

dmr-x commented 4 years ago

Manually tested, found a minor bug, fixed (f5910ff), re-tested manually, re-rebased/--no-ff-merged, pushed.

@artlogic: All good for upstream.

dmr-x commented 4 years ago

A tab I had open answered the initial problem (an AttributeError, which you can see as a comment).

After fixing that and whitespace (1e04c2d), this comes:

Creating test database for alias 'default'...
.s...E
======================================================================
ERROR: test_add_publication_manually (coldfront.core.publication.tests.TestPublicationAddManuallyView)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../coldfront/venv/lib/python3.7/site-packages/django/shortcuts.py", line 93, in get_object_or_404
    return queryset.get(*args, **kwargs)
  File ".../coldfront/venv/lib/python3.7/site-packages/django/db/models/query.py", line 408, in get
    self.model._meta.object_name
coldfront.core.project.models.Project.DoesNotExist: Project matching query does not exist.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File ".../coldfront/coldfront/core/publication/tests.py", line 263, in test_add_publication_manually
    response = view(request)
  File ".../coldfront/venv/lib/python3.7/site-packages/django/views/generic/base.py", line 71, in view
    return self.dispatch(request, *args, **kwargs)
  File ".../coldfront/coldfront/core/publication/views.py", line 253, in dispatch
    Project, pk=self.kwargs.get('project_pk'))
  File ".../coldfront/venv/lib/python3.7/site-packages/django/shortcuts.py", line 95, in get_object_or_404
    raise Http404('No %s matches the given query.' % queryset.model._meta.object_name)
django.http.response.Http404: No Project matches the given query.

----------------------------------------------------------------------
Ran 6 tests in 0.071s

FAILED (errors=1, skipped=1)
Destroying test database for alias 'default'...
System check identified no issues (0 silenced).

crux:

coldfront.core.project.models.Project.DoesNotExist: Project matching query does not exist.

No idea why such a thing happens. Perhaps the Project from the factory needs some more attributes to it? I haven't looked any further into it.

dmr-x commented 4 years ago

@artlogic: ^

dmr-x commented 4 years ago

Force-pushed a rebase onto ubccr/coldfront/master. Tests excluded from that, but still available in arsmentis/coldfront/wip/manual-publication_/tests.

artlogic commented 4 years ago

Submitted to upstream sans tests: https://github.com/ubccr/coldfront/pull/210