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

Admin voting census original load (csv) #7591

Merged
merged 18 commits into from Apr 3, 2021
Merged

Conversation

agustibr
Copy link
Contributor

@agustibr agustibr commented Mar 11, 2021

🎩 What? Why?

This PR adds the Census section to the voting administration panel. Where an admin user can upload a .csv file that will create a Dataset record and creates a CreateDatumJob for each row/participant, some attributes are saved encrypted in the database and others the combination of attributes is hashed.

📌 Related Issues

Testing

  1. as an admin, go to admin > Votings > a voting in the space sidebar click on the Census entry.

  2. Once in the Census section, select a csv file and submit the form.
    Screenshot_Admin_census_1_create

  3. Once the file is submited, the admin is informed that the file is being processed:

Screenshot 2021-03-30 at 09 01 02

  1. After processing the file, shows the results of the csv data import:
  2. All rows imported:
    Screenshot_Admin_census_3_2_processed_ok
  3. Imported some rows:
    Screenshot_Admin_census_3_1_processed_warning
  4. Once the import is finished the admin has the ability to start again, deleting all the current records:
    Screenshot_Admin_census_4_delete

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

♥️ Thank you!

@agustibr agustibr added contract: e-voting Barcelona City Council contract module: elections labels Mar 11, 2021
@agustibr agustibr self-assigned this Mar 11, 2021
@agustibr agustibr force-pushed the feat/admin_voting_census branch 3 times, most recently from 9306bb5 to a2fc505 Compare March 15, 2021 16:58
@agustibr agustibr force-pushed the feat/admin_voting_census branch 3 times, most recently from d3d3eff to 75d6e51 Compare March 18, 2021 08:59
@agustibr agustibr marked this pull request as ready for review March 23, 2021 21:39
leio10
leio10 previously approved these changes Mar 24, 2021
Copy link
Contributor

@leio10 leio10 left a comment

Choose a reason for hiding this comment

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

Very good job!! 👏 👏

decidim-elections/lib/decidim/votings/test/factories.rb Outdated Show resolved Hide resolved
},
visibility: "admin-only"
)
end

def csv_rows
Copy link
Contributor

Choose a reason for hiding this comment

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

💬 I'm not sure about this, but maybe for very big files we need to do something like wc -l #{form.file.tempfile.path} to avoid loading all the lines in the server memory. I have tested it with files of almost 2GB and it takes less than 2 seconds. I don't know if this is too hacky or if there is any better way to this but it would be nice to consider it. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change it to what you suggest if it is more performant. 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you are okay we can work this and the increment counter out in another PR, so we don't block other PR's that depend on this one.

return broadcast(:invalid) unless dataset

# rubocop:disable Rails/SkipsModelValidations
Dataset.increment_counter(:csv_row_processed_count, dataset.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ I have two thoughts related to this:

  • Even when I think this method is better, this could be faster if you do it directly in the CreateDatum command. I don't know if running a new command for each csv row could be a performance issue.
  • Is this operation atomic? If we have several jobs running the same operation we could have race conditions problems. I saw that this is run in the after_perform part of the job, but I don't know exactly how it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First this was in the CreateDatum command but as it updates the Dataset model extracting it here seemed more appropriate.
In the future this command could be used to send an email when all_rows_processed?.

On the other hand, I used the class method Dataset.increment_counter instead of the instance method dataset.increment for that reason according this post.

orlera
orlera previously approved these changes Mar 25, 2021
Copy link
Contributor

@orlera orlera left a comment

Choose a reason for hiding this comment

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

Great job! 👏🏽

beagleknight
beagleknight previously approved these changes Mar 25, 2021
Copy link
Contributor

@beagleknight beagleknight left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Nothing much to add 😄

Comment on lines +30 to +39
attributes = {
hashed_in_person_data: form.hashed_in_person_data,
hashed_check_data: form.hashed_check_data,

full_name: form.full_name,
full_address: form.full_address,
postal_code: form.postal_code,
mobile_phone_number: form.mobile_phone_number,
email: form.email
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this but I think you can just call form.attributes and it will return the same hash that you are building 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, is good to know this, but in this case the form has more attributes than the model because they are used to generate the hashed_*_data attributes and those are not persisted.

@oriolgual oriolgual merged commit 8bcad5c into develop Apr 3, 2021
@oriolgual oriolgual deleted the feat/admin_voting_census branch April 3, 2021 09:29
entantoencuanto added a commit that referenced this pull request Apr 5, 2021
* develop:
  fix: remove vote.js from elections manifest (#7795)
  Evote election log (#7757)
  Evote - identify with access code (#7740)
  Add unique trustee name (#7544)
  New translations en.yml (Finnish (plain)) (#7790)
  New Crowdin updates (#7754)
  Remove proposals dependency from the debates module (#7783)
  Make meetings functional without the proposals module (#7784)
  Make accountability functional without the proposals module (#7785)
  Make budgets functional without the proposals module (#7786)
  Remove BOM in CSV of private participants (#7781)
  Move searchlight gem to core and remove unnecessary requires (#7782)
  Admin voting census original load (csv) (#7591)
  Feat/evote count votes (#7769)
  Move votings answer results to its own table (#7767)
  Fix showing trustees menu on consultations module (#7778)
  Add missing assets to the elections manifest (#7773)
@leio10 leio10 added the type: feature PRs or issues that implement a new feature label Jul 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contract: e-voting Barcelona City Council contract module: elections type: feature PRs or issues that implement a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the participants original load
5 participants