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

[15.0][IMP] web_notify: action button name and close #2789

Merged

Conversation

chienandalu
Copy link
Member

@chienandalu chienandalu commented Apr 1, 2024

  • We can now set a button name to the notification action.
  • We can set an icon button as well.
  • After the button is clicked, the notification gets closed.

image

cc @Tecnativa TT47134

take a look @IT-Ideas @rousseldenis

ping @sergio-teruel

@pedrobaeza pedrobaeza added this to the 15.0 milestone Apr 1, 2024
@chienandalu chienandalu force-pushed the 15.0-imp-web_notify-close-notification branch from c0b534c to 3af38cd Compare April 2, 2024 10:22
Copy link
Contributor

@sergio-teruel sergio-teruel left a comment

Choose a reason for hiding this comment

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

Great!!

@chienandalu
Copy link
Member Author

could you review @carlosdauden @CarlosRoca13 ?

Copy link
Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

LGTM

@CarlosRoca13
Copy link
Contributor

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 15.0-ocabot-merge-pr-2789-by-CarlosRoca13-bump-minor, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Sep 20, 2024
Signed-off-by CarlosRoca13
@OCA-git-bot
Copy link
Contributor

@CarlosRoca13 The merge process could not be finalized, because command twine check odoo_addon_web_notify-15.0.2.1.0-py3-none-any.whl failed with output:

Checking odoo_addon_web_notify-15.0.2.1.0-py3-none-any.whl: �[31mFAILED�[0m
�[31mERROR   �[0m `long_description` has syntax errors in markup and would not be        
         rendered on PyPI.                                                      
         line 109: Warning: Cannot scale image!                                 
           Could not get size from                                              
         "https://raw.githubusercontent.com/OCA/web/15.0/web_notify/static/descr
         iption/notifications_screenshot.gif":                                  
           Requires Python Imaging Library.                                     
           Reading external files disabled.                                     
�[33mWARNING �[0m `long_description_content_type` missing. defaulting to `text/x-rst`.   

@CarlosRoca13
Copy link
Contributor

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 15.0-ocabot-merge-pr-2789-by-CarlosRoca13-bump-minor, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Sep 20, 2024
Signed-off-by CarlosRoca13
@OCA-git-bot
Copy link
Contributor

@CarlosRoca13 The merge process could not be finalized, because command twine check odoo_addon_web_notify-15.0.2.1.0-py3-none-any.whl failed with output:

Checking odoo_addon_web_notify-15.0.2.1.0-py3-none-any.whl: �[31mFAILED�[0m
�[31mERROR   �[0m `long_description` has syntax errors in markup and would not be        
         rendered on PyPI.                                                      
         line 109: Warning: Cannot scale image!                                 
           Could not get size from                                              
         "https://raw.githubusercontent.com/OCA/web/15.0/web_notify/static/descr
         iption/notifications_screenshot.gif":                                  
           Requires Python Imaging Library.                                     
           Reading external files disabled.                                     
�[33mWARNING �[0m `long_description_content_type` missing. defaulting to `text/x-rst`.   

@CarlosRoca13
Copy link
Contributor

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 15.0-ocabot-merge-pr-2789-by-CarlosRoca13-bump-minor, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Sep 20, 2024
Signed-off-by CarlosRoca13
@OCA-git-bot
Copy link
Contributor

@CarlosRoca13 The merge process could not be finalized, because command twine check odoo_addon_web_notify-15.0.2.1.0-py3-none-any.whl failed with output:

Checking odoo_addon_web_notify-15.0.2.1.0-py3-none-any.whl: �[31mFAILED�[0m
�[31mERROR   �[0m `long_description` has syntax errors in markup and would not be        
         rendered on PyPI.                                                      
         line 109: Warning: Cannot scale image!                                 
           Could not get size from                                              
         "https://raw.githubusercontent.com/OCA/web/15.0/web_notify/static/descr
         iption/notifications_screenshot.gif":                                  
           Requires Python Imaging Library.                                     
           Reading external files disabled.                                     
�[33mWARNING �[0m `long_description_content_type` missing. defaulting to `text/x-rst`.   

@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). 🤖

'res_id': self.id,
'views': [(False, 'form')],
})
action = self.env["ir.actions.act_window"]._for_xml_id('sale.action_orders')
Copy link
Member

Choose a reason for hiding this comment

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

You have reduced here one space of indentation, which is not really correct and increases the diff.

'res_id': self.id,
'views': [(False, 'form')],
})
action = self.env["ir.actions.act_window"]._for_xml_id('sale.action_orders')
Copy link
Member

Choose a reason for hiding this comment

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

The same here.

:scale: 80 %
:alt: Sample notifications

You can test the behaviour of the notifications by installing this module in a demo database.
Access the users form through Settings -> Users & Companies. You'll see a tab called "Test web notify", here you'll find two buttons that'll allow you test the module.

.. figure:: static/description/test_notifications_demo.png
.. figure:: ../static/description/test_notifications_demo.png
Copy link
Member

Choose a reason for hiding this comment

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

Try to put the image on the img folder:

Suggested change
.. figure:: ../static/description/test_notifications_demo.png
.. figure:: ../static/img/test_notifications_demo.png

and rebuild the README (forcing it).

@chienandalu chienandalu force-pushed the 15.0-imp-web_notify-close-notification branch from feb0dbe to a91a2e1 Compare September 20, 2024 10:48
@chienandalu
Copy link
Member Author

Changes done, let's see if merge bot is happy now (not very confident thou)

@pedrobaeza
Copy link
Member

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 15.0-ocabot-merge-pr-2789-by-pedrobaeza-bump-patch, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Sep 20, 2024
Signed-off-by pedrobaeza
@OCA-git-bot
Copy link
Contributor

@pedrobaeza The merge process could not be finalized, because command twine check odoo_addon_web_notify-15.0.2.0.1-py3-none-any.whl failed with output:

Checking odoo_addon_web_notify-15.0.2.0.1-py3-none-any.whl: �[31mFAILED�[0m
�[31mERROR   �[0m `long_description` has syntax errors in markup and would not be        
         rendered on PyPI.                                                      
         line 109: Warning: Cannot scale image!                                 
           Could not get size from                                              
         "https://raw.githubusercontent.com/OCA/web/15.0/web_notify/static/img/n
         otifications_screenshot.gif":                                          
           Requires Python Imaging Library.                                     
           Reading external files disabled.                                     
�[33mWARNING �[0m `long_description_content_type` missing. defaulting to `text/x-rst`.   

@chienandalu
Copy link
Member Author

it failed as well 😖

@pedrobaeza
Copy link
Member

I think the GIF is in a format that PIL doesn't load it. Can you reprocess it?

@chienandalu chienandalu force-pushed the 15.0-imp-web_notify-close-notification branch from a91a2e1 to 73299ec Compare September 20, 2024 11:06
@chienandalu
Copy link
Member Author

Changed to png. Let's try again

@pedrobaeza
Copy link
Member

But we are losing the animation then... I was just talking about opening the GIF and saving again with a software that allows it to reprocess the GIF to remove any offending bit.

@ap-wtioit
Copy link
Contributor

Not sure, but wanted to give a hint, this looks like pypa/readme_renderer#304 from the readme-renderer here: https://github.com/OCA/oca-github-bot/blob/master/requirements.txt#L48C8-L48C16

@chienandalu chienandalu force-pushed the 15.0-imp-web_notify-close-notification branch from 73299ec to cb0b2de Compare September 20, 2024 11:23
- We can now set a button name to the notification action.
- We can set an icon button as well.
- After the button is clicked, the notification gets closed.
@chienandalu chienandalu force-pushed the 15.0-imp-web_notify-close-notification branch from cb0b2de to a215ade Compare September 20, 2024 11:28
@chienandalu
Copy link
Member Author

Thanks @ap-wtioit ! I just pushed changes that avoid scaling those screenshots. Hopefully it will be ok now

@CarlosRoca13
Copy link
Contributor

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 15.0-ocabot-merge-pr-2789-by-CarlosRoca13-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit e1b85c4 into OCA:15.0 Sep 20, 2024
5 checks passed
@OCA-git-bot
Copy link
Contributor

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

@pedrobaeza pedrobaeza deleted the 15.0-imp-web_notify-close-notification branch September 20, 2024 14:13
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