[14.0] [ADD] Module web_responsive_company_color #2861

anusriNPS · 2024-06-25T06:42:37Z

Module web_responsive_company_color is introduced for customizing web elements that are part of web_responsive that needs customization

Limitations: If new web elements are included in module, one needs to reinstall web_responsive_company_color for customization to take effect

PicchiSeba

Code review

PicchiSeba · 2024-07-05T06:50:18Z

web_company_color/models/res_company.py

+            if not uninstall:
+              datas = base64.b64encode(record._scss_generate_content().encode("utf-8"))
+            else:
+              datas = base64.b64encode(record._scss_generate_content(uninstall).encode("utf-8"))


suggestion: in the method _scss_generate_content uninstall is already handled. We could just call the method without checking if uninstall is falsy

Suggested change

if not uninstall:

datas = base64.b64encode(record._scss_generate_content().encode("utf-8"))

else:

datas = base64.b64encode(record._scss_generate_content(uninstall).encode("utf-8"))

datas = base64.b64encode(record._scss_generate_content(uninstall).encode("utf-8"))```

Implemented suggested change

PicchiSeba · 2024-07-05T06:52:05Z

web_responsive_company_color/models/res_company.py

+    def _get_scss_template_uninstall(self):
+        return super()._get_scss_template()


question: What does this method do?.

This method tries to remove web elements introduced by the module for customization during uninstall, so that when this module is uninstalled customization related to its web elements are removed.

During uninstall, web_company_color module tries to return self._get_scss_template() % self._scss_get_sanitized_values() using _scss_generate_content() method.

Whenever, self._get_scss_template() is called, it provides the entire scss_template including web element which has be removed for the module for which uninstalled is performed.

In order to avoid including web elements of module which are about to uninstall, calling a scss_template_uninstall method in the module which tries uninstall. As only parent of scss_template is called in this method, I am able to remove customized web elements of module for which uninstall is performed.

If not, though module is uninstalled, customization of web element introduced by the uninstalled module remains.

PicchiSeba · 2024-07-05T07:53:06Z

web_company_color/models/res_company.py

+               custom_attachment.sudo().write(values)
            else:
-                values.update({"type": "binary", "mimetype": "text/scss"})
-                IrAttachmentObj.sudo().create(values)
+               values.update({"type": "binary", "mimetype": "text/scss"})
+               IrAttachmentObj.sudo().create(values)


issue: we lost the indentation here

Indentation is restored

renda-dev · 2024-07-12T12:51:58Z

web_company_color/models/res_company.py

-        return self.SCSS_TEMPLATE % self._scss_get_sanitized_values()
+        if uninstall:
+            return (
+                self._get_scss_template_uninstall() % self._scss_get_sanitized_values()


issue: _get_scss_template_uninstall does not exist in this module and it would raise an error on the module uninstall.

suggestion: instead of creating a new method you could add a parameter inside the context in the "uninstall_hook", check for that inside this method and call super() instead of self

I tried to uninstall web_company_color but did not see any error. However, you are right, it might raise error without _get_scss_template_uninstall function here

I also tried to update context from uninstall_hook and use super() instead of self, but uninstall fails saying super has no attribute

It is trying to call the parent of web_company_color instead of the parent which needs to be called for module which is uninstalled

renda-dev · 2024-07-15T07:25:07Z

web_responsive_company_color/models/res_company.py

+    # One need to reinstall web_responsive_company_color module
+    # to see customization takes effect when new elements added
+    # to _get_scss_template
+    def _get_scss_template(self):
+        return (
+            super()._get_scss_template()
+            + """
+             .o_menu_apps .dropdown-menu {
+                background: url('/web_responsive/static/img/home-menu-bg-overlay.svg'),
+                  linear-gradient(
+                    to bottom,
+                    %(color_navbar_bg)s,
+                    desaturate(lighten(%(color_navbar_bg)s, 20%%), 15)
+                  );
+             }
+            """
+        )


suggestion: In my opinion you should:

Delete _get_scss_template_uninstall, could only cause issues, not needed

Add a context variable in the uninstall hook (something like: uninstall_scss=True) before executing scss_create_or_update_attachment

Check inside _get_scss_template if there is the variable in the context:

If yes: just return super, you only need the basic scss template

If not: call super and add your customizations

Sorry for misunderstanding the suggestion. Thank your for detailed response. Updated solution as suggested

renda-dev · 2024-07-17T12:53:18Z

web_responsive_company_color/hooks.py

+
+
+def uninstall_hook(cr, registry):
+    env = api.Environment(cr, SUPERUSER_ID, {"uninstall": True})


nitpick: just uninstall could lead to conflicts / misunderstandings, could we make it more specific? Something like uninstall_scss would be enough

updated to uninstall_scss in the context

renda-dev · 2024-07-17T12:55:04Z

web_company_color/models/res_company.py

+    def _scss_generate_content(self, uninstall=False):
        self.ensure_one()
        # ir.attachment need files with content to work
        if not self.company_colors:
            return "// No Web Company Color SCSS Content\n"
-        return self.SCSS_TEMPLATE % self._scss_get_sanitized_values()
+        if uninstall:
+            return (
+                self._get_scss_template_uninstall() % self._scss_get_sanitized_values()


suggestion: now that _get_scss_template_uninstall doesn't actually exist, you could remove it (also from the method's parameters) and check for the context variable instead

I missed to push the changes for scss_generate as testing was handled in different repo.

Not checking context variable inside _scss_generate_content and it is checked only inside _get_scss_template

renda-dev · 2024-07-17T12:56:18Z

web_company_color/models/res_company.py

+    def scss_create_or_update_attachment(self, uninstall=False):
        IrAttachmentObj = self.env["ir.attachment"]
        for record in self:
-            datas = base64.b64encode(record._scss_generate_content().encode("utf-8"))
+            datas = base64.b64encode(
+                record._scss_generate_content(uninstall).encode("utf-8")


nitpick: you can get uninstall directly from the context inside the _scss_generate_content method, these changes could be removed

Removed these changes

renda-dev

Code and functional review, LGTM

[14.0][FEAT]Enhanced web_company_color function to support uninstall

64aac3f

anusriNPS marked this pull request as draft June 25, 2024 06:42

pedrobaeza added this to the 14.0 milestone Jun 25, 2024

anusriNPS force-pushed the 14.0-scss-styling branch 2 times, most recently from 9a82e07 to 7badcf2 Compare June 28, 2024 10:23

anusriNPS marked this pull request as ready for review July 1, 2024 15:17

PicchiSeba reviewed Jul 5, 2024

View reviewed changes

anusriNPS force-pushed the 14.0-scss-styling branch 2 times, most recently from be1290d to dd1436e Compare July 5, 2024 07:51

PicchiSeba suggested changes Jul 5, 2024

View reviewed changes

anusriNPS force-pushed the 14.0-scss-styling branch from dd1436e to c488ead Compare July 9, 2024 11:02

renda-dev reviewed Jul 12, 2024

View reviewed changes

renda-dev reviewed Jul 15, 2024

View reviewed changes

anusriNPS force-pushed the 14.0-scss-styling branch 2 times, most recently from 78af2a9 to afc3d92 Compare July 15, 2024 08:50

renda-dev suggested changes Jul 17, 2024

View reviewed changes

anusriNPS force-pushed the 14.0-scss-styling branch 2 times, most recently from c4f326b to 3499f08 Compare July 17, 2024 14:06

anusriNPS requested a review from renda-dev July 17, 2024 14:34

anusriNPS force-pushed the 14.0-scss-styling branch from 3499f08 to fab4cf0 Compare July 23, 2024 13:36

renda-dev approved these changes Jul 23, 2024

View reviewed changes

anusriNPS force-pushed the 14.0-scss-styling branch from fab4cf0 to 4969e25 Compare July 23, 2024 14:54

[14.0][ADD] Module web_responsive_company_color

f729039

anusriNPS force-pushed the 14.0-scss-styling branch from 4969e25 to f729039 Compare July 24, 2024 09:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[14.0] [ADD] Module web_responsive_company_color #2861

[14.0] [ADD] Module web_responsive_company_color #2861

anusriNPS commented Jun 25, 2024 •

edited

Loading

PicchiSeba left a comment

PicchiSeba Jul 5, 2024

anusriNPS Jul 9, 2024

PicchiSeba Jul 5, 2024

anusriNPS Jul 9, 2024

PicchiSeba Jul 5, 2024

anusriNPS Jul 9, 2024

renda-dev Jul 12, 2024 •

edited

Loading

anusriNPS Jul 15, 2024

renda-dev Jul 15, 2024

anusriNPS Jul 15, 2024

renda-dev Jul 17, 2024

anusriNPS Jul 17, 2024

renda-dev Jul 17, 2024

anusriNPS Jul 17, 2024 •

edited

Loading

renda-dev Jul 17, 2024

anusriNPS Jul 17, 2024

renda-dev left a comment

		def _get_scss_template_uninstall(self):
		return super()._get_scss_template()



		def uninstall_hook(cr, registry):
		env = api.Environment(cr, SUPERUSER_ID, {"uninstall": True})

[14.0] [ADD] Module web_responsive_company_color #2861

Are you sure you want to change the base?

[14.0] [ADD] Module web_responsive_company_color #2861

Conversation

anusriNPS commented Jun 25, 2024 • edited Loading

PicchiSeba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renda-dev Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anusriNPS Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renda-dev left a comment

Choose a reason for hiding this comment

anusriNPS commented Jun 25, 2024 •

edited

Loading

renda-dev Jul 12, 2024 •

edited

Loading

anusriNPS Jul 17, 2024 •

edited

Loading