Skip to content
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

Add AO status to legal AO endpoint #2738

Merged
merged 9 commits into from
Nov 8, 2017

Conversation

lbeaufort
Copy link
Member

@lbeaufort lbeaufort commented Nov 6, 2017

Along with https://github.com/18F/fec-cms/pull/1456, this resolves https://github.com/18F/fec-cms/issues/1390

Originally intended to replace is_pending with status but that required a lot of front-end refactoring. Instead, I'm adding status.

-Verified that ao.stage data maps to AO status (0 = pending, 2 = withdrawn, 1 = final)
-add status display field and ao_status search parameter with values "Pending," "Withdrawn," and "Final."
-Slightly refactor SQL query - left join no longer needed, now need inner join to aouser.ao to pull in stage
-Verified searches still work in API where ao_category=W = this is based off document categories
-Tested AO search with status parameter - should be able to search for either
-Hard-coded withdrawn AO 2009-05 that has improperly been marked Final in internal database

@codecov-io
Copy link

codecov-io commented Nov 7, 2017

Codecov Report

Merging #2738 into release/public-20171109 will decrease coverage by 0.02%.
The diff coverage is 76.92%.

Impacted file tree graph

@@                     Coverage Diff                     @@
##           release/public-20171109    #2738      +/-   ##
===========================================================
- Coverage                    89.35%   89.32%   -0.03%     
===========================================================
  Files                           68       68              
  Lines                         5533     5546      +13     
===========================================================
+ Hits                          4944     4954      +10     
- Misses                         589      592       +3
Impacted Files Coverage Δ
webservices/args.py 97.58% <ø> (ø) ⬆️
webservices/legal_docs/index_management.py 39.62% <ø> (ø) ⬆️
webservices/resources/legal.py 75.43% <50%> (-0.31%) ⬇️
webservices/legal_docs/advisory_opinions.py 92.46% <81.81%> (-0.87%) ⬇️

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 63313d0...89b6b9c. Read the comment docs.

@lbeaufort lbeaufort changed the title [WIP] Replacing AO is_Pending field with AO status Replace AO is_Pending field with AO status Nov 7, 2017
@lbeaufort lbeaufort requested a review from vrajmohan November 7, 2017 14:46
@lbeaufort lbeaufort changed the title Replace AO is_Pending field with AO status Add AO status to legal AO endpoint Nov 7, 2017
@lbeaufort lbeaufort changed the title Add AO status to legal AO endpoint [WIP] Add AO status to legal AO endpoint Nov 7, 2017
@lbeaufort lbeaufort requested a review from ccostino November 7, 2017 19:37
@lbeaufort lbeaufort changed the title [WIP] Add AO status to legal AO endpoint Add AO status to legal AO endpoint Nov 7, 2017
ao_parsed.summary,
ao_parsed.req_date,
ao_parsed.issue_date,
CASE ao.stage WHEN 0 THEN TRUE ELSE FALSE END AS is_pending,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to just retrieve ao.stage in the SQL query and do the ao_status and ao_is_pending evaluations in python?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, the logic would be simpler to understand in Python, and we could also keep this evaluation and the function ao_status_to_stage in the same Python file, so that they could be edited together (if there were future changes to be made, or future special cases to be handled like 2009-05).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vrajmohan works for me! Will do.

@vrajmohan
Copy link
Contributor

I made just one comment - it is a suggestion and I'll be happy to merge without it. Everything else looks great! Nice work, @lbeaufort!

ao_parsed.summary,
ao_parsed.req_date,
ao_parsed.issue_date,
CASE ao.stage WHEN 2 THEN 'Withdrawn'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vrajmohan I removed the is_pending logic here, added the function on line 85, and added the is_pending logic on line 117.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great! Sorry, I wasn't clear but CASE ao.stage WHEN 2 THEN 'Withdrawn' --Hard-code one withdrawn AO that has improperly been marked Final WHEN 1 THEN CASE ao.ao_no WHEN '2009-05' THEN 'Withdrawn' ELSE 'Final' END ELSE 'Pending' END AS stage would also be more clear in python.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To elaborate, the function would be:

def ao_stage_to_status(ao_no, stage):
   AOS_WITH_INCORRECT_STAGE = ['2009-05']
   if stage == 2:
       return "Withdrawn"
   elif stage == 1:
       if ao_no in AOS_WITH_INCORRECT_STAGE:
           return "Withdrawn:
       else:
           return "Final"
  else:
      return "Pending"

This would make it easier to add more special cases.

else:
is_pending = False

return is_pending
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In idiomatic python, the entire body of this function would be:
return stage == 'Pending'

@lbeaufort
Copy link
Member Author

@vrajmohan thanks for your suggestions! I tweaked it slightly to make AOS_WITH_CORRECTED_STAGE a dictionary lookup to make it easier to modify other AOs in the future.

@vrajmohan vrajmohan merged commit 197255b into release/public-20171109 Nov 8, 2017
@vrajmohan vrajmohan deleted the feature/add-ao-status-2 branch November 8, 2017 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants