search

arsmentis / coldfront

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

[FOR-REVIEW] Add Journal title field to Publication for DOI (and pull from doi.org) #2

Closed dmr-x closed 4 years ago

dmr-x commented 4 years ago

This is practically done. 2 commits for your review if you'd like.

I changed more files than I had planned on in my estimate, but they seemed to be in scope and I'd rather do a more comprehensive job than let them slip by.

I have separated out a piece of it into an UNTESTED commit. It's a management command that requires a .tsv input. I could make one, but there isn't one in the repo. Lmk your thoughts on moving forward with anything thereon.

For the rest: I've done dev testing through the web UI. No unit testing. I'd still like to add that.

As for the implementation of publication/views.py, I want to refactor the redundant code there. I'll plan to do that unless you say otherwise. Just did a copy/paste/edit implementation for a quick win. snippet: https://github.com/arsmentis/coldfront/blob/a9b0ec722c661509f20a2cbc0b7e52b0a2c6fdc9/coldfront/core/publication/views.py#L116-L125

As for other implementation, I made some arbitrary choices for how to arrange the new journal field in the views. I think they're good choices, but we might want to check with users/stakeholders on this.

One further thing still untested: database migration

So to summarize things not done:

dmr-x commented 4 years ago

@artlogic I think this is ready for review (note the task list and what isn't done).

When we're ready, I would like to squash to [FIXUP] and UNTESTED commits into the earlier one, leaving 1 commit for upstream. Reworded, as needed.

artlogic commented 4 years ago

This is practically done. 2 commits for your review if you'd like.

I changed more files than I had planned on in my estimate, but they seemed to be in scope and I'd rather do a more comprehensive job than let them slip by.

I have separated out a piece of it into an UNTESTED commit. It's a management command that requires a .tsv input. I could make one, but there isn't one in the repo. Lmk your thoughts on moving forward with anything thereon.

I'm not concerned about testing these management commands. It seems a bit out of scope for our work. I'll let you know if I hear otherwise.

For the rest: I've done dev testing through the web UI. No unit testing. I'd still like to add that.

Let's add a couple of tests here, it looks like quick tests of the view and model are in order, but just to the functionality we added. I wouldn't worry about the templates as you've manually tested them and E2E testing isn't in scope.

As for the implementation of publication/views.py, I want to refactor the redundant code there. I'll plan to do that unless you say otherwise. Just did a copy/paste/edit implementation for a quick win. snippet:

https://github.com/arsmentis/coldfront/blob/a9b0ec722c661509f20a2cbc0b7e52b0a2c6fdc9/coldfront/core/publication/views.py#L116-L125

Can you please pull this into a separate PR?

When we're ready, I would like to squash to [FIXUP] and UNTESTED commits into the earlier one, leaving 1 commit for upstream. Reworded, as needed.

Yes, please squash into one commit once the tests are added and I've re-reviewed. Then we'll submit to upstream.

dmr-x commented 4 years ago

Let's add a couple of tests here, it looks like quick tests of the view and model are in order, but just to the functionality we added. I wouldn't worry about the templates as you've manually tested them and E2E testing isn't in scope.

Writing tests currently. The ForeignKey relationships in the models make it fairly tedious to test db save/retrieve behavior.

I can go ahead and do it for this case, but to my understanding, best practice is to use some sort of helper library, such as factory_boy. If I understand correctly, that makes these tests probably 10x faster to write.

@artlogic let me know what you'd like me to do here.

dmr-x commented 4 years ago

So, as a very kludgy way to get some test data into the db, I can do this in tests.py:

def setUpModule():
    call_command('initial_setup')
    call_command('load_test_data')

and e.g.

project = Project.objects.get(id=1)
source = PublicationSource.objects.get(id=1)

to get "dummy" objects to fulfill the ForeignKey fields

It works. But a test invocation takes around 13s... (most of that being setup/teardown).

If you'd rather not use something like factory_boy... ... as gross as it is, I'd drastically prefer doing something like this than to walk the ForeignKey dependencies and create everything necessary (Project depends on a lot)

I believe I could also export things into a Django test fixture, and trim things down. But that's also probably not a great long-term practice due to maintainability tedium.

dmr-x commented 4 years ago

Let's add a couple of tests here, it looks like quick tests of the view and model are in order, but just to the functionality we added. I wouldn't worry about the templates as you've manually tested them and E2E testing isn't in scope.

[...], best practice is to use some sort of helper library, such as factory_boy. [...] @artlogic let me know what you'd like me to do here.

Just recording in the PR comments that @artlogic mentioned we should go with factory_boy.

dmr-x commented 4 years ago

@artlogic: I've pushed 2 commits; would appreciate a review.

the first (https://github.com/arsmentis/coldfront/pull/2/commits/2da64fd8e57a44ba6af7b95336937c0926f2b87d) is straightforward - just including the new dependency. Of note, though: I think we should keep this commit separated in the upstream PR.

Let's add a couple of tests here, it looks like quick tests of the view and model are in order, [...]

model tests are submitted (https://github.com/arsmentis/coldfront/pull/2/commits/90a0d07ab8cf53fc165426e20c3a62bcbf1d2385) view tests will come in a subsequent commit, following similar style to the model tests

dmr-x commented 4 years ago

Let's add a couple of tests here, it looks like quick tests of the view and model are in order, [...]

model tests are submitted (90a0d07) view tests will come [later]

Updated the OP task list. It had gotten a bit stale.

dmr-x commented 4 years ago

I've now got an integration test done of the view method that contacts doi.org, but not anything on the view that adds the publication.

Code is a bit of a mess, will clean up a bit before pushing another day.

dmr-x commented 4 years ago

Just pushed the integration test; see 509d8252e7440566479941d0671d4691b8198b03. Since it makes a remote request, also added a test-helper decorator and applied it - see 480e9f79564deee51feb4e6c755f479c372b9d85 and 9a6902744058c92f0f82b1689d851efbafbfcd5f.

Also added a few fixups to things previously pushed - didn't want to force push because that would make review harder.

dmr-x commented 4 years ago

(More tests to come; just not fully written nor clean.)

artlogic commented 4 years ago

Also, please resolve conflicts.

dmr-x commented 4 years ago

There's a few [INTERIM] force-pushes in there, if you want to review my rebase/fixup efforts.

But the final upstream-PR-ready version, rebased onto upstream master, is now the wip/journal-from-doi branch HEAD. For reference (since github likes to bundle info together sometimes when it shouldn't), that's: dd5d457b2232f461ce46bd64224b1b35106029f1

Implementation in 1 commit, tests in more commits. All test commits are "combined" into a single --no-ff merge commit that has a useful summary to it.

@artlogic please let me know if you want anything else here.

dmr-x commented 4 years ago

Updated the OP task list.

dmr-x commented 4 years ago

@artlogic with ubccr/coldfront#204 now open, would you like to close this PR?

dmr-x commented 4 years ago

(task list) [snip]

  • adjustments to views based on user/stakeholder feedback

The "4 of 5 tasks done" was nagging me, and I realized that this TODO item doesn't really fit the github "PR + task list" workflow model. It would be more appropriate for higher-level organization - projects or milestones or meta issues or such.

Since that extra bookkeeping would be 99% hassle, 1% value - and since "hey, I want something changed" is kind of implicitly a potential TODO in any finished work - I'm just going to simplify: removing this item from all PRs' task lists.