Skip to content

Conversation

@BT-fgarbely
Copy link

No description provided.

sebalix and others added 21 commits January 20, 2017 07:46
…lete but slow) and Fast log (data input only, faster)
)

* [IMP] index the columns we'll be searching for for every request

* [FIX] singleton error if we saved the current session two times
- Update documentation to point to the new auditlog menu locations. These were changed because the 8.0 version was referencing menus that do not exist in 9.0
- Change version from 8.0.X.Y.Z to 9.0.1.0.0
- Make the module installable again
- Remove an unused parameter from pre-migration.py for versioning
- Fix typos and remove commented out blocks of code that were irrelevant
These were updated to follow OCA conventions.

- License set to AGPL-3
- Images set to empty array
Copy link
Contributor

@sebalix sebalix left a comment

Choose a reason for hiding this comment

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

@BT-fgarbely Good job! You even updated screenshots 🎉
Tested on runbot, it works fine (fast/full log, subscribe/unsubscribe rules...).
Can you squash all OCA Transbot commits into one?

@BT-fgarbely
Copy link
Author

Hello @sebalix

Thanks a lot for your feedback.

I don't know how to squash the commits.
I get the commits from the Branch 9.0 according to the migration guide:
https://github.com/OCA/maintainer-tools/wiki/Migration-to-version-10.0#technical-method-to-migrate-a-module-from-90-to-100-branch

Regards

@LoisRForgeFlow
Copy link
Contributor

Copy link
Contributor

@lasley lasley left a comment

Choose a reason for hiding this comment

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

Thanks @BT-fgarbely - few comments inline, but otherwise looks good.

Note on the squashing - please give it a shot first using the instructions in the wiki, because it is definitely a skill that is handy as a developer. If all else fails though, I will squash them for you before merge.

README.md Outdated
----------------
addon | version | summary
--- | --- | ---
[auditlog](auditlog/) | 10.0.1.0.0 | Audit Log
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this change - README.md is managed by a bot, so this change will create some conflicts.

'views/http_session_view.xml',
'views/http_request_view.xml',
],
'images': [],
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove empty key

@@ -0,0 +1,18 @@
<?xml version="1.0" encoding="utf-8"?>
<openerp>
Copy link
Contributor

Choose a reason for hiding this comment

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

<odoo noupdate="1"> & can remove the data tag

@BT-fgarbely
Copy link
Author

Hello @lasley and @lreficent

Thanks for your hints.
I changed the code according to your review and I tried to squash the Transbot commits.
It did something, but I don't know if it worked as expected.

Please can you have a look again?

Thanks a lot.

@lasley
Copy link
Contributor

lasley commented Feb 3, 2017

Hey @BT-fgarbely - Hrmmm looks like you merged the squashed items instead of force pushing them in. We're trying to rewrite timeline, so we don't want the merge.

No matter though, I'll handle for you. I recommend playing around with this a bit though, you will find that things end up a lot more organized afterwards. It also has the added benefit of making you look like you write perfect code, because there are no commits indicating PEP fixes, PR reviews, or troubleshooting rabbit holes you ended in.

@lasley
Copy link
Contributor

lasley commented Feb 3, 2017

@BT-fgarbely - please check the box to allow maintainers to edit your branch so that I may assist - https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

@lasley lasley added this to the 10.0 milestone Feb 3, 2017
@BT-fgarbely
Copy link
Author

Hello @lasley

I checked the "Allow edits from maintainers." checkbox.

Regards

@lasley
Copy link
Contributor

lasley commented Feb 10, 2017

Hrmmm I'm still getting a permissions error when pushing the squash. Not sure what's wrong if the Allow edits from maintainers is enabled 😕

@BT-fgarbely
Copy link
Author

Hello @lasley

This is really strange.
The flag "Allow edits from maintainers." in this pull request is checked.
auswahl_002

My fork is not created from the OCA repository. Our company has a fork from the OCA repository, and my fork comes from our company fork.
Maybe this could be a problem? I don't know. But in my understanding, this should be no problem.

Another question.
I saw that there is a pull request from @sebalix into my fork (BT-fgarbely#1). Should I merge this changes into my branch?
auswahl_001

Regards,

@sebalix
Copy link
Contributor

sebalix commented Feb 10, 2017

Hi,
I prefer to close the PR you mention, it was to port a fix from @StefanRijnhart but I also squash some commits, but I think I was doing wrong with this last point...

@lasley
Copy link
Contributor

lasley commented Feb 10, 2017

@BT-fgarbely Hrmm the check box is supposed to allow me to push to your fork, which is what I was trying. I'll give it another shot today, otherwise will give you some alternate instructions to just switch over to mine - there are many ways to skin a cat.

@sebalix Yeah the commit squashing will just add more commits into the mix here. What about the fix though, was it actually not valid?

@sebalix
Copy link
Contributor

sebalix commented Feb 10, 2017

@lasley no it is still valid, but we can port it after this PR is merged, this is just a cherrypick. No need to add extra work on this one.

@lasley
Copy link
Contributor

lasley commented Feb 10, 2017

Alright sweet - I still can't push to the fork here though. I'm actually kind of curious why & think we may be hitting a bug with Github's new feature, so I contacted them.

Side note - we just started using this module in vertical-medical the other day & functional tests are also good 👍

@lasley
Copy link
Contributor

lasley commented Feb 16, 2017

Hah well a test edit went through, but force push still isn't working. I'm working with Github support though, seems we may have hit some sort of edge case that now intrigues the hell out of me.

Will provide update soon!

@lasley
Copy link
Contributor

lasley commented Feb 17, 2017

@BT-fgarbely Github engineering has confirmed that this is an issue with the fork from a fork. They're working on it, but that's obviously going to take a while.

We can do one of two things here. I can either supersede this PR with my branch, or you can just add the LasLabs fork as a remote then do the following:

git remote update
git checkout 10.0-mig-auditlog
git reset --hard 7b8bd4d
git push -f

@BT-fgarbely
Copy link
Author

Hello @lasley

I added the LasLabs fork as remote and executed your suggested commands.

Regards

Copy link
Contributor

@lasley lasley left a comment

Choose a reason for hiding this comment

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

Thanks @BT-fgarbely

Copy link
Contributor

@sebalix sebalix left a comment

Choose a reason for hiding this comment

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

Cool, thank you @BT-fgarbely and @lasley . Tested on runbot once again, 👍

@lasley
Copy link
Contributor

lasley commented Feb 19, 2017

Merging, this has been approved for long enough - we were just battling with git 😆

@lasley lasley merged commit c111073 into OCA:10.0 Feb 19, 2017
@BT-fgarbely
Copy link
Author

Hello @lasley and @sebalix

Thanks a lot for your help and your support on this issue.

Regards

SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-tools (13.0)
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