support for CSV and multiple columns #2
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A `bundle install` complaints with: ``` Fetching gem metadata from https://rubygems.org/........ Fetching gem metadata from https://rubygems.org/. Resolving dependencies.... Your bundle is locked to url (0.3.2), but that version could not be found in any of the sources listed in your Gemfile. If you haven't changed sources, that means the author of url (0.3.2) has removed it. You'll need to update your bundle to a version other than url (0.3.2) that hasn't been removed in order to install. ``` It looks like that the gem that depends on this one had the same issue and drop it.
And move Parser's specs to a new class-specific file.
No reason to use `#send` if the method is public.
This adds a new parser and while making it backward compatible. It'll only be enabled if you configure with `processor = :metadata`. It does so by extracting specific entry parsing into two classes. Now we have them both encapsulated and we can extend the metadata one however we want without affecting current behaviour.
Well, a bit rudimentary still, but also normalize a bit the entry format. Less flexible => less complexity => less buggy.
This turns the entry parsers into full text parsers each with different features, not only at the entry level. While we keep the simple parser that deals only with names for backward compatibility, the new metadata parser deals with CSV and unlimited columns, while being more strict in its input format.
So they better convey the use of the template method pattern and how things are design around inheritance.
Ruby's CSV parser can't compare to our naïve `split(",")`.
Processor is already used by UserProcessor which is different from the parsers, which may lead to confusion.
This is where it belongs. As a result, now the metadata parser doesn't force you to have the email in the second column. It'll just keep it out of the metadata JSON, that's all.
The CSV lib already does so.
Codecov Report
@@ Coverage Diff @@
## devel #2 +/- ##
========================================
Coverage ? 96.83%
========================================
Files ? 20
Lines ? 379
Branches ? 0
========================================
Hits ? 367
Misses ? 12
Partials ? 0 Continue to review full report at Codecov.
|
sauloperez
added a commit
to CoopCat-Confederacio-de-Cooperatives/decidim-coopcat
that referenced
this pull request
Sep 23, 2020
This brings in the changes in Platoniq/decidim-verifications-direct_verifications#2 so we can try them out in staging.
|
Love this @sauloperez ! looks amazing! I'll look through it carefully and probably update some instructions. This is great thanks a lot. |
sauloperez
commented
Sep 24, 2020
| * Reads CSV format with header (copy and paste it from your spreadsheet) | ||
| * Stores all columns except the email as authorization metadata | ||
|
|
||
| This enables querying the authorization metadata however fits you best. |
There was a problem hiding this comment.
It'd be worth documenting how to do that with an example and a link to PostgreSQL's docs
There was a problem hiding this comment.
A link to PostgreSQL's docs? what do you mean? how to export a csv from a sql table?
There was a problem hiding this comment.
I thought people may not be familiar with PostgreSQL JSON operators and it's worth pointing at the docs and show how it could be queried like in CoopCat-Confederacio-de-Cooperatives/decidim-coopcat#2 (comment).
microstudi
suggested changes
Sep 25, 2020
There was a problem hiding this comment.
@sauloperez Can you point this PR to the branch devel?
I want to accept it but I want to make some changes to it. These are the things I think that should be included before merging to master:
- Explain better the metadata mode, and make it work through the verification system. For instance, in user profile:
It should say the metadata associated with the verification. - When configuring some action, the workflow should show a select list where to associate the permissions restrictions with the metada:
- I'd like to allow admins to use the metadata mode without any further configuration (just a checkbox or similar).
I'll help to do all of this, but I need to point to devel first 🙏
Thanks again!
|
done @microstudi ! |
|
Now in text area accept something like: Name, email So I understand that we keep email for "user matching" and move Name, and N fields to metadata? So CSV or text area should look like: email,nom,pes,tipus Is it like this? |
|
exactly @sseerrggii. We still need to work on new copies though. |
|
@microstudi what if we merge this already and create separate issues for those topics? It'll be easier to manage and review IMO. We could then release a new version when all that is done. |
We are hitting this computation for every single line from the CSV we process.
Closed
Hopefully. It's now consistently passing but I don't know why. I also took the chance to tidy up the shared examples hoping that'd make it easier to find to root cause of the flakyness.
2 tasks
microstudi
added a commit
that referenced
this pull request
Jul 9, 2021
* support for CSV and multiple columns (#2) * Remove not found `url` gem A `bundle install` complaints with: ``` Fetching gem metadata from https://rubygems.org/........ Fetching gem metadata from https://rubygems.org/. Resolving dependencies.... Your bundle is locked to url (0.3.2), but that version could not be found in any of the sources listed in your Gemfile. If you haven't changed sources, that means the author of url (0.3.2) has removed it. You'll need to update your bundle to a version other than url (0.3.2) that hasn't been removed in order to install. ``` It looks like that the gem that depends on this one had the same issue and drop it. * Gitignore common files * Extract email parsing into class And move Parser's specs to a new class-specific file. * Replace send calls to public method No reason to use `#send` if the method is public. * Improve a bit specs wording * Accept email data as a hash and store as metadata * Enable toggling the parser from config This adds a new parser and while making it backward compatible. It'll only be enabled if you configure with `processor = :metadata`. It does so by extracting specific entry parsing into two classes. Now we have them both encapsulated and we can extend the metadata one however we want without affecting current behaviour. * Read CSV format Well, a bit rudimentary still, but also normalize a bit the entry format. Less flexible => less complexity => less buggy. * Accept CSV header and unlimited columns This turns the entry parsers into full text parsers each with different features, not only at the entry level. While we keep the simple parser that deals only with names for backward compatibility, the new metadata parser deals with CSV and unlimited columns, while being more strict in its input format. * Refactor parsers naming So they better convey the use of the template method pattern and how things are design around inheritance. * Use stdlib's CSV interface Ruby's CSV parser can't compare to our naïve `split(",")`. * Increase #register_users test coverage * Doc Metadata mode and better config key Processor is already used by UserProcessor which is different from the parsers, which may lead to confusion. * Move email and name normalization to parser This is where it belongs. As a result, now the metadata parser doesn't force you to have the email in the second column. It'll just keep it out of the metadata JSON, that's all. * Remove unnecessary chomp The CSV lib already does so. * Memoize header row parsing We are hitting this computation for every single line from the CSV we process. * Fix flaky spec Hopefully. It's now consistently passing but I don't know why. I also took the chance to tidy up the shared examples hoping that'd make it easier to find to root cause of the flakyness. * List imported authorizations (#4) * List direct_verification's authorization records * Implement destroy authorization action * Display user name instead of id in table column * Enforce permissions to index authorizations * Link to user stats for consistency * New Crowdin updates (#5) * New translations en.yml (Spanish) * New translations en.yml (Catalan) * New translations en.yml (Czech) * Fix host app not finding asset (#7) This fixes the error: ``` ActionView::Template::Error (The asset "decidim/direct_verifications/authorizations.css" is not present in the asset pipeline.) <%= stylesheet_link_tag "decidim/direct_verifications/authorizations" %> ``` * New Crowdin updates (#6) * New translations en.yml (Spanish) * New translations en.yml (Catalan) * New translations en.yml (Czech) * New translations en.yml (Czech) * New translations en.yml (Catalan) Co-authored-by: Pau Pérez Fabregat <[email protected]> * Remove codeclimate coverage (#12) * remove codeclimate coverage * change badge * udpate gemfile * add js configuration * add package lock * erblint fix * try to avoid a flaky test registering users * erblint fix * Fix authorizations collection to be scped by organization (#9) * Fix authorizations collection to be scped by organization * Fixed test * Added test to cover unscoped authorizations * [email protected] upgrade (#10) * Added missing argument to Verification::ConfirmUserAuthorization * Passed session from controller to the verification service * Fix authorizations collection to be scped by organization * Updated gems to fit [email protected] * Fixed specs * Fixed test * Added test to cover unscoped authorizations * Updated Gemfile.lock after resolving merge conflicts * Fixed UserProcessor call after merging devel Co-authored-by: Pau Pérez Fabregat <[email protected]> * New translations en.yml (Spanish) (#13) * Port info fix (#15) * Remove codeclimate coverage (#11) * remove codeclimate coverage * change badge * udpate gemfile * add js configuration * add package lock * erblint fix * try to avoid a flaky test registering users * restrict decidim version * gemfile update * bump version 0.21 * rollback version req * fix 0.22 verification changes * fix badge * fix users report * fix users check * bump version * fix gemfile * update gemfile * disable erblit check * fix tests * restore erblints * disable erblit check * Upgrade to [email protected] (#16) * Fix N+1 when rendering an auth's user name (#17) Turns this: ``` Rendering /home/pau/dev/decidim-verifications-direct_verifications/app/views/decidim/direct_verifications/verification/admin/authorizations/index.html.erb within layouts/decidim/admin/users Decidim::Authorization Load (0.9ms) SELECT "decidim_authorizations".* FROM "decidim_authorizations" LEFT OUTER JOIN "decidim_users" ON "decidim_users"."id" = "decidim_authorizations"."decidim_user_id" AND "decidim_users"."type" IN ('Decidim::User') LEFT OUTER JOIN "decidim_organizations" ON "decidim_organizations"."id" = "decidim_users"."decidim_organization_id" WHERE "decidim_organizations"."id" = $1 AND "decidim_authorizations"."name" = $2 AND "decidim_authorizations"."granted_at" IS NOT NULL [["id", 1], ["name", "direct_verifications"]] ↳ /home/pau/dev/decidim-verifications-direct_verifications/app/views/decidim/direct_verifications/verification/admin/authorizations/index.html.erb:24 Decidim::User Load (0.4ms) SELECT "decidim_users".* FROM "decidim_users" WHERE "decidim_users"."type" IN ('Decidim::User') AND "decidim_users"."id" = $1 LIMIT $2 [["id", 224], ["LIMIT", 1]] ↳ /home/pau/dev/decidim-verifications-direct_verifications/app/views/decidim/direct_verifications/verification/admin/authorizations/index.html.erb:30 Decidim::User Load (0.4ms) SELECT "decidim_users".* FROM "decidim_users" WHERE "decidim_users"."type" IN ('Decidim::User') AND "decidim_users"."id" = $1 LIMIT $2 [["id", 226], ["LIMIT", 1]] ↳ /home/pau/dev/decidim-verifications-direct_verifications/app/views/decidim/direct_verifications/verification/admin/authorizations/index.html.erb:30 Decidim::User Load (0.4ms) SELECT "decidim_users".* FROM "decidim_users" WHERE "decidim_users"."type" IN ('Decidim::User') AND "decidim_users"."id" = $1 LIMIT $2 [["id", 228], ["LIMIT", 1]] ↳ /home/pau/dev/decidim-verifications-direct_verifications/app/views/decidim/direct_verifications/verification/admin/authorizations/index.html.erb:30 Decidim::User Load (0.3ms) SELECT "decidim_users".* FROM "decidim_users" WHERE "decidim_users"."type" IN ('Decidim::User') AND "decidim_users"."id" = $1 LIMIT $2 [["id", 227], ["LIMIT", 1]] ↳ /home/pau/dev/decidim-verifications-direct_verifications/app/views/decidim/direct_verifications/verification/admin/authorizations/index.html.erb:30 ``` into this: ``` Rendering /home/pau/dev/decidim-verifications-direct_verifications/app/views/decidim/direct_verifications/verification/admin/authorizations/index.html.erb within layouts/decidim/admin/users Decidim::Authorization Load (0.9ms) SELECT "decidim_authorizations".* FROM "decidim_authorizations" LEFT OUTER JOIN "decidim_users" ON "decidim_users"."id" = "decidim_authorizations"."decidim_user_id" AND "decidim_users"."type" IN ('Decidim::User') LEFT OUTER JOIN "decidim_organizations" ON "decidim_organizations"."id" = "decidim_users"."decidim_organization_id" WHERE "decidim_organizations"."id" = $1 AND "decidim_authorizations"."name" = $2 AND "decidim_authorizations"."granted_at" IS NOT NULL [["id", 1], ["name", "direct_verifications"]] ↳ /home/pau/dev/decidim-verifications-direct_verifications/app/views/decidim/direct_verifications/verification/admin/authorizations/index.html.erb:24 Decidim::User Load (0.6ms) SELECT "decidim_users".* FROM "decidim_users" WHERE "decidim_users"."type" IN ('Decidim::User') AND "decidim_users"."id" IN ($1, $2, $3, $4) [["id", 224], ["id", 226], ["id", 228], ["id", 227]] ↳ /home/pau/dev/decidim-verifications-direct_verifications/app/views/decidim/direct_verifications/verification/admin/authorizations/index.html.erb:24 ``` * Mimemagic devel (#20) * Remove codeclimate coverage (#11) * remove codeclimate coverage * change badge * udpate gemfile * add js configuration * add package lock * erblint fix * try to avoid a flaky test registering users * restrict decidim version * gemfile update * bump version 0.21 * rollback version req * fix 0.22 verification changes * fix badge * fix users report * fix users check * bump version * update gems * Process registration individually (#21) * Extract user registration into a new command * Extract registration form to its own file * Move name fetch to caller It turns out that the user registration doesn't need other than the name so we can just completely decouple this class from the data hash. * Remove redundant method arguments These arguments are now the class' instance vars. * Remove unnecessary delegated methods * Move and adapt specs from user_processor_spec.rb We leave user_processor_spec.rb with some integration-like tests but move all the possible scenarios up to register_user_spec.rb, so we have a proper testing pyramid. * Move logging and tracking to instrumenter So RegisterUser doesn't need to know the low-level details of such logic and makes things more changeable. * Fix blank line * Simplify method definition a bit further The `form` local var doesn't provide any value and it's pretty clear that if there are any exceptions, they'll come from the `InviteUser` block. * Process authorization individually (#22) * Extract AuthorizeUser command * Test authorization validation * Refactor AuthorizeUser command * Test methods without #send They've become public now so there's no need for `#send`. * Remove authorization redundant specs and refactor * New Crowdin updates (#32) * New translations en.yml (Spanish) * New translations en.yml (French) * New translations en.yml (French) * New translations en.yml (French) * Async csv import (#28) * Extract RevokeUser command And I added more tests covering missing cases while at it. * Refactor command To simplify and increase readability * Move tests to unit-level and cover more cases * Check if object exists instead of instantiating It's not worth creating an unpersisted authorization just to avoid checking if we there was one already. It was a nice trick but it's not worth the cost of instantiating a model IMO. * Do not swallow exceptions revoking authorization First, given `RevokeUser` was already checking for the existence of authorization, the logic on `DestroyUserAuthorization` that did the same was pointless. At the point we hit it, there's always an authorization. As a result, and because there was no other `:invalid` scenario the `:invalid` callback was never called. Precisely, it was broken and no test failed. All the other possible failure scenarios will be caused by `authorization.destroy!` but rescue to track the error `instrumenter.add_error` would hide any exceptional behaviour we should be aware of and see in the app's error tracker. If a DB record can't be deleted it's likely that something is really wrong. * Extract Instrumenter class out of UserProcessor This a separate responsibility that deserves its own class and which hampers UserProcessor changeability. * Memomize user * Abstract instrumenter to not leak data structures This more abstract public API will enable us to provide different implementations of instrumenter without having to touch its consumers. * Replace uniqueness logic with Ruby Set data type Ruby keeps uniqueness for us 😎 * Move methods within their class * Create job to register a list of users Note we make use of ActiveJob's GlobalIDs instead of *_id arguments as we're used to. * Enable importing a CSV from a new page * Send email notification when users registered ok * Enforce permissions to import * Add error handling through form * Replace test CSV-building logic with file fixture * Map user input actions and test form * Improve registers users specs * Set up mail previews * Use Decidim's layout which includes org's info * Show user registration stats in email As we show the count of success and failures there's no need to a have a separate email notification to send on error. * Revoke users async * Extract base job class and use template methods This makes the algorithm more obvious and job classes less boilerplate-ish. * Use Decidim's client-side validation in form * Validate presence of :authorize as string * Remove unnecessary form label * Display CSV format template to improve UX A better solution is to give users a CSV file template they can download and modify. * Fix leading whitespace by removing I18n-ability I changed my mind. It's not worth making this translatable at this early stage and I find this leading whitespace worse than having only an English version of the template. * Move email delivery responsibility to base job This makes concrete user processing job classes even simpler. * Enable registering and authorizing users async This fixes the confusion we had with how the registering and authorizing actions are sent from the form. The former is optional but authorizing, revoking and checking come from radio buttons so there'll always come one in the request. Because the radio button enabled by default is "check", if only registering is select we'll only register users. See `create_import_form_spec.rb` for all the permutations. Note we don't do any processing when "check" is provided yet. * Decouple callers from the name fetching details * Use users sidebar in imports/new page * Allow trailing and leading whitespaces in CSVs The extra whitespace caused `RegisterUser`'s form to return a nil `#name` and thus, the fallback name was used although the CSV contained a name column. While at it, I also tried to make the system tests more closely mimic the manual acceptance test I'm doing when checking the email in my inbox. That's how I noticed the `Hola saulopefa+ava_test,` wasn't what I expected. * Replace DB checks with UI checks in system specs This makes these specs and thus the app more resilient. In the end, in a manual acceptance test we would have no other option than navigating to the authorizations page to see whether or not the authorization is granted. * Improve file import mail subject * Fix parsing empty columns Closes CoopCat-Confederacio-de-Cooperatives/decidim-coopcat#132. * Set hash value nil when no column is provided Better not to swallow it, so we can still trace the error if the user complains. We'll be able to see the authorization DB record and see the input was broken. * DRY and make spec contexts more evident * Navigate to CSV import from /direct_verifications Closes CoopCat-Confederacio-de-Cooperatives/decidim-coopcat#134. This simply makes it possible to reach the /direct_verifications/imports/new page that was previously introduced and demoed to ICA; the customer requesting this feature. * Serialize file path instead of its contents There's no way passing the file contents as a serialized argument could end well. * Revert "Merge pull request #23 from coopdevs/dont-serialize-file-contents" This reverts commit 9e30bad, reversing changes made to a6f4dcb. * Show error when no CSV header is provided This fixes https://sentry.io/organizations/cercles-coop/issues/2479093815. We infer the user didn't provide a header row (which happens very often) when any of the first row columns are empty. This is a much better UX than silent 500 error. The user will email us at best or simply abandon the job. * Provide CA and ES translations of #25 * Add authorization_handler field in CSV import page * Make RevokeUser work with authorization handlers This fixes the regression introduced in 5cc20be. * Make AuthorizeUser work w/ authorization handlers This fixes the regression introduced in 5cc20be. * Improve tests of registration use case Making the authorization_handler argument mandatory also makes things a bit more clear, because there's no conditional. * Make handler argument mandatory in RevokeUser Removing that conditional bit makes things a bit more clear and robust. * Bump version (#34) * Remove codeclimate coverage (#11) * remove codeclimate coverage * change badge * udpate gemfile * add js configuration * add package lock * erblint fix * try to avoid a flaky test registering users * restrict decidim version * gemfile update * bump version 0.21 * rollback version req * fix 0.22 verification changes * fix badge * fix users report * fix users check * bump version * update gems (#19) * update gems * downgrade declarative option * Bump glob-parent from 5.1.1 to 5.1.2 (#29) Bumps [glob-parent](https://github.com/gulpjs/glob-parent) from 5.1.1 to 5.1.2. - [Release notes](https://github.com/gulpjs/glob-parent/releases) - [Changelog](https://github.com/gulpjs/glob-parent/blob/main/CHANGELOG.md) - [Commits](gulpjs/glob-parent@v5.1.1...v5.1.2) --- updated-dependencies: - dependency-name: glob-parent dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump lodash from 4.17.20 to 4.17.21 (#26) Bumps [lodash](https://github.com/lodash/lodash) from 4.17.20 to 4.17.21. - [Release notes](https://github.com/lodash/lodash/releases) - [Commits](lodash/lodash@4.17.20...4.17.21) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump hosted-git-info from 2.8.8 to 2.8.9 (#25) Bumps [hosted-git-info](https://github.com/npm/hosted-git-info) from 2.8.8 to 2.8.9. - [Release notes](https://github.com/npm/hosted-git-info/releases) - [Changelog](https://github.com/npm/hosted-git-info/blob/v2.8.9/CHANGELOG.md) - [Commits](npm/hosted-git-info@v2.8.8...v2.8.9) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * New Crowdin updates (#31) * New translations en.yml (Spanish) * New translations en.yml (Catalan) * New translations en.yml (Czech) * New translations en.yml (Czech) * New translations en.yml (French) * New translations en.yml (French) * New translations en.yml (French) * version release * update cops * use standar configurable concern * use 2.7 version * update badges * cop * js lint * remove config spec * fix call Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Platoniq Bot <[email protected]> Co-authored-by: Pau Pérez Fabregat <[email protected]> Co-authored-by: Platoniq Bot <[email protected]> Co-authored-by: Màxim Colls <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What? Why?
This adds a new concept of parsers which can optionally be configured to support CSV and N columns. It keeps the existing behavior by default.
There are no restrictions in the format you provide, as long as you pass in a CSV header we can check against. This gives a high level of flexibility which should fit almost everyone's needs.
So a CSV like:
results in
authorizationentries like soExtras
Notice I had some issues with the
urldependency. More details in 63ce5e2.