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

Add an OFN UID column to the Users & Enterprises report #11041

Merged
merged 3 commits into from
Jun 25, 2023

Conversation

cillian
Copy link
Contributor

@cillian cillian commented Jun 16, 2023

What? Why?

export-ofn-uid-column.mp4

Here is what the extra column looks like in the .pdf version of the report....

export-ofn-uid-pdf

Note, SELECT enterprises.id AS ofn_uid, ... is used instead of just SELECT enterprises.id, .... This is because if you do the latter, then the translation key in config/locales/en.yml is report_header_id which is vague and not as clear as report_header_ofn_uid.

What should we test?

See testing steps in #10545.

Release notes

Changelog Category: User facing changes

@cillian cillian self-assigned this Jun 16, 2023
@cillian cillian force-pushed the export-ofn-uid-column branch from 09964a9 to 3412786 Compare June 16, 2023 15:00
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Great, thank you!

describe "Users & Enterprises reports" do
include AuthenticationHelper

let!(:enterprise) { create(:supplier_enterprise) }
Copy link
Member

Choose a reason for hiding this comment

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

You are assuming that all future specs will use an enterprise. But maybe we also want to test what happens when the database is empty. I would have created this later in the it block. But your version is good for now, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 fixed by a579333

end

it "displays the report" do
find("[type='submit']").click
Copy link
Member

Choose a reason for hiding this comment

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

This is brittle and will fail if we ever add any other form to the page. Other specs use click_button "Go". It also looks more like what the user does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, fixed by 41dead8

@drummer83 drummer83 self-assigned this Jun 25, 2023
@drummer83 drummer83 added the pr-staged-fr staging.coopcircuits.fr label Jun 25, 2023
@drummer83 drummer83 force-pushed the export-ofn-uid-column branch from a579333 to 8ab077b Compare June 25, 2023 17:01
@drummer83
Copy link
Contributor

Hi @cillian,
Thanks for this one! I have tested this on staging FR.

After staging your PR

  • 'OFN UID' is listed in the columns dropdown menu. ✔️
  • It is deactivated by default. ❓
    In the original issue it is mentioned that it should be available by default. Not sure if this means that it should be activated by default. However I think it is better to have it deactivated by default because that way nothing changes for users.
    image
  • Activating 'OFN UID' and rendering a report displays the Enterprise ID in an additional column. ✔️
    image
  • I verified that the UID is showing the correct value. ✔️
    image
  • I compared the report content before and after your PR: data is identical, but OFN UID is added of course. ✔️
  • I also checked filtering of the report: Works as before. ✔️
  • Exported files include the new column as well. ✔️

Great work, thank you! 🎉 This one is ready to go! Merging! 🚀

@drummer83 drummer83 merged commit 8a8d78c into openfoodfoundation:master Jun 25, 2023
@drummer83 drummer83 removed the pr-staged-fr staging.coopcircuits.fr label Jun 25, 2023
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.

[Reports] Add Enterprise ID number (OFN UID) as field in the Users & Enterprises admin report
4 participants