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

[17.0][IMP] web_responsive: Redirect to home after login #2864

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

Patelangel1995
Copy link

Redirect to the home page after login will occur only if the user has enabled the 'Redirect to Home' configuration in their user profile settings.

@OCA-git-bot
Copy link
Contributor

Hi @Tardo, @SplashS, @yajo,
some modules you are maintaining are being modified, check this out!

@Patelangel1995
Copy link
Author

@maintainer_username Could you please review and merge this pull request? I have completed all the required checks, but I don't have the necessary permissions to merge it myself. Thank you!

@gabriel-grinspan
Copy link

@yajo @Tardo @SplashS
Please review.

@gabriel-grinspan
Copy link

@pedrobaeza

@pedrobaeza
Copy link
Member

pedrobaeza commented Jul 10, 2024

Please squash all the commits into one and follow commit message guidelines: https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#commit-message

This is [IMP], not [ADD].

@pedrobaeza pedrobaeza added this to the 17.0 milestone Jul 10, 2024
@@ -9,14 +9,14 @@
{
"name": "Web Responsive",
"summary": "Responsive web client, community-supported",
"version": "17.0.1.0.1",
"version": "17.0.1.0.2",
Copy link
Member

Choose a reason for hiding this comment

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

Don't change manually the version.

"category": "Website",
"website": "https://github.com/OCA/web",
"author": "LasLabs, Tecnativa, ITerra, Onestein, "
"Odoo Community Association (OCA)",
"license": "LGPL-3",
"installable": True,
"depends": ["web", "mail"],
"depends": ["base", "web", "mail"],
Copy link
Member

Choose a reason for hiding this comment

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

No need to add this dependency.

@@ -24,3 +24,11 @@ class ResUsers(models.Model):
default="milk",
required=True,
)
is_redirect_home = fields.Boolean(
string="Redirect to Home",
help="Automatically navigate to " "the home dashboard after signing in",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
help="Automatically navigate to " "the home dashboard after signing in",
help="Automatically navigate to the home dashboard after signing in",


@api.onchange("action_id")
def _onchange_action_id(self):
self.is_redirect_home = self.action_id and self.is_redirect_home
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this onchange.

Copy link
Author

Choose a reason for hiding this comment

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

@pedrobaeza
Apply the change to the code as described above. This onchange method will set the value of is_redirect_home to False when the user sets the Home Action under Preferences.

Copy link
Member

Choose a reason for hiding this comment

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

OK, although this can be implemented as a computed writable field for avoiding that this works only by UI. The commit message shouldn't contain the module version, and there's still a comment unattended.

Copy link
Author

Choose a reason for hiding this comment

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

@pedrobaeza
Updated as per the suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

@pedrobaeza Why do you prefer a compute field instead of a constraint?

Copy link
Member

Choose a reason for hiding this comment

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

Both are complementary. I only prefer computed writable over onchange.

@pedrobaeza pedrobaeza changed the title [ADD] redirect to home after login [17.0][IMP] web_responsive: Redirect to home after login Jul 11, 2024
@Patelangel1995
Copy link
Author

@pedrobaeza

Could you please initiate the process for the merging stage?

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Working as expected. Little improvement inline.

A second review is required according OCA guidelines: https://github.com/OCA/odoo-community.org/blob/master/website/Contribution/CONTRIBUTING.rst#review

Comment on lines 42 to 43
for user in self:
user.is_redirect_home = False if user.action_id else user.is_redirect_home
Copy link
Member

Choose a reason for hiding this comment

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

Little code improvement:

Suggested change
for user in self:
user.is_redirect_home = False if user.action_id else user.is_redirect_home
self.filtered("action_id").is_redirect_home = False

Copy link
Author

Choose a reason for hiding this comment

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

@pedrobaeza
Updated as per the suggestion.

@Patelangel1995
Copy link
Author

@pedrobaeza
Could you kindly move forward with the next step of the approval stage?

@pedrobaeza
Copy link
Member

@francesco-ooops
Copy link

@Patelangel1995 if you're changing module's functional behavior, this should be added to the documentation files

@Patelangel1995
Copy link
Author

@francesco-ooops
I have made updates to the documentation.

@francesco-ooops
Copy link

@Patelangel1995 ok, now you can squash commits

@Patelangel1995
Copy link
Author

@francesco-ooops
Merged all commits

- Redirect to the dashboard after logging in

![image](../static/img/redirecthome.gif)

Choose a reason for hiding this comment

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

Redirect to the home page after login will occur only if the user has enabled the 'Redirect to Home' configuration in their user profile settings

Copy link
Author

Choose a reason for hiding this comment

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

@francesco-ooops
I've updated it according to the suggestion.

Redirect to the home page after login will occur only if the user has enabled the 'Redirect to Home' configuration in their user profile settings
@francesco-ooops
Copy link

@Patelangel1995 I see that the feature is accessible only in dev mode. Is this intended?

@gabe-grinspan
Copy link

@francesco-ooops
It is as intentional as Odoo's choice to make the default home action a dev mode setting. It is consistent with the rest of the system.

Copy link

@francesco-ooops francesco-ooops left a comment

Choose a reason for hiding this comment

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

Functional ok!

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@pedrobaeza
Copy link
Member

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 17.0-ocabot-merge-pr-2864-by-pedrobaeza-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit c4d889b into OCA:17.0 Jul 17, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 290ec8e. Thanks a lot for contributing to OCA. ❤️

trisdoan pushed a commit to trisdoan/web that referenced this pull request Jul 20, 2024
Signed-off-by pedrobaeza
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants