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

RDISCROWD-7364 custom task export fields #988

Merged
merged 8 commits into from
Oct 31, 2024
Merged

RDISCROWD-7364 custom task export fields #988

merged 8 commits into from
Oct 31, 2024

Conversation

n00rsy
Copy link

@n00rsy n00rsy commented Oct 29, 2024

Issue number of the reported bug or feature request: RDISCROWD-7364

Describe your changes

  • Update task csv export: when display_info_columns is in the request body, filter the task__info fields to only contain fields in display_info_columns.

Testing performed
tested locally

@coveralls
Copy link

coveralls commented Oct 29, 2024

Pull Request Test Coverage Report for Build 11600690507

Details

  • 15 of 16 (93.75%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.001%) to 94.015%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pybossa/exporter/task_csv_export.py 2 3 66.67%
Totals Coverage Status
Change from base Build 11559628491: -0.001%
Covered Lines: 17374
Relevant Lines: 18480

💛 - Coveralls

@n00rsy n00rsy requested a review from dchhabda October 30, 2024 15:17
Comment on lines 219 to 230
new_headers = []
accepted_task_info_fields_set = set(accepted_task_info_fields)
for header in headers:
if header == 'task__info':
pass
elif header.startswith('task__info__'):
masked_header = header[12: ]
if masked_header in accepted_task_info_fields_set:
new_headers.append(header)
else:
new_headers.append(header)
return new_headers

Choose a reason for hiding this comment

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

cosmetic; indentation to be corrected here

Comment on lines 222 to 225
if header == 'task__info':
pass
elif header.startswith('task__info__'):
masked_header = header[12: ]

Choose a reason for hiding this comment

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

For code clarity, we can add a variable for number here

Comment on lines 218 to 230
def filter_table_headers(headers, accepted_task_info_fields):
new_headers = []
accepted_task_info_fields_set = set(accepted_task_info_fields)
for header in headers:
if header == 'task__info':
pass
elif header.startswith('task__info__'):
masked_header = header[12: ]
if masked_header in accepted_task_info_fields_set:
new_headers.append(header)
else:
new_headers.append(header)
return new_headers

Choose a reason for hiding this comment

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

See if following changes makes sense considering comments mentioned above

def filter_table_headers(headers, filtered_columns):
  accepted_task_info_fields = set(filtered_columns) if filtered_columns else set()
  task_info_header = "task__info__"
  task_info_header_len = len(task_info_header)

  for header in headers:
    if header != 'task__info':
      if header.startswith(task_info_header):
        masked_header = header[task_info_header_len:]  
        if masked_header in accepted_task_info_fields:  
            new_headers.append(header)  
      else:  
        new_headers.append(header) 

Comment on lines 222 to 223
if header == 'task__info':
pass

Choose a reason for hiding this comment

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

pass can be avoided w/ flipping the condition

Copy link
Author

Choose a reason for hiding this comment

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

if we switch to

if header != 'task__info':
 ...
else:  
     new_headers.append(header) 

task__info will always get added to new_headers.

Copy link
Author

Choose a reason for hiding this comment

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

For clarity, there are 3 cases for each header:

  1. header = 'task__info', so we skip it
  2. header is like 'task__info__[fieldname], so we check fieldname against accepted fields set and add accordingly
  3. header is something else, so we add it to the new_headers list (task__id, task__state, etc)

Choose a reason for hiding this comment

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

Likely due to small tab space, it was not clear that else part of if header != 'task__info': won't be adding new_header always.

@n00rsy n00rsy changed the title RDISCROWD-7364 RDISCROWD-7364 custom task export fields Oct 30, 2024
Copy link

@dchhabda dchhabda left a comment

Choose a reason for hiding this comment

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

Upon making this changes, PR can be merged. Thanks!

@n00rsy n00rsy merged commit 0663318 into main Oct 31, 2024
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants