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.

Sign up for GitHub

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

Jump to bottom

feat: allow staff to access detailed impacts from explorer. #878

Merged
merged 7 commits into from
Jan 8, 2025
Merged

feat: allow staff to access detailed impacts from explorer. #878

merged 7 commits into from
Jan 8, 2025

Conversation

n1k0
Copy link
Member

@n1k0 n1k0 commented Jan 6, 2025 •
edited
Loading

🔧 Problem

Notion card

Now that detailed processes impacts are encrypted in the repo, there's no UI to browse them. Ecobalyse staff (team members) should be able to browser and navigate them easily from the web UI and CSV exports.

capture

🍰 Solution

  • Check if the authenticated user is a staff member, and if so render detailed processes impacts in explorers
  • Allow sorting impact columns
  • Add detailed impacts columns to CSV exports for staff members

🚨 Points to watch/comments

  • Process explorer pages for food, objects and textile have been merged into a single Elm module, so they now all share the same UI and features, hence the LOC reduction in the diff.
  • It might be a good idea to check how hackable is the check for a staff boolean by faking the auth endpoint response, but I suspect we could live with that anyway.

🏝️ How to test

You need two accounts on Ecobalyse, one with the is_staff flag enabled, and the other one without it (note: your own personal existing team account probably has this flag enabled already). The account page features a note about this bit being enabled when connected on the platform:

image

Then, check that:

  • a logged-in staff member sees detailed impacts in explorers
  • a logged-in non-staff member can't access detailed impacts in explorers
  • unauthenticated users cant's access detailed impacts in explorers
  • detailed impacts are also listed in the detailed process modal view (to open one, click a table row)
  • detailed impacts are exported in the CSV export when authenticated as a staff member
  • detailed impacts are NOT exported in the CSV export when authenticated as a regular member

@n1k0 n1k0 force-pushed the feat/expose-detailed-processes-impacts-to-staff branch from 3337deb to 71b6c66 Compare January 6, 2025 15:30
@n1k0 n1k0 changed the title feat: expose User.staff bool feat: allow staff to access detailed process impacts from explorer UI Jan 6, 2025
@n1k0 n1k0 marked this pull request as ready for review January 6, 2025 22:07
@n1k0 n1k0 force-pushed the feat/expose-detailed-processes-impacts-to-staff branch from 5b5f79c to db0411d Compare January 6, 2025 22:08
@n1k0 n1k0 requested a review from vjousse January 6, 2025 22:08
@n1k0 n1k0 changed the title feat: allow staff to access detailed process impacts from explorer UI feat: allow staff to access detailed impacts from explorer. Jan 7, 2025
@n1k0 n1k0 force-pushed the feat/expose-detailed-processes-impacts-to-staff branch from ceb4f5d to e0f5487 Compare January 7, 2025 10:17
vjousse
vjousse reviewed Jan 7, 2025
View reviewed changes
Copy link
Collaborator

@vjousse vjousse left a comment

Choose a reason for hiding this comment

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

I’m trying to test it locally, but I have a strange behavior: once logged in, and the « Mon compte » menu displayed in the top right, if I refresh the page it seems that I am disconnected, the « Mon compte » link goes back to « Connexion ou inscription ». Do you see the same problem?

@n1k0
Copy link
Member Author

n1k0 commented Jan 7, 2025

Do you see the same problem?

Yes and it also happens on production, so we should probably file a card about it :(

@vjousse
Copy link
Collaborator

vjousse commented Jan 8, 2025

Do you see the same problem?

Yes and it also happens on production, so we should probably file a card about it :(

It works fine on production for me, but not on this review app, that's pretty strange 🤔

@vjousse
Copy link
Collaborator

vjousse commented Jan 8, 2025

@n1k0 the problem is with the local storage decoding. The staff field is missing in my local storage (so I suppose that the Elm json decode is failing), if I add it by hand it works again as expected.

@n1k0
Copy link
Member Author

n1k0 commented Jan 8, 2025 •
edited
Loading

the problem is with the local storage decoding. The staff field is missing in my local storage (so I suppose that the Elm json decode is failing), if I add it by hand it works again as expected.

Damn, good catch. I'll fix the bug in this PR but we should probably warn in some way that decoding has failed, for another PR. I'll file a card about that.
Edit: 49ba6f7

@n1k0 n1k0 force-pushed the feat/expose-detailed-processes-impacts-to-staff branch from 5f6fce3 to 49ba6f7 Compare January 8, 2025 08:30
vjousse
vjousse approved these changes Jan 8, 2025
View reviewed changes
Copy link
Collaborator

@vjousse vjousse left a comment

Choose a reason for hiding this comment

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

Awesome, LGTM!

@n1k0
Copy link
Member Author

n1k0 commented Jan 8, 2025

@vjousse thanks! I'll wait for Camille's approval to land this patch. Meanwhile I filed an issue about the deserialization bug mentioned earlier: https://www.notion.so/Bug-d-codage-session-utilisateur-depuis-localStorage-8e135bd915a0423c89e67756f40074a1

@n1k0
Copy link
Member Author

n1k0 commented Jan 8, 2025

@vjousse wait, actually scratch that, I'm gonna land the patch onto staging because I have a demo planned in 15 minutes 🤣

@n1k0 n1k0 merged commit 7c8868d into master Jan 8, 2025
6 checks passed
@n1k0 n1k0 deleted the feat/expose-detailed-processes-impacts-to-staff branch January 8, 2025 08:46
@github-actions github-actions bot mentioned this pull request Jan 8, 2025
Merged
@n1k0 n1k0 mentioned this pull request Jan 8, 2025
Merged
vjousse pushed a commit that referenced this pull request Jan 13, 2025
@github-actions
chore(release): release v3.0.0 (#845)
c0c02a3 
## [3.0.0](https://github.com/MTES-MCT/ecobalyse/compare/v2.7.1..v3.0.0)
(2025-01-13)



### 🚀 Features

- Generalize density, electricity, heat and waste process fields
([#855](#855))
- *(data)* Ensure consistent nullable alias field in all processes
files. ([#862](#862))
- Add betagouv logo.
([#848](#848))
- *(data)* Unified, cross-domain processes file format.
([#866](#866))
- *(data)* Validate processes files against a JSON schema.
([#869](#869))
- *(data,textile)* Add trim process and components data.
([#824](#824))
- *(textile)* Implement trims.
([#873](#873))
- *(data,ui)* Add trims to more textile examples, render them in
explorer ([#876](#876))
- Allow expanding trim details.
([#877](#877))
- Allow staff to access detailed impacts from explorer.
([#878](#878))

### 🪲 Bug Fixes

- *(food)* [**breaking**] Food processes identifiers are now UUIDs
([#844](#844))
- *(data)* [**breaking**] Update textile process ids to use UUID format
([#858](#858))
- Data pipeline with new UUIDs
([#857](#857))
- Fix api error with old versions
([#851](#851))
- Broken homepage after upgrading highcharts
([#863](#863))
- *(dev)* Fix npm ci error with `transcrypt`
([#870](#870))
- Correct data on trims
([#879](#879))
- Warn on session data decoding error.
([#884](#884))
- *(textile)* Apply durability to trims impacts.
([#886](#886))
- Update PEF score label.
([#887](#887))

### 🚜 Refactor

- Move textile step_usage field to categories.
([#850](#850))
- *(data)* Move textile process "correctif" to comment
([#852](#852))
- Add encrypted detailed impacts files to the source code
([#840](#840))
- Abstract components.
([#872](#872))
- Order json keys
([#871](#871))

### 📚 Documentation

- Fix openapi food examples
([#867](#867))

### ⚙️ Miscellaneous Tasks

- Increase API test timeout
([#853](#853))
- *(data)* Remove system_description process field.
([#859](#859))
- Upgrade dependencies, December 2024.
([#860](#860))
- Remove obsolete/unused info textile process field.
([#861](#861))
- *(data)* Merge PastoEco in a single file to speedup imports and fixed
linking to AGB
([#833](#833))
- Fix score_history workflow for transcrypt
([#864](#864))
- Standardize number formatting across codebase
([#804](#804))
- Standardize tkm unit
([#868](#868))
- Remove obsolete pre-commit command.
([#874](#874))
- Update trim api parameter ordering.
([#875](#875))
- Remove data directory, now in `ecobalyse-data` repo
([#888](#888))
- Update crypto-related docs.
([#890](#890))
- *(security)* Upgrade django to >=5.1.4.
([#885](#885))
- Readd score_history
([#891](#891))

<!-- generated by git-cliff -->

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Jan 13, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vjousse vjousse vjousse approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@n1k0 @vjousse