New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update labor category filter in CALC tool to search for SIN prefix #612

Closed
mtorres253 opened this Issue Aug 25, 2016 · 21 comments

Comments

Projects
None yet
5 participants
@mtorres253

mtorres253 commented Aug 25, 2016

As a user of CALC, I want to look up labor categories by SIN prefix (not the entire SIN number) rather than the old schedule name, so that I can be confident that I am getting the current prices (since the recent data upload migrated the old schedules into one professional services schedule with SIN prefix replacing of the current and former schedule names).

For example, If I want to see all labor categories in FABS (former schedule name), then I should be able to filter all labor categories with SIN prefix of 520. I should also be able to do this for SIN prefix 132/IT 70 (current schedule name)

Acceptance Criteria

  • Scenario: Filters are adjusted so they show former (or current) schedule name, along with SIN prefix
  • Given I am a visitor to CALC (no authentication required)
  • When I am on the CALC homepage (https://calc.gsa.gov/)
  • Then I should see the following values for the schedule filter (pattern [SIN prefix] - [current or former schedule name])

520 - Formerly FABS
541 - Formerly AIMS
73802 - Formerly Language
871 - Formerly PES
874 - Formerly Mobis
87405 - Formerly Logistics
899 - Formerly Environmental
132 - IT 70

https://docs.google.com/a/gsa.gov/spreadsheets/d/1FgqTf-z5esSleI4vfUYBi53VIXL1Tg3uglG2rVccw54/edit?usp=sharing

  • Scenario: Filtering by SIN prefix/schedule name picks up all labor categories with that SIN prefix
  • Given I am a visitor to CALC (https://calc.gsa.gov/)
  • When I select [SIN prefix] - [current or former schedule name] from the schedule filter
  • Then my view should be filtered so that I see data only for vendors that have the selected SIN prefix
  • Scenario: Help text to explain SIN prefix/schedule name migration
  • When I visit https://calc.gsa.gov/about/#schedules
  • Then for each schedule listed except for schedule 70, I should see a sub-header containing the current SIN prefix for each of the schedule followed by “(formally known as [former schedule name])”
  • And for Schedule 70, I should see a sub-header containing the SIN prefix followed by “(Schedule 70)”
  • Scenario: Update API to return the correct information when filtering by SIN prefix
  • Given I am an authorized user of the CALC API
  • When I make a request for schedule data using a valid SIN prefix
  • Then the response should include data for all vendors in the schedule with that SIN prefix
  • Scenario: Update API documentation
  • When I view documentation (https://github.com/18F/calc#other-filters)
  • Then I should see that I can request schedule information using the SIN prefix

General Acceptance Criteria

@dsummersl

This comment has been minimized.

Contributor

dsummersl commented Sep 7, 2016

I see that for some database records have SIN/schedule that don't match the expected combination. This seems to be for contracts under the 'Consolidated' schedule:

calc=# select sin,schedule from contracts_contract where sin like '%541%' limit 10;
       sin        |   schedule
------------------+--------------
 541-4B           | AIMS
 541-4B           | Consolidated
 541 3            | AIMS
 541-4B           | Consolidated
 541-4B           | Consolidated
 541-4B           | AIMS
 541 3            | AIMS
 541-4B           | Consolidated
 541-4B, 541-4BRC | AIMS
 541-4B, 541-4BRC | AIMS
(10 rows)

Should the SIN/schedule query show records that match both columns (ie, if you select '871 - Formerly PES' it would return the results for sin like '%871%' or schedule='PES')?

@dsummersl

This comment has been minimized.

Contributor

dsummersl commented Sep 7, 2016

I'm going to guess that I should just match for the sin prefix, unless I hear otherwise.

@mtorres253

This comment has been minimized.

mtorres253 commented Sep 8, 2016

Hi @dsummersl! @KellyBailey would you answer this question for us? This seems like a product decision. Also @dsummersl it might help for you to file your pull request (even if it's incomplete) so we can better help out.

@KellyBailey

This comment has been minimized.

Collaborator

KellyBailey commented Sep 8, 2016

@dsummersl For all references to 541 or AIMS show Formerly AIMs. If it is an easy fix, can we use the term "Legacy" instead of "Formerly"? Disregard if it is too late at this point. I just noticed the use of the term Legacy on this page- http://www.gsa.gov/portal/content/246403, when researching the answer to your question. Thanks, Kelly.

@dsummersl

This comment has been minimized.

Contributor

dsummersl commented Sep 8, 2016

@KellyBailey I can change it to Legacy, no problem. That is 'Legacy AIMS' or something more like 'AIMS (Legacy)'?

@dsummersl

This comment has been minimized.

Contributor

dsummersl commented Sep 8, 2016

@mtorres253 and @KellyBailey I've pushed my initial PR. I think I still need to go back and fix the 'Legacy vs Formerly' once I know the format you would like.

@mtorres253

This comment has been minimized.

mtorres253 commented Sep 8, 2016

Thanks @dsummersl!

@KellyBailey

This comment has been minimized.

Collaborator

KellyBailey commented Sep 8, 2016

@dsummersl Can we do this:
541 (Legacy AIMS)
871 (Legacy PES)
etc.

@dsummersl

This comment has been minimized.

Contributor

dsummersl commented Sep 8, 2016

@KellyBailey @mtorres253 I've made those two changes. I notice that my PR shows a three broken test cases...but I notice the three cases are all broken on the develop branch too when I test locally - do I need to fix these or are they some ongoing 'bulk test' related issue I can ignore? Went ahead and added a fix for one of them...but the two bulk tests are still broken.

I did a git bisect on the test_old_contracts_are_deleted test and got to here:

git bisect start
git bisect bad
git bisect good c9f879d
...
...
...
calc › git bisect bad
Bisecting: 1 revision left to test after this (roughly 1 step)
[f8627056c3b697278ff70b7f1dc4c80c752738fb] Oops, remove debugging code.
calc › git bisect bad
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[cdde3b5acbafee463d3571a510a287217a3425ab] Process bulk uploads in a separate thread.
calc › git bisect bad
cdde3b5acbafee463d3571a510a287217a3425ab is the first bad commit
commit cdde3b5acbafee463d3571a510a287217a3425ab
Author: Atul Varma <varmaa@gmail.com>
Date:   Thu Aug 25 09:04:07 2016 -0400

    Process bulk uploads in a separate thread.

:040000 040000 b359a7245ef040bbced1f42e93c51f6f528b7432 16e6ddaca4e44af8f1953d6b29fa4c435dfc2383 M      data_capture
calc ›
@mtorres253

This comment has been minimized.

mtorres253 commented Sep 8, 2016

@toolness @KellyBailey The acceptance criteria for this story is that all tests pass but these seem to be broken on the development branch. Are you aware of that and is the team already working on that?

@jseppi

This comment has been minimized.

Contributor

jseppi commented Sep 8, 2016

I just ran tests from the develop branch via docker-compose run app python manage.py ultratest and all tests are passing. They also appear to all be passing in the latest travis build of develop: https://travis-ci.org/18F/calc/builds/158492497

@dsummersl, how exactly are you running tests? We don't want develop to ever have broken tests, so I'd definitely like to figure out what's wrong. Sorry for the trouble!

screen shot 2016-09-08 at 3 03 16 pm

@dsummersl

This comment has been minimized.

Contributor

dsummersl commented Sep 8, 2016

Hi @jseppi I've been using docker-compose, and running in the 'app' container. I just pulled from upstream and reran the tests:

calc_user@a4f642e6e89a:/calc$ python manage.py ultratest
Connection to database established.
  _    _ _ _          _______        _
 | |  | | | |        |__   __|      | |
 | |  | | | |_ _ __ __ _| | ___  ___| |_
 | |  | | | __| '__/ _` | |/ _ \/ __| __|
 | |__| | | |_| | | (_| | |  __/\__ \ |_
  \____/|_|\__|_|  \__,_|_|\___||___/\__|

Running ALL THE TESTS
-> flake8 OK
-> eslint OK
-> bandit OK
-> py.test FAIL
------------------------------------------------------------------------------
Output from py.test:

============================= test session starts ==============================
platform linux -- Python 3.4.5, pytest-2.9.2, py-1.4.31, pluggy-0.3.1
django settings: hourglass.settings (from environment variable)
rootdir: /calc, inifile: pytest.ini
plugins: django-2.9.1, cov-2.3.1
collected 252 items

api/tests/test_api.py ........................................
api/tests/test_utils.py ......
contracts/tests/test_contract.py ........
contracts/tests/test_load_s70.py .............
data_capture/tests/test_admin.py ............
data_capture/tests/test_apps.py E
data_capture/tests/test_bulk_upload_views.py ................FF.
data_capture/tests/test_decorators.py ..........
data_capture/tests/test_email.py .......
data_capture/tests/test_forms.py ........
data_capture/tests/test_jobs.py ..
data_capture/tests/test_management_commands.py .
data_capture/tests/test_models.py ......
data_capture/tests/test_price_list_views.py ....................
data_capture/tests/test_r10_spreadsheet_converter.py ......
data_capture/tests/test_s70.py ..............
data_capture/tests/test_schedules_registry.py .........
frontend/tests/test_date.py ...........
frontend/tests/test_qunit.py .
frontend/tests/test_selenium.py ...............
hourglass/tests.py ...................
styleguide/tests.py ...
uaa_client/tests/test_authentication.py ...........
uaa_client/tests/test_views.py ..........

----------- coverage: platform linux, python 3.4.5-final-0 -----------
Coverage XML written to file coverage.xml


==================================== ERRORS ====================================
 ERROR at setup of DataCaptureSchedulerAppTests.test_data_capture_scheduler_app_adds_cron_job

request = <SubRequest '_django_setup_unittest' for <TestCaseFunction 'test_data_capture_scheduler_app_adds_cron_job'>>
_django_cursor_wrapper = <pytest_django.plugin.CursorManager object at 0x7fe11f16cb00>

    @pytest.fixture(autouse=True, scope='class')
    def _django_setup_unittest(request, _django_cursor_wrapper):
        """Setup a django unittest, internal to pytest-django."""
        if django_settings_is_configured() and is_django_unittest(request):
            request.getfuncargvalue('_django_test_environment')
            request.getfuncargvalue('_django_db_setup')

            _django_cursor_wrapper.enable()

            cls = request.node.cls

            _restore_class_methods(cls)
>           cls.setUpClass()

/usr/local/lib/python3.4/site-packages/pytest_django/plugin.py:386:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/usr/local/lib/python3.4/site-packages/django/test/testcases.py:945: in setUpClass
    super(TestCase, cls).setUpClass()
/usr/local/lib/python3.4/site-packages/django/test/testcases.py:155: in setUpClass
    cls._cls_overridden_context.enable()
/usr/local/lib/python3.4/site-packages/django/test/utils.py:212: in enable
    apps.set_installed_apps(self.options['INSTALLED_APPS'])
/usr/local/lib/python3.4/site-packages/django/apps/registry.py:324: in set_installed_apps
    self.populate(installed)
/usr/local/lib/python3.4/site-packages/django/apps/registry.py:115: in populate
    app_config.ready()
data_capture/apps.py:40: in ready
    scheduler = self.get_scheduler()
data_capture/apps.py:32: in get_scheduler
    return django_rq.get_scheduler(cls.rq_queue_name)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _                                                                      [45/1837]

args = ('default',), kwargs = {}

    def get_scheduler(*args, **kwargs):
>       raise ImproperlyConfigured('rq_scheduler not installed')
E       django.core.exceptions.ImproperlyConfigured: rq_scheduler not installed

/usr/local/lib/python3.4/site-packages/django_rq/queues.py:243: ImproperlyConfigured
=================================== FAILURES ===================================
___________ Region10UploadStep3Tests.test_old_contracts_are_deleted ____________

self = <data_capture.tests.test_bulk_upload_views.Region10UploadStep3Tests testMethod=test_old_contracts_are_deleted>

    def test_old_contracts_are_deleted(self):
        user = self.login(is_staff=True)
        other_source = BulkUploadContractSource.objects.create(
            procurement_center=BulkUploadContractSource.REGION_10
        )
        Contract.objects.create(
            min_years_experience=1,
            hourly_rate_year1=20.20,
            idv_piid='abc',
            vendor_name='blah',
            labor_category='whatever',
            upload_source=other_source
        )
        self.setup_upload_source(user)
        res = self.client.post(self.url)
        self.assertEqual(res.status_code, 200)

        process_worker_jobs()

        contracts = Contract.objects.all()
>       self.assertEqual(len(contracts), 3)
E       AssertionError: 0 != 3

data_capture/tests/test_bulk_upload_views.py:186: AssertionError
----------------------------- Captured stderr call -----------------------------
[08/Sep/2016 20:15:48] INFO [rq.worker:424] RQ worker 'rq:worker:a4f642e6e89a.225' started, version 0.6.0
[08/Sep/2016 20:15:48] INFO [rq.worker:690] Cleaning registries for queue: default
[08/Sep/2016 20:15:48] INFO [rq.worker:466]
[08/Sep/2016 20:15:48] INFO [rq.worker:467] *** Listening on default...
[08/Sep/2016 20:15:48] INFO [rq.worker:444] RQ worker 'rq:worker:a4f642e6e89a.225' done, quitting
_ Region10UploadStep3Tests.test_post_is_ok_and_contracts_are_created_properly __

self = <data_capture.tests.test_bulk_upload_views.Region10UploadStep3Tests testMethod=test_post_is_ok_and_contracts_are_created_properly>

    def test_post_is_ok_and_contracts_are_created_properly(self):
        user = self.login(is_staff=True)
        self.setup_upload_source(user)
        res = self.client.post(self.url)
        self.assertEqual(res.status_code, 200)
        self.assertIn('SEND_TRANSACTIONAL_EMAILS', res.context)

        process_worker_jobs()

        contracts = Contract.objects.all()
>       self.assertEqual(len(contracts), 3)
E       AssertionError: 0 != 3

data_capture/tests/test_bulk_upload_views.py:159: AssertionError
----------------------------- Captured stderr call -----------------------------
[08/Sep/2016 20:15:48] INFO [rq.worker:424] RQ worker 'rq:worker:a4f642e6e89a.225' started, version 0.6.0
[08/Sep/2016 20:15:48] INFO [rq.worker:690] Cleaning registries for queue: default
[08/Sep/2016 20:15:48] INFO [rq.worker:466]
[08/Sep/2016 20:15:48] INFO [rq.worker:467] *** Listening on default...
[08/Sep/2016 20:15:48] INFO [rq.worker:444] RQ worker 'rq:worker:a4f642e6e89a.225' done, quitting
================ 2 failed, 249 passed, 1 error in 92.32 seconds ================


Failing tests: py.test
Coverage: 86.05%
@dsummersl

This comment has been minimized.

Contributor

dsummersl commented Sep 8, 2016

Maybe it's a configuration issue with the docker containers?

@mtorres253

This comment has been minimized.

mtorres253 commented Sep 8, 2016

Thanks for stepping in @jseppi

@jseppi

This comment has been minimized.

Contributor

jseppi commented Sep 8, 2016

Hrm, in that log you pasted I see

E       django.core.exceptions.ImproperlyConfigured: rq_scheduler not installed

Have you done a fresh docker-compose build? It looks like you just might be missing a package that was added to the requirements.txt sometime between when you originally built your docker containers and when you recently pulled.

@mtorres253

This comment has been minimized.

mtorres253 commented Sep 8, 2016

@dsummersl can you try again given @jseppi's comments

@dsummersl

This comment has been minimized.

Contributor

dsummersl commented Sep 8, 2016

I'll rebase my PR and rebuild my dockers and see if that helps, thanks all!

@jseppi

This comment has been minimized.

Contributor

jseppi commented Sep 8, 2016

@dsummersl another thing you might run into is that some tests will fail if you run docker-compose run app python manage.py ultratest while you also have a running docker-compose up in another terminal.

I'm not exactly sure why this is right now, but I think it is something to do with how the tests run the queued jobs synchronously while the app is running them normally/asynchronously.

@mtorres253

This comment has been minimized.

mtorres253 commented Sep 8, 2016

@dsummersl looks like the tests are passing now. When you've completed all the work detailed in the scenarios, let us know by going here https://micropurchase.18f.gov/auctions/29 and clicking the "I'm done" button.

@mtorres253

This comment has been minimized.

mtorres253 commented Sep 9, 2016

Hi @jseppi can you take a look and let me know if #696 is OK to merge?

@toolness

This comment has been minimized.

Contributor

toolness commented Sep 16, 2016

Woot, fixed by #696!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment