Settings > Application Settings > Replication tab enhancements

[elsamaryv]

Converts Settings > Replication tab to React. Also includes jest and cypress tests.

Before

Add subscription

Validation failure

After

Add subscription modal

Subscription table

Validation failure

Edit subscription

[GilbertCherrie]

Also, the cypress tests are consistently failing, we need to get them passing some what consistently or we can't merge this

[Fryguy]

@elsamaryv Is this ready to go? @GilbertCherrie can you review?

[elsamaryv]

@elsamaryv Is this ready to go? @GilbertCherrie can you review?

Not yet, the PR needs to be reviewed again.

[Fryguy]

Overall, this looks great - awesome work. I think there's a little cleanup here, but overall I'm good. @asirvadAbrahamVarghese can you please also review, particularly the cypress tests as I didn't dig deep into those.

[asirvadAbrahamVarghese]

Could you also remove the Carbon v10 classes (bx--) and the pull-right class as well? - that shouldn’t have any impact and isn't necessary since we’re already aligning that using flex (also, that’s a mix of Bootstrap and Carbon)

[asirvadAbrahamVarghese]

I could be wrong here, but shouldn’t the Save button be disabled when the type select has no value (i.e., set to None)?

[elsamaryv]

Initially, the dropdown shows 'None', and the Save button is disabled because no replication role has been selected yet. However, if the user changes the replication type and later decides to set it back to 'None', that action should still be allowed.

[asirvadAbrahamVarghese]

Could be a bug, or I might have misunderstood the functionality

I believe this was also done previously by someone else, adding a new subscription preserves the previous state (the last entered data)

Adding a Subscription:

Then trying to add another one:

Previous record values are populated automatically:

[asirvadAbrahamVarghese]

The modal’s submit button should have its text as “Accept” or “Add.” I’ve seen “Add” more often, but I’m not entirely sure

[elsamaryv]

Before -

After -

"Accept" button doesn't doesn't trigger any API calls. In the angular code, it only stores the subscription data to the local array. Would it make sense to rename it to "Save" in the subscription modal?

@Fryguy

[elsamaryv]

Previous workflow -

Replication.workflow.with.angular.code.mov

[Fryguy]

I don't really have a preference. If we're moving to a modal, it makes sense to match the pattern of other modal's, which sounds like Add is the better choice. I don't like Save in the modal because it doesn't actually save anywhere until the entire form is saved, so Add is better.

[asirvadAbrahamVarghese]

Not sure about Jest, do we actually need it? I thought the idea was to eventually remove Jest completely and move fully to Cypress(including component testing).

cc: @GilbertCherrie @Fryguy

[Fryguy]

Jest is convenient for running unit tests, whereas Cypress is more for integration tests - the former is fast and easier to run locally, particularly for small functions - the latter is better for testing across boundaries, particularly without the mocking hitting the real API and Rails backend

They both have their place and I'm ok with both kinds of tests where appropriate.

[GilbertCherrie]

Yeah we should still have both Jest and Cypress tests as part of our react conversion

[asirvadAbrahamVarghese]

Alright, makes sense. In that case we’ll probably hit Enzyme as a blocker soon, especially with the React upgrade since it likely won’t support React 18+. We’ll need to move to RTL

[asirvadAbrahamVarghese]

Could we usecy.getFormButtonByTypeWithText here?

[asirvadAbrahamVarghese]

Actually, that might end up selecting buttons from the main form, right?

[asirvadAbrahamVarghese]

May be we can use cy.getFormButtonByTypeWithText here

[asirvadAbrahamVarghese]

Just a thought, if you feel adding data through the UI isn’t necessary everywhere, using factories can be a big help.

[asirvadAbrahamVarghese]

We also have a command for table data assertions: cy.gtlGetRows. It can return data from the table based on the column index which can make this easier.

[asirvadAbrahamVarghese]

Just to confirm, should reset also wipe the table data?

Now selecting the type as "Global" again repopulates the data, is that expected?

[Fryguy]

Reset should go to what it was before any changes were made. If the table had data already, then it should restore the original data. Code-wise a reset could just be implemented as a reload of the component.

[asirvadAbrahamVarghese]

Thanks, this looks great now, lit work - just one small question here

• @GilbertCherrie I believe you already reviewed this before Carbon11 migration, could you take another quick look?
• @Fryguy could you also check the comment about the modal button name here?

[Fryguy]

Thanks - I replied above.

[GilbertCherrie]

@elsamaryv looks good. The cypress test failures are sporatic and unrelated.

The only 2 minor issues I found in the PR:

1. You don't need a cancel button on this form. I checked the old form and there is no cancel button there. This can be removed using the <MiqFormRenderer> prop cancancel={false}.
2. Not sure what we want to do with the reset button here as pointed out above. @Fryguy the old form only resets new data (such as new rows added to the table) where as the new form resets the entire form back to None. Do we want to try and replicate the old form Reset?

[Fryguy]

Reset should reset back to whatever is saved in the database. If the table had data at the start it should reset back to that.

[miq-bot]

Checked commit elsamaryv@a2993cd with ruby 3.3.10, rubocop 1.56.3, haml-lint 0.72.0, and yamllint 1.37.1
3 files checked, 0 offenses detected
Everything looks fine. ⭐

[elsamaryv]

@elsamaryv looks good. The cypress test failures are sporatic and unrelated.

The only 2 minor issues I found in the PR:

1. You don't need a cancel button on this form. I checked the old form and there is no cancel button there. This can be removed using the <MiqFormRenderer> prop cancancel={false}.
2. Not sure what we want to do with the reset button here as pointed out above. @Fryguy the old form only resets new data (such as new rows added to the table) where as the new form resets the entire form back to None. Do we want to try and replicate the old form Reset?

The Cancel button has been removed, and the Reset button is now functioning as expected—it restores the values to what’s currently saved in the database. Please review, @GilbertCherrie