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

[ODOO-414][ADD] Added new view description to control panel area #20

Conversation

Jessica-BlueStingray
Copy link

Add report description

This update adds a description area and view_description field to the Control Panel between the name and the Search Area.

To test:
Upgrade the modules
Turn on debug mode
Click on the debug button > Edit Action > Add a text to view_description > Save > Refresh page.

@Jessica-BlueStingray
Copy link
Author

@amh-mw Travis reminded me that I should had made the view_description feature responsive to different screen sizes. I just added a commit that hides it when the screen is smaller. See video:

metricwise_add_report_description.mp4

@Jessica-BlueStingray Jessica-BlueStingray force-pushed the feature/15.0/19853/_add_report_description branch from f1a0152 to 3ebff71 Compare September 9, 2022 09:53
@Jessica-BlueStingray
Copy link
Author

@amh-mw PR is down to 1 commit now

@amh-mw amh-mw changed the title [BLUE-1][ADD] Added new view description to control panel area ODOO-393 Added new view description to control panel area Sep 12, 2022
@amh-mw
Copy link
Member

amh-mw commented Sep 12, 2022

This doesn't seem to want to patch with either -p1 or -p2. This is an unfortunate side effect of how Odoo changes the file hierarchy on disk as part of the Python packaging process. The "easy" fix would be to break this into two pull requests and patch one each way, but... 🤮 Let me think about it.

@amh-mw amh-mw changed the title ODOO-393 Added new view description to control panel area [ODOO-414][ADD] Added new view description to control panel area Sep 14, 2022
amh-mw
amh-mw previously requested changes Sep 14, 2022
Copy link
Member

@amh-mw amh-mw left a comment

Choose a reason for hiding this comment

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

So after carefully reviewing the patch -p1 output, I think the best course of action is to take the server side changes to ir.actions and move them into a separate odoo-metricwise pull requests -- see my untested ODOO-414 branch for a head start.

@amh-mw
Copy link
Member

amh-mw commented Sep 16, 2022

Just as a heads up, I am working up a new patching modality that may solve this without any changes. Stay tuned.

@Jessica-BlueStingray
Copy link
Author

Just as a heads up, I am working up a new patching modality that may solve this without any changes. Stay tuned.

Sorry I was really busy.
I'm currently waiting for the news on the new patching then. If that doesn't solve it I'll do the other changes required.

@amh-mw
Copy link
Member

amh-mw commented Sep 16, 2022

Ok, so the latest develop I just pushed adds a new patch.sh script that uses the GitHub CLI tool gh to figure out what patches to pull, then sed to modify their path hints so that -p1 works for all of them.

To test your patch, you can basically extract

DIR="docker/odoo/patches"
NUM="20"
URL="https://patch-diff.githubusercontent.com/raw/MetricWise/odoo/pull"

curl --output-dir $DIR --remote-name --silent --url "$URL/$NUM.diff"
sed -E -I '' -e 's%^(--- a|\+\+\+ b)/odoo%\1%' "$DIR/$NUM.diff"

And then ./run.sh -b or ./test.sh -b and if it looks good, git add -u docker/odoo/patches/20.diff

@Jessica-BlueStingray
Copy link
Author

@amh-mw The file was added correctly but I can't get past the sed command, I believe that's due to platform dependency, I'm using Linux. So I couldn't finish testing yet. Is there a sed command linux version I can use?

@traviswaelbro
Copy link

I believe Mac uses a slightly different version of sed (and similar commands) compared to Linux. There's a gnu-sed that can be installed with Brew that should run the same sed as Linux. It's installed as gsed by default, but can be aliased to sed if desired.

@amh-mw
Copy link
Member

amh-mw commented Sep 16, 2022

I can't say that I have ever used any unix flavor that didn't have sed. It's been around since 1973. 😁

Centos 6:

$ yum whatprovides `which sed`
sed-4.2.2-5.el7.x86_64 : A GNU stream text editor

Debian 11:

$ dpkg -l sed
sed            4.7-1        amd64        GNU stream editor for filtering/transforming text

@amh-mw
Copy link
Member

amh-mw commented Sep 16, 2022

Ah... so the issue is that your flavor of sed doesn't have -E for ERE? I'll just have to rewrite the pattern as BRE.

@Jessica-BlueStingray
Copy link
Author

Jessica-BlueStingray commented Sep 16, 2022

I'm not sure what the issue is, but the sed is not locating the file, although the file has been added correctly.
I think it is due to how the command is written, it might need to be different for linux. Sorry for my poor explanation earlier.
Screenshot from 2022-09-16 14-59-48

Screenshot from 2022-09-16 15-04-54

@amh-mw
Copy link
Member

amh-mw commented Sep 16, 2022

I've force pushed a BRE-compatible script to develop. Could you type which sed

@Jessica-BlueStingray
Copy link
Author

I've force pushed a BRE-compatible script to develop. Could you type which sed

Yes, I get /usr/bin/sed

@amh-mw
Copy link
Member

amh-mw commented Sep 16, 2022

Try the new patterns?

sed -i '' -e 's%^\(--- a\)/odoo%\1%' "$DIR/$NUM.diff"
sed -i '' -e 's%^\(+++ b\)/odoo%\1%' "$DIR/$NUM.diff"

@Jessica-BlueStingray
Copy link
Author

Still nothing

@amh-mw
Copy link
Member

amh-mw commented Sep 16, 2022

What flavor of unix are you running? Maybe I can stand up a container to try and reproduce?

@Jessica-BlueStingray
Copy link
Author

Linux 5.15.0-46-generic
Ubuntu 20.04.4 LTS

@amh-mw
Copy link
Member

amh-mw commented Sep 16, 2022

docker run ubuntu:focal sed --help

  -E, -r, --regexp-extended
                 use extended regular expressions in the script
                 (for portability use POSIX -E).

So you do have access to ERE. Maybe that's not the problem. Is your source code volume mounted read-only?

@Jessica-BlueStingray
Copy link
Author

Is your source code volume mounted read-only?

No, it's not

@Jessica-BlueStingray
Copy link
Author

@amh-mw is there anything else I can to do to help here?

@Jessica-BlueStingray
Copy link
Author

@amh-mw we managed it. It ran successfully.
The sed used in Ubuntu is gsed (GNU) for MAC, and command line used was slightly different than the original:

original: sed -E -I '' -e 's%^(--- a|\+\+\+ b)/odoo%\1%' "$DIR/$NUM.diff"
used: sed -E -i'' -e 's%^(--- a|\+\+\+ b)/odoo%\1%' "$DIR/$NUM.diff"

We removed the silent flag from the curl command line and found out that it was silently failing too due to https not being supported, then we changed to http and it was ok after that.

Both test and run were successful. Is there anything else we can do?

@amh-mw
Copy link
Member

amh-mw commented Sep 22, 2022

You should create a pull request against odoo-metricwise, now that your patch has been copied into the docker/odoo/patches folder.

@amh-mw amh-mw dismissed their stale review September 22, 2022 21:17

Dismiss review

@Jessica-BlueStingray Jessica-BlueStingray force-pushed the feature/15.0/19853/_add_report_description branch from 3ebff71 to e9fa865 Compare September 22, 2022 21:37
@Jessica-BlueStingray
Copy link
Author

Jessica-BlueStingray commented Sep 22, 2022

You should create a pull request against odoo-metricwise, now that your patch has been copied into the docker/odoo/patches folder.

I don't think I have permissions.. I don't see this repo here.

Screenshot from 2022-09-22 17-00-55

@traviswaelbro
Copy link

@Jessica-BlueStingray I think Adam is referring to a new PR in the other repo, similar to Dylan's on MetricWise/odoo-metricwise#8

@Jessica-BlueStingray
Copy link
Author

Like this? : Metricwise#11

@amh-mw
Copy link
Member

amh-mw commented Sep 26, 2022

Moved into #27.

@amh-mw amh-mw closed this Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants