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

[features][Auth logo customization] #15096

Merged
merged 25 commits into from Jan 20, 2023
Merged

[features][Auth logo customization] #15096

merged 25 commits into from Jan 20, 2023

Conversation

ronronscelestes
Copy link
Contributor

@ronronscelestes ronronscelestes commented Dec 6, 2022

⚠️ Before merging:

What

Iteration of the menu logo customization feature
As a user I should be able to customize the auth pages logo through the admin.

Snapshots

image
image

Tests

Temporary setup:

  • Use features/authLogo to run the admin
  • Use authLogo/backend to run getstarted

Customize menu and auth logo

Acceptance criteria

  • Given that I am in the Overview page of the Settings with default logo auth pages
    • I can customize the auth pages logo by submitting a customized image
    • Autothorized image extensions should only be jpeg, png, svg
    • Authorized max size should be 100KB
    • Authorized max dimension should be 750px
  • Given that I am in the Overview page of the Settings with a customized logo auth pages
    • I can reset the auth logo to the default one and submit
  • Given that I have a customized auth logo
    • I can go to login, reset password, lost password pages and see my customized logo
  • Given that I don’t have update persmissions on project settings but I have read permission
    • I should be able to see the logo inputs in Overview page
    • I shouldn’t be able to update menu or auth logos
  • Given that I don’t have read persmissions on project settings
    • I shouldn’t be able to see the logo inputs
    • I shouldn’t be able to see the Save button

@ronronscelestes ronronscelestes added source: core:admin Source is core/admin package pr: feature This PR adds a new feature flag: documentation This PR requires a documentation update labels Dec 6, 2022
@codecov
Copy link

codecov bot commented Dec 6, 2022

Codecov Report

Base: 50.37% // Head: 60.57% // Increases project coverage by +10.19% 🎉

Coverage data is based on head (6960c98) compared to base (133fb98).
Patch coverage: 90.42% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##             main   #15096       +/-   ##
===========================================
+ Coverage   50.37%   60.57%   +10.19%     
===========================================
  Files         292     1353     +1061     
  Lines       10310    33243    +22933     
  Branches     2286     6363     +4077     
===========================================
+ Hits         5194    20136    +14942     
- Misses       4225    11270     +7045     
- Partials      891     1837      +946     
Flag Coverage Δ
back 50.40% <100.00%> (+0.02%) ⬆️
front 65.14% <89.28%> (?)
unit_back 50.40% <100.00%> (+0.02%) ⬆️
unit_front 65.14% <89.28%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...n/src/components/ConfigurationsProvider/reducer.js 100.00% <ø> (ø)
...s/core/admin/admin/src/core/utils/axiosInstance.js 76.47% <ø> (ø)
packages/core/admin/admin/src/pages/App/index.js 66.66% <0.00%> (ø)
...ionInfosPage/components/CustomizationInfos/init.js 100.00% <ø> (ø)
.../admin/admin/src/permissions/defaultPermissions.js 100.00% <ø> (ø)
...onInfosPage/components/CustomizationInfos/index.js 82.35% <82.35%> (ø)
...s/SettingsPage/pages/ApplicationInfosPage/index.js 90.62% <89.47%> (ø)
...min/src/components/ConfigurationsProvider/index.js 100.00% <100.00%> (ø)
.../admin/src/components/UnauthenticatedLogo/index.js 100.00% <100.00%> (ø)
...InfosPage/components/CustomizationInfos/reducer.js 100.00% <100.00%> (ø)
... and 1066 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@joshuaellis
Copy link
Member

@ronronscelestes is this ready to review? 👀

@ronronscelestes
Copy link
Contributor Author

ronronscelestes commented Jan 11, 2023

@ronronscelestes is this ready to review? 👀

I'll have a last look beginning of the afternoon + update the branch with main and then will mark it as ready to review 😁

@ronronscelestes ronronscelestes marked this pull request as ready for review January 11, 2023 12:57
@ronronscelestes ronronscelestes removed the pr: feature This PR adds a new feature label Jan 11, 2023
@ronronscelestes ronronscelestes added pr: enhancement This PR adds or updates some part of the codebase or features and removed flag: documentation This PR requires a documentation update labels Jan 11, 2023
@ronronscelestes
Copy link
Contributor Author

@joshuaellis @gu-stav
since updating the branch with main updating logos do not work because of the axios bump included in the last release
Content-Type and headers need to be fixed and I could fix it directly in the axiosInstance used here

we also discussed using Simone's hooks to handle requests but:

  • the helpers from the admin are currently having the same headers bug
  • I don't know when the hooks will be ready and fixed in the helper-plugin

wdyt, should I wait for getFetchClient and useFetchClient to be fully ready and merged before moving on?
I'd like to merge this before the end of the sprint though

@gu-stav
Copy link
Contributor

gu-stav commented Jan 11, 2023

@ronronscelestes I think you could move forward with axiosInstance and fixing the headers directly there because I won't cause any harm (TM). Since Simones work takes a bit of time, we can do the refactoring later on.

@ronronscelestes
Copy link
Contributor Author

@gu-stav @joshuaellis ready to review with the headers fix!

Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

Minor comment about a mis-name.

This could be a wider discussion but I don't understand why we're doing this and just want to get some vibes:
CleanShot 2023-01-12 at 13 55 01@2x

we have a component (that needs a reducer) inside a page that inside another page – IMO it's bonkers organisation / architecture (w/e you wanna call it). I'm wondering if we should clean up some areas in the next sprint....?? cc @gu-stav

@gu-stav
Copy link
Contributor

gu-stav commented Jan 12, 2023

we have a component (that needs a reducer) inside a page that inside another page – IMO it's bonkers organisation / architecture (w/e you wanna call it). I'm wondering if we should clean up some areas in the next sprint....?? cc @gu-stav

Did somebody say cleanup + refactoring? Hell yes 👍🏼

@ronronscelestes
Copy link
Contributor Author

we have a component (that needs a reducer) inside a page that inside another page – IMO it's bonkers organisation /
architecture (w/e you wanna call it). I'm wondering if we should clean up some areas in the next sprint....?? cc @gu-stav

Did somebody say cleanup + refactoring? Hell yes 👍🏼

creating a Jira ticket to refine 😄

Copy link
Contributor

@Marc-Roig Marc-Roig left a comment

Choose a reason for hiding this comment

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

LGTM! The acceptance criteria was super well defined, well done :)

@joshuaellis joshuaellis added this to the 4.6.0 milestone Jan 20, 2023
@ronronscelestes ronronscelestes merged commit e66609e into main Jan 20, 2023
@ronronscelestes ronronscelestes deleted the features/authLogo branch January 20, 2023 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: enhancement This PR adds or updates some part of the codebase or features source: core:admin Source is core/admin package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants