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] dms_auto_classification: Add folder support within the .zip file #329

Merged

Conversation

victoralmau
Copy link
Member

@victoralmau victoralmau commented May 3, 2024

Add folder support within the .zip file

Changes done:

  • The full path is now shown in the detail line.
  • The dms file that will be created will be with the name (not the full file path).
  • When scanning the .zip files the folders are skipped.
  • Add more use cases in tests
  • Improve documentation

Please @pedrobaeza and @CarlosRoca13 can you review it?

@Tecnativa TT49047

@pedrobaeza pedrobaeza added this to the 15.0 milestone May 3, 2024
@pedrobaeza
Copy link
Member

You have to more specific with your commit messages. What "folder support" means in this case? If I'm not mistaking, what you are doing is to gather all the files, no matter if it's in any subfolder,so that's not strictly folder support to begin with, but as said, you have to be always more specific. "Devil is in the details". As said on our internal task, the ideal is that the pattern supports folders (in this case the denomination is correct). Meanwhile, you have to include this new behavior in the README and put the ROADMAP, although I would prefer to implement the feature at once. If not, migrating all the old classifications will mean more work than the implementation itself. Another thing you can do is a mix:

  • Add a check to indicate to "ignore folders", and apply this new algorithm.
  • Add the roadmap for adding the folders in the pattern.

This way, all the existing classification templates with the check will preserve its behavior.

@victoralmau
Copy link
Member Author

I comment a little about some issues.

1- Allow folders in patterns.
Actually this was possible before and it is possible now, re.search(pattern, filename) is used, so you can set the pattern you want to "filter" by folder or whatever you need. Let's not confuse this functionality (to use or not the file example.text with the name that will have the file that will be created).

2- Incorrect use of "folders" as files.
Previously we used namelist() that already returned all the files (including folders), and the filename contained the name of the folder (test1/test.txt for example).
folder (test1/test.txt for example).

All this has led to some fixes:

A- Folders should be skipped, for that reason infolist() is now used, which allows to know more easily if it is a folder or not.
B- When trying to create a file, only the file name should be used (therefore everything before / is omitted because it will refer to the folder regardless of the level).

Having clarified all this, are these fixes necessary? I think so, but if you think not I add it in ROADMAP as pending.

@pedrobaeza
Copy link
Member

pedrobaeza commented May 3, 2024

Having read what you put, and if I have understood well, the patch is not correct:

  • You should keep complete path in the preview popup.
  • As said, all the files shouldn't be considered by default. Add the check for having such behavior only by opt-in. Let's document the behavior in the README

@victoralmau victoralmau force-pushed the 15.0-imp-dms_auto_classification-TT49047 branch from 35a026b to db0bd40 Compare May 3, 2024 15:30
@victoralmau
Copy link
Member Author

Changes done and updated PR description.

dms_auto_classification/README.rst Show resolved Hide resolved
dms_auto_classification/README.rst Outdated Show resolved Hide resolved
Changes done:
- The full path is now shown in the detail line.
- The dms file that will be created will be with the name (not the full file path).
- When scanning the .zip files the folders are skipped.
- Add more use cases in tests
- Improve documentation

TT49047
@victoralmau victoralmau force-pushed the 15.0-imp-dms_auto_classification-TT49047 branch from db0bd40 to 75dd24c Compare May 6, 2024 10:30
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.

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 15.0-ocabot-merge-pr-329-by-pedrobaeza-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 4f33bdd into OCA:15.0 May 7, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@pedrobaeza pedrobaeza deleted the 15.0-imp-dms_auto_classification-TT49047 branch May 7, 2024 09:08
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.

None yet

4 participants