Skip to content

Conversation

@kotrynav
Copy link
Member

#734 This issue prevented errors when headers in CSV files were present. Also added HTML error messages on failed uploads to specify which rows could not be matched to existing schools or committees.

Copy link
Contributor

@mathildepm mathildepm left a comment

Choose a reason for hiding this comment

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

LGTM!
Idk why some tests are running so we'll see when you push again.
Let's revert committee.py to what it was before since we aren't worried about that:
https://dev.to/lofiandcode/git-and-github-how-to-revert-a-single-file-dha

failed_uploads.append(row)
if failed_uploads:
messages.error(
request, 'Not all secretariat members could upload. These rows failed: '
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
request, 'Not all secretariat members could upload. These rows failed: '
request, 'Not all Committees could upload. These rows failed: '

Copy link
Contributor

Choose a reason for hiding this comment

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

JK let's just revert this file

@mathildepm
Copy link
Contributor

ALSO; let's update the PR description so it describes more accurately what the PR is doing

  • Reduces errors due to CSV headers
  • Prevents entire failures of uploads if a single row fails - instead tracks failed rows.

@mathildepm
Copy link
Contributor

We were looking through this code for huxley notes app and realized that we should also be validating emails in load so that should be a todo.

@srisainachuri
Copy link
Contributor

srisainachuri commented Nov 28, 2021

Here's a reference to the commit in huxley notes app in delegate.py's load method for the email check:
9d9859f

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.

4 participants