wip-flag-migration #28
Merged
Alfred-mk
merged 44 commits from 2024-09-04 11:25:34 +02:00
wip-flag-migration into master
Dismiss Review
Are you sure you want to dismiss this review?
Labels
Clear labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
cleanup
devops
documentation
easypeasy
exchange
i18n
legacy
meta
migration
optimization
privilege
refactor
smell
support
tooling
ux
Breaking change that won't be backward compatible
Something is not working
Documentation changes
Improve existing functionality
New functionality
This is security issue
Issue or pull request related to testing
Priority
Critical
The priority is critical
Priority
High
The priority is high
Priority
Low
The priority is low
Priority
Medium
The priority is medium
Reviewed
Confirmed
Issue has been confirmed
Reviewed
Duplicate
This issue or pull request already exists
Reviewed
Invalid
Invalid issue
Reviewed
Won't Fix
This issue won't be fixed
Status
Abandoned
Somebody has started to work on this but abandoned work
Status
Blocked
Something is blocking this issue or pull request
Status
Need More Info
Feedback is required to reproduce issue or to continue work
Activity
Doing
An item actively being worked on currently
Activity
Hold
Not being actively worked on
Activity
Proposal
A proposal for a new feature or enhancement
Activity
QA
Ready for QA testing
Activity
Validate
Dev team validating changes
Runner
AT
Runner
CLI
Runner
HTTP
Runner
SSH
A task that can be completed with little effort
relating to swapping of assets and offramps
Languages and translations
Legacy features from the old ussd system to be ported
container for other issues
features that have to do with special, privileged axx
Code smell
transparency and help with exception states
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: urdt/ussd#28
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Silly, I made the pull request so the review can only be marked as comment...
@@ -54,0 +48,4 @@}// Register all flags loaded from pp.csvflagKeys := []string{these flags should be loaded not hardcoded.
@@ -54,0 +67,4 @@"flag_incorrect_date_format",}for _, key := range flagKeys {iterate on csv lines and populate with rows 2 and 3...
@@ -38,2 +66,3 @@func NewHandlers(path string, st *state.State) *Handlers {func NewHandlers(dir string, st *state.State) (*Handlers, error) {db, err := gdbm.Open(dbFile, gdbm.ModeWrcreat)FYI this will truncate the file on every open.
... but lets not spend time on that, as it needs to be changed to the new gdbm implementation anyway.
So a question on this,we want to be able to keep a reference to the file created for each session and be able to initialize a database instance depending on its existence.So we have this code and it still seems to be removing the previously stored information when it is opened with mode ModeWriter .A fix for the time being for this will be appreciated because the refactoring depends on storing the user information to gdbm.
First, as I said, let's not spend time on this, since this code will be gone when you implement db.gdbmDb in dev-0.1.0.
That said, choosing flag on os.Stat would be the way to go, I guess.
As a reference example on how to do it directly, see
go-visebranchdev-0.1.0methoddb/gdbm/gdbmDb.Connect(...)But I repeat, please don't spend time on this.
@@ -60,1 +98,4 @@func (h *Handlers) PreloadFlags(flagKeys []string) (map[string]uint32, error) {flags := make(map[string]uint32)for _, key := range flagKeys {It shouldn't be necessary to process the flags twice.
@@ -85,2 +143,3 @@err := h.accountFileHandler.EnsureFileExists()// Preload the required flagsflagKeys := []string{"flag_account_created", "flag_account_creation_failed"}also seems unnecessary, there should be one single source of flag label to value map.
For clarity, do you mean that we should load all flags once in the menuhandler and have functions access the already loaded flags?
yes, or perhaps even globally.
@@ -88,3 +148,4 @@return res, err}// err = h.accountFileHandler.EnsureFileExists()please remove commented code if it is not of any more use
@@ -156,3 +222,2 @@case "2":res.FlagReset = append(res.FlagSet, models.USERFLAG_ALLOW_UPDATE)res.FlagSet = append(res.FlagSet, models.USERFLAG_SINGLE_EDIT)res.FlagReset = append(res.FlagSet, flags["flag_allow_update"])here appends to set but assign to reset. happens in the below, too.
@@ -546,1 +629,3 @@return res, errerr = h.db.Delete([]byte(Amount))if err != nil && !errors.Is(err, gdbm.ErrItemNotFound) {panic(err)we cannot have panics anywhere in the vm execution! Just return error.
@@ -54,0 +71,4 @@}// Register the flagstate.FlagDebugger.Register(uint32(flagValue), flagName)A logline to show debug the flag loaded would be nice
@@ -32,0 +56,4 @@// FlagManager handles centralized flag managementtype FlagManager struct {parser *asm.FlagParsermu sync.RWMutexI don't think mutex is needed here, it will be loaded once (before threads) and only read from then right?
@@ -87,0 +110,4 @@ussdHandlers, err := ussd.NewHandlers(fp, &st, sessionId)if err != nil {fmt.Fprintf(os.Stderr, "handler setup failed with error: %v\n", err)Exit in main if any setup fails.