feature/role-title-constants #155

dphoria · 2022-01-26T00:15:23Z

Link to Relevant Issue

This pull request is a prerequisite for resolving CouncilDataProject/cdp-scrapers#66

Description of Changes

Include a description of the proposed changes.

Added RoleTitle string constants to cdp_backend/database/constants.py. This will be useful for cdp_scrapers when applying logic to ensure that a Person has only one Role with title = RoleTitle.COUNCILMEMBER per term.

I took RoleTitle members verbatim from Jackson's suggestions in this comment. Certainly open to additions, modifications, etc.

For reference, below is the entire unique list of OfficeRecordTitle from all Legistar API OfficeRecords in Seattle.

Vice Chair
Member
Chair
Councilmember
City Clerk
None
Alternate
Mayor
Council President

(You gotta love None 🤣 )

For OfficeRecordMemberType,

Chair
Vice Chair
Member

codecov · 2022-01-26T00:19:59Z

Codecov Report

Merging #155 (f5bf9e9) into main (4f8b6c7) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #155      +/-   ##
==========================================
+ Coverage   94.53%   94.57%   +0.03%     
==========================================
  Files          50       50              
  Lines        2543     2560      +17     
==========================================
+ Hits         2404     2421      +17     
  Misses        139      139

Impacted Files	Coverage Δ
cdp_backend/pipeline/mock_get_events.py	`100.00% <ø> (ø)`
cdp_backend/database/constants.py	`100.00% <100.00%> (ø)`
cdp_backend/database/models.py	`100.00% <100.00%> (ø)`
cdp_backend/tests/database/test_validators.py	`100.00% <100.00%> (ø)`
...ckend/tests/pipeline/test_event_gather_pipeline.py	`100.00% <0.00%> (ø)`
cdp_backend/database/functions.py	`91.21% <0.00%> (+0.04%)`	⬆️
cdp_backend/pipeline/event_gather_pipeline.py	`85.75% <0.00%> (+0.07%)`	⬆️

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 4f8b6c7...f5bf9e9. Read the comment docs.

cdp_backend/database/constants.py

tohuynh · 2022-01-26T00:36:36Z

Also, don't know if you want to add it to this PR, but a validator needs to be added to https://github.com/CouncilDataProject/cdp-backend/blob/main/cdp_backend/database/models.py#L190.

See an example of a constant validator: https://github.com/CouncilDataProject/cdp-backend/blob/main/cdp_backend/database/models.py#L612

dphoria · 2022-01-26T00:38:03Z

Also, don't know if you want to add it to this PR, but a validator needs to be added to https://github.com/CouncilDataProject/cdp-backend/blob/main/cdp_backend/database/models.py#L190.

See an example of a constant validator: https://github.com/CouncilDataProject/cdp-backend/blob/main/cdp_backend/database/models.py#L612

Ohhh thank you so much. I'm switching this PR to draft for now then. 😅

evamaxfield

Looks good to me! Thanks! Will let one of the front end folks approve too before merge

isaacna

Looks good to me!

tohuynh

Thanks for adding the validator. Looks good.

I have one clarifying question.

tohuynh · 2022-02-07T17:59:41Z

cdp_backend/database/models.py

+    title = fields.TextField(
+        validator=validators.create_constant_value_validator(RoleTitle, True)
+    )


I notice that the other required constant field like matterstatus.status have a required=True, while this one doesn't. It doesn't matter that it is missing right, as long the second arg to create_constant_value_validator is True?

cc @isaacna @JacksonMaxfield

Oops that's my mistake. Thank you for catching that!

Yea this is just a styling thing. The second arg of True will be used for the required kwarg. Can include or not.

The only reason why I commonly include the full kwarg name is because if we ever change the API, and we shift the order of the arguments, the kwargs still get filled properly regardless of the order. But I don't think we plan on changing the arg order haha. Either way, I'm not concerned.

I should still probably fix it to set required for fields.TextField(), i.e.

title = fields.TextField( required=True, # this one validator=validators.create_constant_value_validator(RoleTitle, True), )

I had the same thought earlier but also agree that keeping the arg name is not a big deal. I think we can add them for Jackson's reasons above and also readability. Earlier I was gonna comment why we changed title to not be required, but then I saw that True was for is_required. If we add the arg name it becomes more clear

Oh woops. Yes, you should set it for the TextField I believe.

I think if we set the FireO arg required, than we can just get rid of the is_required field in create_constant_value_validator. Same goes for everywhere else since I don't think it really adds any functionality

Isaac's suggestion makes sense, we only need one way to check if something is required.

I'll change as above, but I guess this is food for thought for the future then.

Agree. Change to match others for now. Can you open an issue to track the above thoughts as well?

dphoria added 2 commits January 25, 2022 15:44

RoleTitle

8bd8328

pythonic constants convention

c41092d

dphoria requested review from evamaxfield, isaacna and tohuynh January 26, 2022 00:15

dphoria added the enhancement New feature or request label Jan 26, 2022

dphoria requested a review from BrianL3 January 26, 2022 00:16

dphoria added the low priority Doesn't need immediate attention label Jan 26, 2022

tohuynh reviewed Jan 26, 2022

View reviewed changes

cdp_backend/database/constants.py Outdated Show resolved Hide resolved

dphoria marked this pull request as draft January 26, 2022 00:38

dphoria added 4 commits February 6, 2022 15:04

validator pytest

2b31f42

RoleTitle constants in models.py

94ecf59

linting

ad86d24

use RoleTitle

ecf0bac

dphoria marked this pull request as ready for review February 7, 2022 00:42

evamaxfield approved these changes Feb 7, 2022

View reviewed changes

isaacna approved these changes Feb 7, 2022

View reviewed changes

tohuynh approved these changes Feb 7, 2022

View reviewed changes

Role.title is required

f5bf9e9

evamaxfield merged commit 8743498 into CouncilDataProject:main Feb 8, 2022

isaacna mentioned this pull request Feb 8, 2022

Remove is_required from create_constant_value_validator #160

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/role-title-constants #155

feature/role-title-constants #155

dphoria commented Jan 26, 2022

codecov bot commented Jan 26, 2022 •

edited

tohuynh commented Jan 26, 2022

dphoria commented Jan 26, 2022

evamaxfield left a comment

isaacna left a comment

tohuynh left a comment

tohuynh Feb 7, 2022

dphoria Feb 7, 2022

evamaxfield Feb 7, 2022

dphoria Feb 7, 2022 •

edited

isaacna Feb 7, 2022

evamaxfield Feb 7, 2022

isaacna Feb 7, 2022

tohuynh Feb 7, 2022

dphoria Feb 7, 2022

evamaxfield Feb 7, 2022

feature/role-title-constants #155

feature/role-title-constants #155

Conversation

dphoria commented Jan 26, 2022

Link to Relevant Issue

Description of Changes

codecov bot commented Jan 26, 2022 • edited

Codecov Report

tohuynh commented Jan 26, 2022

dphoria commented Jan 26, 2022

evamaxfield left a comment

Choose a reason for hiding this comment

isaacna left a comment

Choose a reason for hiding this comment

tohuynh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dphoria Feb 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 26, 2022 •

edited

dphoria Feb 7, 2022 •

edited