Esteban/feature/editable configs #111
Merged
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
jcii
reviewed
Jun 26, 2022
| first() | ||
| ); | ||
| } | ||
|
|
||
| update(config: Config): Observable<Config> { | ||
| if (MS_NON_EDITABLE_CONFIGS_LOWER_CASE.includes(config.key.toLowerCase())) { |
There was a problem hiding this comment.
Does the backend make this same check ? You have to assume the UI is untrustworthy and check on the server-side too.
There was a problem hiding this comment.
I added a PR on the back end to validate
jcii
reviewed
Jun 26, 2022
| /** | ||
| * Model for Machete Setting Terms | ||
| */ | ||
| export class Term { |
There was a problem hiding this comment.
We're using Term here and Confirm in the backend?
There was a problem hiding this comment.
Yes. I have updated it to be OnlineOrderTerm
jcii
force-pushed
the
esteban/feature/editable_configs
branch
2 times, most recently
from
1c0627e to
63d9fa6
Compare
esteban-gs
commented
Nov 19, 2022
Comment on lines
+11
to
+41
| beforeEach(() => { | ||
| cy.apiLogin(MACHETE_ADMIN.user, MACHETE_ADMIN.password); | ||
|
|
||
| // Stubbing the response from server here | ||
| cy.fixture("settings/settings.json"); | ||
| cy.intercept( | ||
| { | ||
| method: "GET", | ||
| url: "/api/configs", | ||
| }, | ||
| { fixture: "settings/settings" } | ||
| ).as("configs"); | ||
| cy.visit(macheteSettingsRoutes.list); | ||
|
|
||
| cy.wait("@configs").then((i) => { | ||
| Cypress.env(STUBBED_CONFGIS_KEY, i.response.body.data); | ||
| const configs = Cypress.env(STUBBED_CONFGIS_KEY) as Config[]; | ||
|
|
||
| userEditableConfigs = configs.filter( | ||
| (c: Config) => | ||
| c.publicConfig && | ||
| !MS_NON_EDITABLE_CONFIGS_LOWER_CASE.includes(c.key.toLowerCase()) | ||
| ); | ||
|
|
||
| privateConfigs = configs.filter( | ||
| (c: Config) => | ||
| !c.publicConfig && | ||
| MS_NON_EDITABLE_CONFIGS_LOWER_CASE.includes(c.key.toLowerCase()) | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This is injecting test config data from fixtures/settings into the http response
esteban-gs
commented
Nov 19, 2022
Comment on lines
+28
to
+34
| cy.wait("@configs").then(() => { | ||
| configs = Cypress.env(ENV_KEY_MACHETE_CONFIGS) as Config[]; | ||
| userEditableConfigs = configs.filter( | ||
| (c: Config) => | ||
| c.publicConfig && | ||
| !MS_NON_EDITABLE_CONFIGS_LOWER_CASE.includes(c.key.toLowerCase()) | ||
| ); |
There was a problem hiding this comment.
Also injecting test data in the http response
esteban-gs
commented
Nov 19, 2022
Comment on lines
+23
to
+28
| path: "settings", | ||
| loadChildren: () => | ||
| import("./machete-settings/machete-settings.module").then( | ||
| (m) => m.MacheteSettingsModule | ||
| ), | ||
| }, |
There was a problem hiding this comment.
this lazy-loads the module
esteban-gs
commented
Nov 19, 2022
Comment on lines
+71
to
+92
| return this.http | ||
| .put(`${this.uriBase}/${config.id}`, config, { withCredentials: true }) | ||
| .pipe( | ||
| catchError((error) => { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||
| const errorAsText: string = error["statusText"] as string; | ||
| this.appMessages.showErrors({ | ||
| Error: `${errorAsText}: Contact Machete support.`, | ||
| }); | ||
| console.log(error); | ||
| return throwError(error); | ||
| }), | ||
| pluck("data"), | ||
| map((data) => data as Config), | ||
| tap(() => { | ||
| this.appMessages.showSuccess({ | ||
| label: "Success", | ||
| message: "Record Saved", | ||
| }); | ||
| }) | ||
| ); | ||
| } |
There was a problem hiding this comment.
catchError will break the stream and user will receive the feedback
remove unused bootstrap routing for edit component bootstrap edit component handle confirm save and cancel implement edit configs service Handle SAVE errors and return new reference on `getOne` Include description in template Exclude rows not editable by users user-defined settings - exclude from list - validate against edits Better titles define user-defined settings implement terms editor e2e testing stub settings reponses edit component unit tests fix confitional render better type names better file name better online order term naming
jcii
force-pushed
the
esteban/feature/editable_configs
branch
from
63d9fa6 to
2e41751
Compare
jcii
approved these changes
Nov 19, 2022
esteban-gs
commented
Nov 19, 2022
Comment on lines
+45
to
+67
| <div class="p-field p-col-12 p-input-filled" [hidden]="r.key === TERMS_KEY"> | ||
| <span class="p-float-label"> | ||
| <input | ||
| pInputText | ||
| [(ngModel)]="r.value" | ||
| id="value" | ||
| name="value" | ||
| required | ||
| ngModel | ||
| #value="ngModel" | ||
| /> | ||
| <label for="value">Value</label> | ||
| </span> | ||
| <small class="p-error" *ngIf="!r.value" | ||
| >Value is required</small | ||
| > | ||
| </div> | ||
| <div class="p-field p-col-12" *ngIf="r.key === TERMS_KEY"> | ||
| <app-machete-settings-term-form [termsAsString]="r.value" (termChange)="onChildTermChange($event, r)"></app-machete-settings-term-form> | ||
| </div> | ||
| </div> | ||
| </form> | ||
| </div> |
There was a problem hiding this comment.
logic to determine if form will display an input field or the onlineTerms form. The terms form is in a child component: app-machete-settings-term-form
esteban-gs
commented
Nov 19, 2022
| @@ -0,0 +1,119 @@ | |||
| <ng-container *ngIf="record$ | async as r"> | |||
There was a problem hiding this comment.
foo$ | async as bar aka async pipe is a new ng feature that handles the subscribe/unsubscribe logic automatically
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.
Changes:
Module for machete configs (called machete settings in ui) within the Configs module