Pull request dedicated to support analysis and integration of the functional modules designed for ANAC project 2024 #4313 #4314
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…t for decrypting reports
…[email protected]/servizi-interni/whistleblowing/globaleaks.git #Conflicts: # backend/globaleaks/handlers/auth/__init__.py
…t for decrypting reports
This was referenced Nov 21, 2024
evilaliv3
changed the title
evilaliv3
commented
Nov 23, 2024
| @@ -679,8 +728,10 @@ class _Mail(Model): | |||
| address = Column(UnicodeText, nullable=False) | |||
| subject = Column(UnicodeText, nullable=False) | |||
| body = Column(UnicodeText, nullable=False) | |||
| is_pec = Column(Boolean, nullable=False, default=False) | |||
There was a problem hiding this comment.
TODO: to be renamed secondary_smtp that is the minor fix that wont cause any code rewrite except a variable renaming.
Change needed to abstract the possibility of having a PEC system by enabling to have two SMTP configurations, one primary and one secondary to address the problem of internationalization detailed in #4328
evilaliv3
commented
Nov 23, 2024
| @@ -874,7 +936,7 @@ class _Subscriber(Model): | |||
| surname = Column(UnicodeText, nullable=False) | |||
| phone = Column(UnicodeText, default='', nullable=False) | |||
| email = Column(UnicodeText, nullable=False) | |||
| organization_name = Column(UnicodeText, default='', nullable=False) | |||
| organization_name = Column(UnicodeText, default='', nullable=False, unique=True) | |||
There was a problem hiding this comment.
This change might not be included as known to have caused some problems:
- If an organization register and do not get approved they wont be able to register again.
- If a fake user user attempt a registration on behalf of a real organization, the real organization wont be able to register again
evilaliv3
commented
Nov 23, 2024
| admin_name = Column(UnicodeText, nullable=True) | ||
| admin_surname = Column(UnicodeText, nullable=True) | ||
| admin_email = Column(UnicodeText, nullable=True) | ||
| admin_fiscal_code = Column(UnicodeText, nullable=True) |
There was a problem hiding this comment.
TODO: rename this admin_tax_code (as the existent oranization_tax_code) that is an abstraction that we use at international level.
evilaliv3
commented
Nov 23, 2024
| admin_surname = Column(UnicodeText, nullable=True) | ||
| admin_email = Column(UnicodeText, nullable=True) | ||
| admin_fiscal_code = Column(UnicodeText, nullable=True) | ||
| recipient_fiscal_code = Column(UnicodeText, nullable=True) |
There was a problem hiding this comment.
TODO: rename this recipient_tax_code (as the existant oranization_tax_code) that is an abstraction that we could use at international level.
evilaliv3
commented
Nov 23, 2024
| 'client_ip_address', 'client_user_agent', 'state', 'organization_email', | ||
| 'organization_institutional_site', 'admin_name', 'admin_surname', 'admin_email', | ||
| 'admin_fiscal_code', 'recipient_name', 'recipient_surname', 'recipient_email', | ||
| 'recipient_fiscal_code', 'sharing_id'] |
There was a problem hiding this comment.
POSSIBLE BUG: recipient_name, recipient_surname seems not defined.
evilaliv3
commented
Nov 23, 2024
| can_redact_information = Column(Boolean, default=False, nullable=False) | ||
| can_mask_information = Column(Boolean, default=True, nullable=False) | ||
| can_reopen_reports = Column(Boolean, default=True, nullable=False) | ||
| can_edit_general_settings = Column(Boolean, default=False, nullable=False) | ||
| readonly = Column(Boolean, default=False, nullable=False) | ||
| two_factor_secret = Column(UnicodeText(32), default='', nullable=False) | ||
| reminder_date = Column(DateTime, default=datetime_null, nullable=False) | ||
| status = Column(Enum(EnumUserStatus), default='active', nullable=False) | ||
| fiscal_code = Column(UnicodeText(18), default='', nullable=False) |
There was a problem hiding this comment.
TODO: to be renamed idp_id that is the abstraction we could use at international level as inidicated in #4107
evilaliv3
commented
Nov 23, 2024
| admin_email = Column(UnicodeText, nullable=True) | ||
| admin_fiscal_code = Column(UnicodeText, nullable=True) | ||
| recipient_fiscal_code = Column(UnicodeText, nullable=True) | ||
| sharing_id = Column(UnicodeText(36), nullable=False, default=uuid4) |
There was a problem hiding this comment.
TODO: remove, already subscriber.tid equals tenant.id acting as a unique id and reference making this attribute fully redundant.
Changes: - Renamed user.fiscal_code in user.idp_id to abstract the variable name in international scope
Changes: - Add 'antivirus_enabled' variable to make it possible to enable/disable the functionality via UI - Edit the set of 'antivirus' variables to enable type checking on the configuration of the ClamD endpoint
Changes: - Performed code review of the variables name choice to keep codebase uniform
Changes: - replace 'is_pec' variable with 'use_secondary_smtp' to perform international abstraction of 'pec' using just the concept of smtp - replace 'fiscal_code' variables 'tax_code' convention used in the software as international level - remove 'sharing_id' addition redundant considering existing subscriber.tid equals tenant.id as unique reference - add 'smtp2_enabled' to make it possible to enable/disable the smtp2 configuration - add missing variables recipient_* on signup table planned to host name for a subscriber - uniform variables for subscriber user (used to instantiate the admin) and the new recipient variables
Changes: - Add 'forwarding' enabler variable to enable/disable forwarding module at site level (default false) - Renamed 'eo_internaltip_id' to 'forwarding_internaltip_id' considering future possible exention of the the functionality to enable to forward reports both to other sites regardless they are external or affiliated
evilaliv3
force-pushed
the
devel
branch
7 times, most recently
from
3f8f509 to
0b3dfc0
Compare
evilaliv3
force-pushed
the
devel
branch
2 times, most recently
from
151ae61 to
57e664c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm opening this pull request to kick of the analysis of the possible integration of some external implementations for works performed for the Italian National Authority for Anticorruption.
This pull request is solely intended to provide an overall review intended to support the integration of the functional modules included in the provided codebase.
The work of integration is tracked by Milestone 5.1.0.
Following the project practices to proceed with the code integration the code will be split in separated by functional module with corresponding dedicated branches and pull requests:
\cc @joeman65 @alessiofranceschini
Review summary
Backend tests: failure 1/237
Client tests: failure 3/24
To proceed with an integration it is needed to proceed fixing them. In addition it currently seems that too few test have been written in comparison to the large amount of functionalities introduced into the software as proven by the high regression in the coverage. Examples of missing tests include the forwarding module that would probably require a client test simulating the operations of the different users involved, tests for the backup and tests for the antivirus functionalities.
While this value could be impacted by current tests failures we remind that for a possible integration every implementation requires to include relevant tests to verify its functionality and the project standard allows up to 0.5% of regression.
The code includes new features, edited features, review of unrelated code, and removal/re-addition of already existing code.
To ensure review quality, it is necessary to split this into multiple pull requests, separating features by branch (e.g., addition of antivirus, accreditor, or reports generation are independent functionalities).
Also, correct mistakes such as the removal and re-addition of original project code.
This indicates the pull request has not been verified for integration before proposal.
This complexity makes currently difficult to understand the changes. To possibly integrate each functionality every functional module should be proposed for integration in a dedicated and self-contained pull request .
Example of this is "accreditation-request" functionality seems to rely on some unfinished or not-committed work in relation to the IDP functionality .. Currently when running the code the applications redirects to a not existing /onboarding route. To proceed with the integration we should wait for the completion or isolate the functionality and the components that rely on it postponing their integration.